[PATCH v14 00/11] Add support for Hikey 970 PCIe

[PATCH v14 00/11] Add support for Hikey 970 PCIe

[Mauro Carvalho Chehab]

Hi Lorenzo,

I addressed the issues you pointed on this review.

The pcie-kirin PCIe driver contains internally a PHY interface for
Kirin 960, but it misses support for Kirin 970. A new PHY driver
for it was added at drivers/phy/hisilicon/phy-hi3670-pcie.c
(already merged via PHY tree).

Add support for Kirin 970 PHY driver at the pcie-kirin.c.

While here, also add the needed logic to compile it as module and
to allow to dynamically remove the driver in runtime.

Tested on HiKey970:

  # lspci -D -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)

Tested on HiKey960:

  # lspci -D 
  0000:00:00.0 PCI bridge: Huawei Technologies Co., Ltd. Device 3660 (rev 01)

---

Diff from v13:

@@ -455,7 +455,7 @@ 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;
+       struct device_node *child, *node = dev->of_node;
        void __iomem *apb_base;
        int ret;
 
@@ -687,9 +687,8 @@ static int kirin_pcie_power_off(struct kirin_pcie *kirin_pcie)
        if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
                return hi3660_pcie_phy_power_off(kirin_pcie);
 
-       for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++) {
+       for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++)
                gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 1);
-       }
 
        phy_power_off(kirin_pcie->phy);
        phy_exit(kirin_pcie->phy);
@@ -790,7 +789,7 @@ static int kirin_pcie_probe(struct platform_device *pdev)
                return -EINVAL;
        }
 
-       phy_type = (enum pcie_kirin_phy_type)of_id->data;
+       phy_type = (long)of_id->data;
 
        kirin_pcie = devm_kzalloc(dev, sizeof(struct kirin_pcie), GFP_KERNEL);
        if (!kirin_pcie)

v14:
  - Split a timeout logic from patch 4, placing it on a separate patch;
  - Added fixes: and cc: tags to the power_off fixup patch;
  - change a typecast from of_data to long, in order to avoid a warning on
   some randconfigs;
  - removed uneeded brackets at the power_off patch;
  - reordered struct device pointers at kirin_pcie_get_resource();
  - added a c/c to kishon at the PHY-related patches.

v13:
  - Added Xiaowei's ack for the series.

v12:
  - Change a comment at patch 1 to not use c99 style.

v11:
  - patch 5 changed to use the right PCIe topology
  - all other patches are identical to v10.

v10:
  - patch 1: dropped magic numbers from PHY driver
  - patch 5: allow pcie child nodes without reset-gpios
  - all other patches are identical to v9.

v9:
  - Did some cleanups at patches 1 and 5

Mauro Carvalho Chehab (11):
  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 support for bridge slot DT schema
  PCI: kirin: give more time for PERST# reset to finish
  PCI: kirin: Add Kirin 970 compatible
  PCI: kirin: Add MODULE_* macros
  PCI: kirin: Allow building it as a module
  PCI: kirin: Add power_off support for Kirin 960 PHY
  PCI: kirin: fix poweroff sequence
  PCI: kirin: Allow removing the driver

 drivers/pci/controller/dwc/Kconfig      |   2 +-
 drivers/pci/controller/dwc/pcie-kirin.c | 643 ++++++++++++++++++------
 2 files changed, 497 insertions(+), 148 deletions(-)

-- 
2.31.1

[PATCH v14 01/11] PCI: kirin: Reorganize the PHY logic inside the driver

[Mauro Carvalho Chehab]

The pcie-kirin PCIe driver contains internally a PHY interface for
Kirin 960.

As the next patches will add support for using an external PHY
driver, reorganize the driver in a way that the PHY part
will be self-contained.

This could be moved to a separate PHY driver, but a change
like that would mean a non-backward-compatible DT schema
change.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 298 ++++++++++++++----------
 1 file changed, 173 insertions(+), 125 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 026fd1e42a55..b4063a3434df 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -28,26 +28,16 @@
 
 #define to_kirin_pcie(x) dev_get_drvdata((x)->dev)
 
-#define REF_CLK_FREQ			100000000
-
 /* PCIe ELBI registers */
 #define SOC_PCIECTRL_CTRL0_ADDR		0x000
 #define SOC_PCIECTRL_CTRL1_ADDR		0x004
-#define SOC_PCIEPHY_CTRL2_ADDR		0x008
-#define SOC_PCIEPHY_CTRL3_ADDR		0x00c
 #define PCIE_ELBI_SLV_DBI_ENABLE	(0x1 << 21)
 
 /* info located in APB */
 #define PCIE_APP_LTSSM_ENABLE	0x01c
-#define PCIE_APB_PHY_CTRL0	0x0
-#define PCIE_APB_PHY_CTRL1	0x4
 #define PCIE_APB_PHY_STATUS0	0x400
 #define PCIE_LINKUP_ENABLE	(0x8020)
 #define PCIE_LTSSM_ENABLE_BIT	(0x1 << 11)
-#define PIPE_CLK_STABLE		(0x1 << 19)
-#define PHY_REF_PAD_BIT		(0x1 << 8)
-#define PHY_PWR_DOWN_BIT	(0x1 << 22)
-#define PHY_RST_ACK_BIT		(0x1 << 16)
 
 /* info located in sysctrl */
 #define SCTRL_PCIE_CMOS_OFFSET	0x60
@@ -60,6 +50,29 @@
 #define PCIE_DEBOUNCE_PARAM	0xF0F400
 #define PCIE_OE_BYPASS		(0x3 << 28)
 
+struct kirin_pcie {
+	struct dw_pcie	*pci;
+	struct phy	*phy;
+	void __iomem	*apb_base;
+	void		*phy_priv;	/* Needed for Kirin 960 PHY */
+};
+
+/*
+ * Kirin 960 PHY. Can't be split into a PHY driver without changing the
+ * DT schema.
+ */
+
+#define REF_CLK_FREQ			100000000
+
+/* PHY info located in APB */
+#define PCIE_APB_PHY_CTRL0	0x0
+#define PCIE_APB_PHY_CTRL1	0x4
+#define PCIE_APB_PHY_STATUS0   0x400
+#define PIPE_CLK_STABLE		BIT(19)
+#define PHY_REF_PAD_BIT		BIT(8)
+#define PHY_PWR_DOWN_BIT	BIT(22)
+#define PHY_RST_ACK_BIT		BIT(16)
+
 /* peri_crg ctrl */
 #define CRGCTRL_PCIE_ASSERT_OFFSET	0x88
 #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
@@ -69,8 +82,6 @@
 #define REF_2_PERST_MAX		25000
 #define PERST_2_ACCESS_MIN	10000
 #define PERST_2_ACCESS_MAX	12000
-#define LINK_WAIT_MIN		900
-#define LINK_WAIT_MAX		1000
 #define PIPE_CLK_WAIT_MIN	550
 #define PIPE_CLK_WAIT_MAX	600
 #define TIME_CMOS_MIN		100
@@ -78,118 +89,112 @@
 #define TIME_PHY_PD_MIN		10
 #define TIME_PHY_PD_MAX		11
 
-struct kirin_pcie {
-	struct dw_pcie	*pci;
-	void __iomem	*apb_base;
-	void __iomem	*phy_base;
+struct hi3660_pcie_phy {
+	struct device	*dev;
+	void __iomem	*base;
 	struct regmap	*crgctrl;
 	struct regmap	*sysctrl;
 	struct clk	*apb_sys_clk;
 	struct clk	*apb_phy_clk;
 	struct clk	*phy_ref_clk;
-	struct clk	*pcie_aclk;
-	struct clk	*pcie_aux_clk;
+	struct clk	*aclk;
+	struct clk	*aux_clk;
 	int		gpio_id_reset;
 };
 
-/* 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);
-}
-
 /* Registers in PCIePHY */
-static inline void kirin_apb_phy_writel(struct kirin_pcie *kirin_pcie,
+static inline void kirin_apb_phy_writel(struct hi3660_pcie_phy *hi3660_pcie_phy,
 					u32 val, u32 reg)
 {
-	writel(val, kirin_pcie->phy_base + reg);
+	writel(val, hi3660_pcie_phy->base + reg);
 }
 
-static inline u32 kirin_apb_phy_readl(struct kirin_pcie *kirin_pcie, u32 reg)
+static inline u32 kirin_apb_phy_readl(struct hi3660_pcie_phy *hi3660_pcie_phy,
+				      u32 reg)
 {
-	return readl(kirin_pcie->phy_base + reg);
+	return readl(hi3660_pcie_phy->base + reg);
 }
 
-static long kirin_pcie_get_clk(struct kirin_pcie *kirin_pcie,
-			       struct platform_device *pdev)
+static int hi3660_pcie_phy_get_clk(struct hi3660_pcie_phy *phy)
 {
-	struct device *dev = &pdev->dev;
+	struct device *dev = phy->dev;
 
-	kirin_pcie->phy_ref_clk = devm_clk_get(dev, "pcie_phy_ref");
-	if (IS_ERR(kirin_pcie->phy_ref_clk))
-		return PTR_ERR(kirin_pcie->phy_ref_clk);
+	phy->phy_ref_clk = devm_clk_get(dev, "pcie_phy_ref");
+	if (IS_ERR(phy->phy_ref_clk))
+		return PTR_ERR(phy->phy_ref_clk);
 
-	kirin_pcie->pcie_aux_clk = devm_clk_get(dev, "pcie_aux");
-	if (IS_ERR(kirin_pcie->pcie_aux_clk))
-		return PTR_ERR(kirin_pcie->pcie_aux_clk);
+	phy->aux_clk = devm_clk_get(dev, "pcie_aux");
+	if (IS_ERR(phy->aux_clk))
+		return PTR_ERR(phy->aux_clk);
 
-	kirin_pcie->apb_phy_clk = devm_clk_get(dev, "pcie_apb_phy");
-	if (IS_ERR(kirin_pcie->apb_phy_clk))
-		return PTR_ERR(kirin_pcie->apb_phy_clk);
+	phy->apb_phy_clk = devm_clk_get(dev, "pcie_apb_phy");
+	if (IS_ERR(phy->apb_phy_clk))
+		return PTR_ERR(phy->apb_phy_clk);
 
-	kirin_pcie->apb_sys_clk = devm_clk_get(dev, "pcie_apb_sys");
-	if (IS_ERR(kirin_pcie->apb_sys_clk))
-		return PTR_ERR(kirin_pcie->apb_sys_clk);
+	phy->apb_sys_clk = devm_clk_get(dev, "pcie_apb_sys");
+	if (IS_ERR(phy->apb_sys_clk))
+		return PTR_ERR(phy->apb_sys_clk);
 
-	kirin_pcie->pcie_aclk = devm_clk_get(dev, "pcie_aclk");
-	if (IS_ERR(kirin_pcie->pcie_aclk))
-		return PTR_ERR(kirin_pcie->pcie_aclk);
+	phy->aclk = devm_clk_get(dev, "pcie_aclk");
+	if (IS_ERR(phy->aclk))
+		return PTR_ERR(phy->aclk);
 
 	return 0;
 }
 
-static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
-				    struct platform_device *pdev)
+static int hi3660_pcie_phy_get_resource(struct hi3660_pcie_phy *phy)
 {
-	kirin_pcie->apb_base =
-		devm_platform_ioremap_resource_byname(pdev, "apb");
-	if (IS_ERR(kirin_pcie->apb_base))
-		return PTR_ERR(kirin_pcie->apb_base);
-
-	kirin_pcie->phy_base =
-		devm_platform_ioremap_resource_byname(pdev, "phy");
-	if (IS_ERR(kirin_pcie->phy_base))
-		return PTR_ERR(kirin_pcie->phy_base);
-
-	kirin_pcie->crgctrl =
-		syscon_regmap_lookup_by_compatible("hisilicon,hi3660-crgctrl");
-	if (IS_ERR(kirin_pcie->crgctrl))
-		return PTR_ERR(kirin_pcie->crgctrl);
-
-	kirin_pcie->sysctrl =
-		syscon_regmap_lookup_by_compatible("hisilicon,hi3660-sctrl");
-	if (IS_ERR(kirin_pcie->sysctrl))
-		return PTR_ERR(kirin_pcie->sysctrl);
+	struct device *dev = phy->dev;
+	struct platform_device *pdev;
+
+	/* registers */
+	pdev = container_of(dev, struct platform_device, dev);
+
+	phy->base = devm_platform_ioremap_resource_byname(pdev, "phy");
+	if (IS_ERR(phy->base))
+		return PTR_ERR(phy->base);
+
+	phy->crgctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3660-crgctrl");
+	if (IS_ERR(phy->crgctrl))
+		return PTR_ERR(phy->crgctrl);
+
+	phy->sysctrl = syscon_regmap_lookup_by_compatible("hisilicon,hi3660-sctrl");
+	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;
 }
 
-static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie)
+static int hi3660_pcie_phy_start(struct hi3660_pcie_phy *phy)
 {
-	struct device *dev = kirin_pcie->pci->dev;
+	struct device *dev = phy->dev;
 	u32 reg_val;
 
-	reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL1);
+	reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_CTRL1);
 	reg_val &= ~PHY_REF_PAD_BIT;
-	kirin_apb_phy_writel(kirin_pcie, reg_val, PCIE_APB_PHY_CTRL1);
+	kirin_apb_phy_writel(phy, reg_val, PCIE_APB_PHY_CTRL1);
 
-	reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL0);
+	reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_CTRL0);
 	reg_val &= ~PHY_PWR_DOWN_BIT;
-	kirin_apb_phy_writel(kirin_pcie, reg_val, PCIE_APB_PHY_CTRL0);
+	kirin_apb_phy_writel(phy, reg_val, PCIE_APB_PHY_CTRL0);
 	usleep_range(TIME_PHY_PD_MIN, TIME_PHY_PD_MAX);
 
-	reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_CTRL1);
+	reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_CTRL1);
 	reg_val &= ~PHY_RST_ACK_BIT;
-	kirin_apb_phy_writel(kirin_pcie, reg_val, PCIE_APB_PHY_CTRL1);
+	kirin_apb_phy_writel(phy, reg_val, PCIE_APB_PHY_CTRL1);
 
 	usleep_range(PIPE_CLK_WAIT_MIN, PIPE_CLK_WAIT_MAX);
-	reg_val = kirin_apb_phy_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
+	reg_val = kirin_apb_phy_readl(phy, PCIE_APB_PHY_STATUS0);
 	if (reg_val & PIPE_CLK_STABLE) {
 		dev_err(dev, "PIPE clk is not stable\n");
 		return -EINVAL;
@@ -198,105 +203,157 @@ static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie)
 	return 0;
 }
 
-static void kirin_pcie_oe_enable(struct kirin_pcie *kirin_pcie)
+static void hi3660_pcie_phy_oe_enable(struct hi3660_pcie_phy *phy)
 {
 	u32 val;
 
-	regmap_read(kirin_pcie->sysctrl, SCTRL_PCIE_OE_OFFSET, &val);
+	regmap_read(phy->sysctrl, SCTRL_PCIE_OE_OFFSET, &val);
 	val |= PCIE_DEBOUNCE_PARAM;
 	val &= ~PCIE_OE_BYPASS;
-	regmap_write(kirin_pcie->sysctrl, SCTRL_PCIE_OE_OFFSET, val);
+	regmap_write(phy->sysctrl, SCTRL_PCIE_OE_OFFSET, val);
 }
 
-static int kirin_pcie_clk_ctrl(struct kirin_pcie *kirin_pcie, bool enable)
+static int hi3660_pcie_phy_clk_ctrl(struct hi3660_pcie_phy *phy, bool enable)
 {
 	int ret = 0;
 
 	if (!enable)
 		goto close_clk;
 
-	ret = clk_set_rate(kirin_pcie->phy_ref_clk, REF_CLK_FREQ);
+	ret = clk_set_rate(phy->phy_ref_clk, REF_CLK_FREQ);
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(kirin_pcie->phy_ref_clk);
+	ret = clk_prepare_enable(phy->phy_ref_clk);
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(kirin_pcie->apb_sys_clk);
+	ret = clk_prepare_enable(phy->apb_sys_clk);
 	if (ret)
 		goto apb_sys_fail;
 
-	ret = clk_prepare_enable(kirin_pcie->apb_phy_clk);
+	ret = clk_prepare_enable(phy->apb_phy_clk);
 	if (ret)
 		goto apb_phy_fail;
 
-	ret = clk_prepare_enable(kirin_pcie->pcie_aclk);
+	ret = clk_prepare_enable(phy->aclk);
 	if (ret)
 		goto aclk_fail;
 
-	ret = clk_prepare_enable(kirin_pcie->pcie_aux_clk);
+	ret = clk_prepare_enable(phy->aux_clk);
 	if (ret)
 		goto aux_clk_fail;
 
 	return 0;
 
 close_clk:
-	clk_disable_unprepare(kirin_pcie->pcie_aux_clk);
+	clk_disable_unprepare(phy->aux_clk);
 aux_clk_fail:
-	clk_disable_unprepare(kirin_pcie->pcie_aclk);
+	clk_disable_unprepare(phy->aclk);
 aclk_fail:
-	clk_disable_unprepare(kirin_pcie->apb_phy_clk);
+	clk_disable_unprepare(phy->apb_phy_clk);
 apb_phy_fail:
-	clk_disable_unprepare(kirin_pcie->apb_sys_clk);
+	clk_disable_unprepare(phy->apb_sys_clk);
 apb_sys_fail:
-	clk_disable_unprepare(kirin_pcie->phy_ref_clk);
+	clk_disable_unprepare(phy->phy_ref_clk);
 
 	return ret;
 }
 
-static int kirin_pcie_power_on(struct kirin_pcie *kirin_pcie)
+static int hi3660_pcie_phy_power_on(struct kirin_pcie *pcie)
 {
+	struct hi3660_pcie_phy *phy = pcie->phy_priv;
 	int ret;
 
 	/* Power supply for Host */
-	regmap_write(kirin_pcie->sysctrl,
+	regmap_write(phy->sysctrl,
 		     SCTRL_PCIE_CMOS_OFFSET, SCTRL_PCIE_CMOS_BIT);
 	usleep_range(TIME_CMOS_MIN, TIME_CMOS_MAX);
-	kirin_pcie_oe_enable(kirin_pcie);
 
-	ret = kirin_pcie_clk_ctrl(kirin_pcie, true);
+	hi3660_pcie_phy_oe_enable(phy);
+
+	ret = hi3660_pcie_phy_clk_ctrl(phy, true);
 	if (ret)
 		return ret;
 
 	/* ISO disable, PCIeCtrl, PHY assert and clk gate clear */
-	regmap_write(kirin_pcie->sysctrl,
+	regmap_write(phy->sysctrl,
 		     SCTRL_PCIE_ISO_OFFSET, SCTRL_PCIE_ISO_BIT);
-	regmap_write(kirin_pcie->crgctrl,
+	regmap_write(phy->crgctrl,
 		     CRGCTRL_PCIE_ASSERT_OFFSET, CRGCTRL_PCIE_ASSERT_BIT);
-	regmap_write(kirin_pcie->sysctrl,
+	regmap_write(phy->sysctrl,
 		     SCTRL_PCIE_HPCLK_OFFSET, SCTRL_PCIE_HPCLK_BIT);
 
-	ret = kirin_pcie_phy_init(kirin_pcie);
+	ret = hi3660_pcie_phy_start(phy);
 	if (ret)
-		goto close_clk;
+		goto disable_clks;
 
 	/* perst assert Endpoint */
-	if (!gpio_request(kirin_pcie->gpio_id_reset, "pcie_perst")) {
+	if (!gpio_request(phy->gpio_id_reset, "pcie_perst")) {
 		usleep_range(REF_2_PERST_MIN, REF_2_PERST_MAX);
-		ret = gpio_direction_output(kirin_pcie->gpio_id_reset, 1);
+		ret = gpio_direction_output(phy->gpio_id_reset, 1);
 		if (ret)
-			goto close_clk;
+			goto disable_clks;
 		usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
-
 		return 0;
 	}
 
-close_clk:
-	kirin_pcie_clk_ctrl(kirin_pcie, false);
+disable_clks:
+	hi3660_pcie_phy_clk_ctrl(phy, false);
 	return ret;
 }
 
+static int hi3660_pcie_phy_init(struct platform_device *pdev,
+				struct kirin_pcie *pcie)
+{
+	struct device *dev = &pdev->dev;
+	struct hi3660_pcie_phy *phy;
+	int ret;
+
+	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
+	if (!phy)
+		return -ENOMEM;
+
+	pcie->phy_priv = phy;
+	phy->dev = dev;
+
+	/* registers */
+	pdev = container_of(dev, struct platform_device, dev);
+
+	ret = hi3660_pcie_phy_get_clk(phy);
+	if (ret)
+		return ret;
+
+	return hi3660_pcie_phy_get_resource(phy);
+}
+
+/*
+ * The non-PHY part starts here
+ */
+
+/* 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)
+{
+	kirin_pcie->apb_base =
+		devm_platform_ioremap_resource_byname(pdev, "apb");
+	if (IS_ERR(kirin_pcie->apb_base))
+		return PTR_ERR(kirin_pcie->apb_base);
+
+	return 0;
+}
+
 static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie,
 					   bool on)
 {
@@ -444,7 +501,7 @@ static int kirin_pcie_probe(struct platform_device *pdev)
 	pci->pp.ops = &kirin_pcie_host_ops;
 	kirin_pcie->pci = pci;
 
-	ret = kirin_pcie_get_clk(kirin_pcie, pdev);
+	ret = hi3660_pcie_phy_init(pdev, kirin_pcie);
 	if (ret)
 		return ret;
 
@@ -452,16 +509,7 @@ static int kirin_pcie_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	kirin_pcie->gpio_id_reset = of_get_named_gpio(dev->of_node,
-						      "reset-gpios", 0);
-	if (kirin_pcie->gpio_id_reset == -EPROBE_DEFER) {
-		return -EPROBE_DEFER;
-	} else if (!gpio_is_valid(kirin_pcie->gpio_id_reset)) {
-		dev_err(dev, "unable to get a valid gpio pin\n");
-		return -ENODEV;
-	}
-
-	ret = kirin_pcie_power_on(kirin_pcie);
+	ret = hi3660_pcie_phy_power_on(kirin_pcie);
 	if (ret)
 		return ret;
 
@@ -479,8 +527,8 @@ static struct platform_driver kirin_pcie_driver = {
 	.probe			= kirin_pcie_probe,
 	.driver			= {
 		.name			= "kirin-pcie",
-		.of_match_table = kirin_pcie_match,
-		.suppress_bind_attrs = true,
+		.of_match_table		= kirin_pcie_match,
+		.suppress_bind_attrs	= true,
 	},
 };
 builtin_platform_driver(kirin_pcie_driver);
-- 
2.31.1

[PATCH v14 02/11] PCI: kirin: Add support for a PHY layer

[Mauro Carvalho Chehab]

The pcie-kirin driver contains both PHY and generic PCI driver
on it.

The best would be, instead, to support a PCI PHY driver, making
the driver more generic.

However, it is too late to remove the Kirin 960 PHY, as a change
like that would make the DT schema incompatible with past versions.

So, add support for an external PHY driver without removing the
existing Kirin 960 PHY from it.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 95 +++++++++++++++++++++----
 1 file changed, 80 insertions(+), 15 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index b4063a3434df..91a7c096bf8f 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -8,16 +8,18 @@
  * Author: Xiaowei Song <songxiaowei@huawei.com>
  */
 
-#include <linux/compiler.h>
 #include <linux/clk.h>
+#include <linux/compiler.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio.h>
 #include <linux/interrupt.h>
 #include <linux/mfd/syscon.h>
 #include <linux/of_address.h>
+#include <linux/of_device.h>
 #include <linux/of_gpio.h>
 #include <linux/of_pci.h>
+#include <linux/phy/phy.h>
 #include <linux/pci.h>
 #include <linux/pci_regs.h>
 #include <linux/platform_device.h>
@@ -50,11 +52,18 @@
 #define PCIE_DEBOUNCE_PARAM	0xF0F400
 #define PCIE_OE_BYPASS		(0x3 << 28)
 
+enum pcie_kirin_phy_type {
+	PCIE_KIRIN_INTERNAL_PHY,
+	PCIE_KIRIN_EXTERNAL_PHY
+};
+
 struct kirin_pcie {
+	enum pcie_kirin_phy_type	type;
+
 	struct dw_pcie	*pci;
 	struct phy	*phy;
 	void __iomem	*apb_base;
-	void		*phy_priv;	/* Needed for Kirin 960 PHY */
+	void		*phy_priv;	/* only for PCIE_KIRIN_INTERNAL_PHY */
 };
 
 /*
@@ -476,8 +485,63 @@ static const struct dw_pcie_host_ops kirin_pcie_host_ops = {
 	.host_init = kirin_pcie_host_init,
 };
 
+static int kirin_pcie_power_on(struct platform_device *pdev,
+			       struct kirin_pcie *kirin_pcie)
+{
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY) {
+		ret = hi3660_pcie_phy_init(pdev, kirin_pcie);
+		if (ret)
+			return ret;
+
+		return hi3660_pcie_phy_power_on(kirin_pcie);
+	}
+
+	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;
+
+	return 0;
+err:
+	phy_exit(kirin_pcie->phy);
+	return ret;
+}
+
+static int __exit kirin_pcie_remove(struct platform_device *pdev)
+{
+	struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
+
+	if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
+		return 0;
+
+	phy_power_off(kirin_pcie->phy);
+	phy_exit(kirin_pcie->phy);
+
+	return 0;
+}
+
+static const struct of_device_id kirin_pcie_match[] = {
+	{
+		.compatible = "hisilicon,kirin960-pcie",
+		.data = (void *)PCIE_KIRIN_INTERNAL_PHY
+	},
+	{},
+};
+
 static int kirin_pcie_probe(struct platform_device *pdev)
 {
+	enum pcie_kirin_phy_type phy_type;
+	const struct of_device_id *of_id;
 	struct device *dev = &pdev->dev;
 	struct kirin_pcie *kirin_pcie;
 	struct dw_pcie *pci;
@@ -488,6 +552,14 @@ static int kirin_pcie_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	of_id = of_match_device(kirin_pcie_match, dev);
+	if (!of_id) {
+		dev_err(dev, "OF data missing\n");
+		return -EINVAL;
+	}
+
+	phy_type = (long)of_id->data;
+
 	kirin_pcie = devm_kzalloc(dev, sizeof(struct kirin_pcie), GFP_KERNEL);
 	if (!kirin_pcie)
 		return -ENOMEM;
@@ -500,31 +572,24 @@ static int kirin_pcie_probe(struct platform_device *pdev)
 	pci->ops = &kirin_dw_pcie_ops;
 	pci->pp.ops = &kirin_pcie_host_ops;
 	kirin_pcie->pci = pci;
-
-	ret = hi3660_pcie_phy_init(pdev, kirin_pcie);
-	if (ret)
-		return ret;
+	kirin_pcie->type = phy_type;
 
 	ret = kirin_pcie_get_resource(kirin_pcie, pdev);
 	if (ret)
 		return ret;
 
-	ret = hi3660_pcie_phy_power_on(kirin_pcie);
-	if (ret)
-		return ret;
-
 	platform_set_drvdata(pdev, kirin_pcie);
 
+	ret = kirin_pcie_power_on(pdev, kirin_pcie);
+	if (ret)
+		return ret;
+
 	return dw_pcie_host_init(&pci->pp);
 }
 
-static const struct of_device_id kirin_pcie_match[] = {
-	{ .compatible = "hisilicon,kirin960-pcie" },
-	{},
-};
-
 static struct platform_driver kirin_pcie_driver = {
 	.probe			= kirin_pcie_probe,
+	.remove	        	= __exit_p(kirin_pcie_remove),
 	.driver			= {
 		.name			= "kirin-pcie",
 		.of_match_table		= kirin_pcie_match,
-- 
2.31.1

[PATCH v14 03/11] PCI: kirin: Use regmap for APB registers

[Mauro Carvalho Chehab]

The PHY layer need to access APB registers too, for Kirin 970.
So, place them into a named regmap.

Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 49 +++++++++++++------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 91a7c096bf8f..86c13661e02d 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -61,8 +61,8 @@ struct kirin_pcie {
 	enum pcie_kirin_phy_type	type;
 
 	struct dw_pcie	*pci;
+	struct regmap   *apb;
 	struct phy	*phy;
-	void __iomem	*apb_base;
 	void		*phy_priv;	/* only for PCIE_KIRIN_INTERNAL_PHY */
 };
 
@@ -340,25 +340,27 @@ static int hi3660_pcie_phy_init(struct platform_device *pdev,
  * The non-PHY part starts here
  */
 
-/* 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 const struct regmap_config pcie_kirin_regmap_conf = {
+	.name = "kirin_pcie_apb",
+	.reg_bits = 32,
+	.val_bits = 32,
+	.reg_stride = 4,
+};
 
 static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 				    struct platform_device *pdev)
 {
-	kirin_pcie->apb_base =
-		devm_platform_ioremap_resource_byname(pdev, "apb");
-	if (IS_ERR(kirin_pcie->apb_base))
-		return PTR_ERR(kirin_pcie->apb_base);
+	struct device *dev = &pdev->dev;
+	void __iomem *apb_base;
+
+	apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
+	if (IS_ERR(apb_base))
+		return PTR_ERR(apb_base);
+
+	kirin_pcie->apb = devm_regmap_init_mmio(dev, apb_base,
+						&pcie_kirin_regmap_conf);
+	if (IS_ERR(kirin_pcie->apb))
+		return PTR_ERR(kirin_pcie->apb);
 
 	return 0;
 }
@@ -368,13 +370,13 @@ static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie,
 {
 	u32 val;
 
-	val = kirin_apb_ctrl_readl(kirin_pcie, SOC_PCIECTRL_CTRL0_ADDR);
+	regmap_read(kirin_pcie->apb, SOC_PCIECTRL_CTRL0_ADDR, &val);
 	if (on)
 		val = val | PCIE_ELBI_SLV_DBI_ENABLE;
 	else
 		val = val & ~PCIE_ELBI_SLV_DBI_ENABLE;
 
-	kirin_apb_ctrl_writel(kirin_pcie, val, SOC_PCIECTRL_CTRL0_ADDR);
+	regmap_write(kirin_pcie->apb, SOC_PCIECTRL_CTRL0_ADDR, val);
 }
 
 static void kirin_pcie_sideband_dbi_r_mode(struct kirin_pcie *kirin_pcie,
@@ -382,13 +384,13 @@ static void kirin_pcie_sideband_dbi_r_mode(struct kirin_pcie *kirin_pcie,
 {
 	u32 val;
 
-	val = kirin_apb_ctrl_readl(kirin_pcie, SOC_PCIECTRL_CTRL1_ADDR);
+	regmap_read(kirin_pcie->apb, SOC_PCIECTRL_CTRL1_ADDR, &val);
 	if (on)
 		val = val | PCIE_ELBI_SLV_DBI_ENABLE;
 	else
 		val = val & ~PCIE_ELBI_SLV_DBI_ENABLE;
 
-	kirin_apb_ctrl_writel(kirin_pcie, val, SOC_PCIECTRL_CTRL1_ADDR);
+	regmap_write(kirin_pcie->apb, SOC_PCIECTRL_CTRL1_ADDR, val);
 }
 
 static int kirin_pcie_rd_own_conf(struct pci_bus *bus, unsigned int devfn,
@@ -448,8 +450,9 @@ static void kirin_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base,
 static int kirin_pcie_link_up(struct dw_pcie *pci)
 {
 	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
-	u32 val = kirin_apb_ctrl_readl(kirin_pcie, PCIE_APB_PHY_STATUS0);
+	u32 val;
 
+	regmap_read(kirin_pcie->apb, PCIE_APB_PHY_STATUS0, &val);
 	if ((val & PCIE_LINKUP_ENABLE) == PCIE_LINKUP_ENABLE)
 		return 1;
 
@@ -461,8 +464,8 @@ static int kirin_pcie_start_link(struct dw_pcie *pci)
 	struct kirin_pcie *kirin_pcie = to_kirin_pcie(pci);
 
 	/* assert LTSSM enable */
-	kirin_apb_ctrl_writel(kirin_pcie, PCIE_LTSSM_ENABLE_BIT,
-			      PCIE_APP_LTSSM_ENABLE);
+	regmap_write(kirin_pcie->apb, PCIE_APP_LTSSM_ENABLE,
+		     PCIE_LTSSM_ENABLE_BIT);
 
 	return 0;
 }
-- 
2.31.1

[PATCH v14 04/11] PCI: kirin: Add support for bridge slot DT schema

[Mauro Carvalho Chehab]

On HiKey970, there's a PEX 8606 PCI bridge on its PHY with
6 lanes. Only 4 lanes are connected:

	lane 0 - connected to Kirin 970;
	lane 4 - M.2 slot;
	lane 5 - mini PCIe slot;
	lane 6 - in-board Ethernet controller.

Each lane has its own PERST# gpio pin, and needs a clock
request.

Add support to parse a DT schema containing the above data.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
 1 file changed, 231 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 86c13661e02d..de375795a3b8 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,19 @@ 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;
+
+	/* Per-slot PERST# */
+	int		num_slots;
+	int		gpio_id_reset[MAX_PCI_SLOTS];
+	const char	*reset_names[MAX_PCI_SLOTS];
+
+	/* Per-slot clkreq */
+	int		n_gpio_clkreq;
+	int		gpio_id_clkreq[MAX_PCI_SLOTS];
+	const char	*clkreq_names[MAX_PCI_SLOTS];
 };
 
 /*
@@ -108,7 +134,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 +196,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,15 +312,7 @@ 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;
-	}
+	return 0;
 
 disable_clks:
 	hi3660_pcie_phy_clk_ctrl(phy, false);
@@ -347,11 +354,98 @@ static const struct regmap_config pcie_kirin_regmap_conf = {
 	.reg_stride = 4,
 };
 
+static int kirin_pcie_get_gpio_enable(struct kirin_pcie *pcie,
+				      struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	char name[32];
+	int ret, i;
+
+	/* This is an optional property */
+	ret = of_gpio_named_count(np, "hisilicon,clken-gpios");
+	if (ret < 0)
+		return 0;
+
+	if (ret > MAX_PCI_SLOTS) {
+		dev_err(dev, "Too many GPIO clock requests!\n");
+		return -EINVAL;
+	}
+
+	pcie->n_gpio_clkreq = ret;
+
+	for (i = 0; i < pcie->n_gpio_clkreq; i++) {
+		pcie->gpio_id_clkreq[i] = of_get_named_gpio(dev->of_node,
+							    "hisilicon,clken-gpios", i);
+		if (pcie->gpio_id_clkreq[i] < 0)
+			return pcie->gpio_id_clkreq[i];
+
+		sprintf(name, "pcie_clkreq_%d", i);
+		pcie->clkreq_names[i] = devm_kstrdup_const(dev, name,
+							    GFP_KERNEL);
+		if (!pcie->clkreq_names[i])
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int kirin_pcie_parse_port(struct kirin_pcie *pcie,
+				 struct platform_device *pdev,
+				 struct device_node *node)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *parent, *child;
+	int ret, slot, i;
+	char name[32];
+
+	for_each_available_child_of_node(node, parent) {
+		for_each_available_child_of_node(parent, child) {
+			i = pcie->num_slots;
+
+			pcie->gpio_id_reset[i] = of_get_named_gpio(child,
+								"reset-gpios", 0);
+			if (pcie->gpio_id_reset[i] < 0)
+				continue;
+
+			pcie->num_slots++;
+			if (pcie->num_slots > MAX_PCI_SLOTS) {
+				dev_err(dev, "Too many PCI slots!\n");
+				return -EINVAL;
+			}
+
+			ret = of_pci_get_devfn(child);
+			if (ret < 0) {
+				dev_err(dev, "failed to parse devfn: %d\n", ret);
+				goto put_node;
+			}
+
+			slot = PCI_SLOT(ret);
+
+			sprintf(name, "pcie_perst_%d", slot);
+			pcie->reset_names[i] = devm_kstrdup_const(dev, name,
+								GFP_KERNEL);
+			if (!pcie->reset_names[i]) {
+				ret = -ENOMEM;
+				goto put_node;
+			}
+		}
+	}
+
+	return 0;
+
+put_node:
+	of_node_put(child);
+	return ret;
+}
+
 static long kirin_pcie_get_resource(struct kirin_pcie *kirin_pcie,
 				    struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *child, *node = dev->of_node;
 	void __iomem *apb_base;
+	int ret;
 
 	apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
 	if (IS_ERR(apb_base))
@@ -362,7 +456,32 @@ 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;
+	}
+
+	ret = kirin_pcie_get_gpio_enable(kirin_pcie, pdev);
+	if (ret)
+		return ret;
+
+	/* Parse OF children */
+	for_each_available_child_of_node(node, child) {
+		ret = kirin_pcie_parse_port(kirin_pcie, pdev, child);
+		if (ret)
+			goto put_node;
+	}
+
 	return 0;
+
+put_node:
+	of_node_put(child);
+	return ret;
 }
 
 static void kirin_pcie_sideband_dbi_w_mode(struct kirin_pcie *kirin_pcie,
@@ -419,9 +538,33 @@ 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;
+
+	if (!kirin_pcie->num_slots)
+		return 0;
+
+	/* 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) {
+			dev_err(pci->dev, "PERST# %s error: %d\n",
+				kirin_pcie->reset_names[i], 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 +620,44 @@ 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->n_gpio_clkreq; 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;
+
+		ret = gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 0);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static const struct dw_pcie_ops kirin_dw_pcie_ops = {
 	.read_dbi = kirin_pcie_read_dbi,
 	.write_dbi = kirin_pcie_write_dbi,
@@ -499,24 +680,43 @@ static int kirin_pcie_power_on(struct platform_device *pdev,
 		if (ret)
 			return ret;
 
-		return hi3660_pcie_phy_power_on(kirin_pcie);
+		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 = kirin_pcie_gpio_request(kirin_pcie, dev);
+		if (ret)
+			return ret;
+
+		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_bridge")) {
+		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;
 }
 
-- 
2.31.1

Re: [PATCH v14 04/11] PCI: kirin: Add support for bridge slot DT schema

[Bjorn Helgaas]

On Tue, Oct 19, 2021 at 07:06:41AM +0100, Mauro Carvalho Chehab wrote:
> On HiKey970, there's a PEX 8606 PCI bridge on its PHY with
> 6 lanes. Only 4 lanes are connected:
> 
> 	lane 0 - connected to Kirin 970;
> 	lane 4 - M.2 slot;
> 	lane 5 - mini PCIe slot;
> 	lane 6 - in-board Ethernet controller.
> 
> Each lane has its own PERST# gpio pin, and needs a clock
> request.
> 
> Add support to parse a DT schema containing the above data.
> 
> Cc: Kishon Vijay Abraham I <kishon@ti.com>
> Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> 
> See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> 
>  drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
>  1 file changed, 231 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index 86c13661e02d..de375795a3b8 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.
> ...

> +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;
> +
> +	if (!kirin_pcie->num_slots)
> +		return 0;
> +
> +	/* 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) {
> +			dev_err(pci->dev, "PERST# %s error: %d\n",
> +				kirin_pcie->reset_names[i], 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,

This seems a little weird.  Can you educate me?

From [1], it looks like the topology here is:

  00:00.0 Root Port
  01:00.0 PEX 8606 Upstream Port
  02:01.0 PEX 8606 Downstream Port
  02:04.0 PEX 8606 Downstream Port
  02:05.0 PEX 8606 Downstream Port
  02:07.0 PEX 8606 Downstream Port
  02:09.0 PEX 8606 Downstream Port
  06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller 

The .add_bus() callback will be called for *every* child bus we want
to enumerate.  So if any of those PEX 8606 Downstream Ports are
connected to another switch, when we enumerate the secondary buses of
that other switch, it looks like we'll send PERST# to all the slots
again, which doesn't sound right.  Am I missing something?

Bjorn

[1] https://lore.kernel.org/all/20210129173057.30288c9d@coco.lan/

Re: [PATCH v14 04/11] PCI: kirin: Add support for bridge slot DT schema

[Bjorn Helgaas]

[resending with updated address for Mauro]

On Tue, May 24, 2022 at 12:19:50PM -0500, Bjorn Helgaas wrote:
> On Tue, Oct 19, 2021 at 07:06:41AM +0100, Mauro Carvalho Chehab wrote:
> > On HiKey970, there's a PEX 8606 PCI bridge on its PHY with
> > 6 lanes. Only 4 lanes are connected:
> > 
> > 	lane 0 - connected to Kirin 970;
> > 	lane 4 - M.2 slot;
> > 	lane 5 - mini PCIe slot;
> > 	lane 6 - in-board Ethernet controller.
> > 
> > Each lane has its own PERST# gpio pin, and needs a clock
> > request.
> > 
> > Add support to parse a DT schema containing the above data.
> > 
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > 
> > See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> > 
> >  drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
> >  1 file changed, 231 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > index 86c13661e02d..de375795a3b8 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.
> > ...
> 
> > +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;
> > +
> > +	if (!kirin_pcie->num_slots)
> > +		return 0;
> > +
> > +	/* 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) {
> > +			dev_err(pci->dev, "PERST# %s error: %d\n",
> > +				kirin_pcie->reset_names[i], 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,
> 
> This seems a little weird.  Can you educate me?
> 
> From [1], it looks like the topology here is:
> 
>   00:00.0 Root Port
>   01:00.0 PEX 8606 Upstream Port
>   02:01.0 PEX 8606 Downstream Port
>   02:04.0 PEX 8606 Downstream Port
>   02:05.0 PEX 8606 Downstream Port
>   02:07.0 PEX 8606 Downstream Port
>   02:09.0 PEX 8606 Downstream Port
>   06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller 
> 
> The .add_bus() callback will be called for *every* child bus we want
> to enumerate.  So if any of those PEX 8606 Downstream Ports are
> connected to another switch, when we enumerate the secondary buses of
> that other switch, it looks like we'll send PERST# to all the slots
> again, which doesn't sound right.  Am I missing something?
> 
> Bjorn
> 
> [1] https://lore.kernel.org/all/20210129173057.30288c9d@coco.lan/

Re: [PATCH v14 04/11] PCI: kirin: Add support for bridge slot DT schema

[Mauro Carvalho Chehab]

Hi Bjorn,

Em Tue, 24 May 2022 12:19:47 -0500
Bjorn Helgaas <helgaas@kernel.org> escreveu:

> On Tue, Oct 19, 2021 at 07:06:41AM +0100, Mauro Carvalho Chehab wrote:
> > On HiKey970, there's a PEX 8606 PCI bridge on its PHY with
> > 6 lanes. Only 4 lanes are connected:
> > 
> > 	lane 0 - connected to Kirin 970;
> > 	lane 4 - M.2 slot;
> > 	lane 5 - mini PCIe slot;
> > 	lane 6 - in-board Ethernet controller.
> > 
> > Each lane has its own PERST# gpio pin, and needs a clock
> > request.
> > 
> > Add support to parse a DT schema containing the above data.
> > 
> > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > 
> > See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> > 
> >  drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
> >  1 file changed, 231 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > index 86c13661e02d..de375795a3b8 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.
> > ...  
> 
> > +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;
> > +
> > +	if (!kirin_pcie->num_slots)
> > +		return 0;
> > +
> > +	/* 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) {
> > +			dev_err(pci->dev, "PERST# %s error: %d\n",
> > +				kirin_pcie->reset_names[i], 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,  
> 
> This seems a little weird.  Can you educate me?
> 
> From [1], it looks like the topology here is:
> 
>   00:00.0 Root Port
>   01:00.0 PEX 8606 Upstream Port
>   02:01.0 PEX 8606 Downstream Port
>   02:04.0 PEX 8606 Downstream Port
>   02:05.0 PEX 8606 Downstream Port
>   02:07.0 PEX 8606 Downstream Port
>   02:09.0 PEX 8606 Downstream Port
>   06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller 
> 
> The .add_bus() callback will be called for *every* child bus we want
> to enumerate.  So if any of those PEX 8606 Downstream Ports are
> connected to another switch, when we enumerate the secondary buses of
> that other switch, it looks like we'll send PERST# to all the slots
> again, which doesn't sound right.  Am I missing something?

The implementation made on Kirin 960/970 for PCI is to not have a
PERST# bus. Instead, it has one independent GPIO driving the PERST#
signal for each single individual port that is physically connected.

See the schematics at:

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

Yet, my first proposal were to send all GPIOs altogether. See, for
instance:

	https://lore.kernel.org/all/3acf2c073e8ea67efaae91074dda0763bf7a2ab9.1626768323.git.mchehab+huawei@kernel.org/

There, the PERST# signals are initialized altogether, at the end
of hi3670_pcie_phy_power_on():

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

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

	hi3670_pcie_set_eyeparam(phy);

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

During the review process, I was requested to change it in order to do it
via .add_bus. On my tests at the boards, I didn't see the same PERST#
signal to hit more than once, and the driver is working fine. 
So, this wasn't an actual issue, as far as I can tell. 

That's what it ended getting merged upstream.

I don't mind if somewone wants to return it to use the previous approach of
sending all per-port PERST# signals at the same time, just before calling 
is_pipe_clk_stable(), like the above. However, I don't have access to the 
hardware anymore, so I can't test any patches for it.

Thanks,
Mauro

Re: [PATCH v14 04/11] PCI: kirin: Add support for bridge slot DT schema

[Bjorn Helgaas]

[+cc Yicong, Rafael, just FYI; trying to figure out how native host
bridge drivers should do power management]

On Tue, May 24, 2022 at 09:55:41PM +0200, Mauro Carvalho Chehab wrote:
> Em Tue, 24 May 2022 12:19:47 -0500
> Bjorn Helgaas <helgaas@kernel.org> escreveu:
> > On Tue, Oct 19, 2021 at 07:06:41AM +0100, Mauro Carvalho Chehab wrote:
> > > On HiKey970, there's a PEX 8606 PCI bridge on its PHY with
> > > 6 lanes. Only 4 lanes are connected:
> > > 
> > > 	lane 0 - connected to Kirin 970;
> > > 	lane 4 - M.2 slot;
> > > 	lane 5 - mini PCIe slot;
> > > 	lane 6 - in-board Ethernet controller.
> > > 
> > > Each lane has its own PERST# gpio pin, and needs a clock
> > > request.
> > > 
> > > Add support to parse a DT schema containing the above data.
> > > 
> > > Cc: Kishon Vijay Abraham I <kishon@ti.com>
> > > Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > > 
> > > See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> > > 
> > >  drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
> > >  1 file changed, 231 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > > index 86c13661e02d..de375795a3b8 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.
> > > ...  
> > 
> > > +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;
> > > +
> > > +	if (!kirin_pcie->num_slots)
> > > +		return 0;
> > > +
> > > +	/* 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) {
> > > +			dev_err(pci->dev, "PERST# %s error: %d\n",
> > > +				kirin_pcie->reset_names[i], 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,  
> > 
> > This seems a little weird.  Can you educate me?
> > 
> > From [1], it looks like the topology here is:
> > 
> >   00:00.0 Root Port
> >   01:00.0 PEX 8606 Upstream Port
> >   02:01.0 PEX 8606 Downstream Port
> >   02:04.0 PEX 8606 Downstream Port
> >   02:05.0 PEX 8606 Downstream Port
> >   02:07.0 PEX 8606 Downstream Port
> >   02:09.0 PEX 8606 Downstream Port
> >   06:00.0 RTL8111/8168/8411 PCI Express Gigabit Ethernet Controller 
> > 
> > The .add_bus() callback will be called for *every* child bus we want
> > to enumerate.  So if any of those PEX 8606 Downstream Ports are
> > connected to another switch, when we enumerate the secondary buses of
> > that other switch, it looks like we'll send PERST# to all the slots
> > again, which doesn't sound right.  Am I missing something?
> 
> The implementation made on Kirin 960/970 for PCI is to not have a
> PERST# bus. Instead, it has one independent GPIO driving the PERST#
> signal for each single individual port that is physically connected.
> 
> See the schematics at:
> 
> 	- https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/files/hikey970-schematics.pdf
> 	- https://github.com/96boards/documentation/blob/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf
> 
> Yet, my first proposal were to send all GPIOs altogether. See, for
> instance:
> 
> 	https://lore.kernel.org/all/3acf2c073e8ea67efaae91074dda0763bf7a2ab9.1626768323.git.mchehab+huawei@kernel.org/
> 
> There, the PERST# signals are initialized altogether, at the end
> of hi3670_pcie_phy_power_on():
> 
> 	/* perst assert Endpoints */
> 	usleep_range(21000, 23000);
> 	for (i = 0; i < phy->n_gpio_resets; i++) {
> 		ret = gpio_direction_output(phy->gpio_id_reset[i], 1);
> 		if (ret)
> 			return ret;
> 	}
> 	usleep_range(10000, 11000);
> 
> 	ret = is_pipe_clk_stable(phy);
> 	if (!ret)
> 		goto disable_clks;
> 
> 	hi3670_pcie_set_eyeparam(phy);
> 
> 	ret = hi3670_pcie_noc_power(phy, false);
> 	if (ret)
> 		goto disable_clks;
> 
> During the review process, I was requested to change it in order to do it
> via .add_bus. 

I see Rob suggested .add_bus() at
https://lore.kernel.org/all/CAL_JsqLA7Z908SQKkZpyEcCvpkWsW3pa42eajpxCSkbUy4rv9g@mail.gmail.com/

This seems sort of similar to what Yicong is doing here:
https://lore.kernel.org/r/20220517124319.47125-1-yangyicong@hisilicon.com
but I don't know enough to put all the pieces together.

> On my tests at the boards, I didn't see the same PERST#
> signal to hit more than once, and the driver is working fine. 
> So, this wasn't an actual issue, as far as I can tell. 

Do you remember whether you tested with a switch below the PEX 8606?
E.g., something like this:

  00:00.0 Root Port                to [bus 01-ff]
  01:00.0 PEX 8606 Upstream Port   to [bus 02-ff]
  02:01.0 PEX 8606 Downstream Port to [bus 06-ff]
  06:00.0 Switch Upstream Port     to [bus 07-ff]
  07:01.0 Switch Downstream Port   to [bus 08]
  08:00.0 Endpoint

I think kirin_pcie_add_bus() will be called several times, probably
once for each bus (00, 01, 02, 06, 07, 08), and it looks like it will
do the same thing every time.  Do we reset the 8606 again when we're
about to scan bus 08?  Surely not, because otherwise we would have
reset the 8606 when scanning bus 06 in the original topology where the
8606 is the only switch.

I don't know enough about GPIOs to understand what happens here.  The
doc suggests that gpio_direction_output() sets the direction (output)
and the initial output value (1).  But I don't see any other reference
to gpio_id_reset[i] that looks like it deasserts PERST#.

Bjorn

[PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Mauro Carvalho Chehab]

Before code refactor, the PERST# signals were sent at the
end of the power_on logic. Then, the PCI core would probe for
the buses and add them.

The new logic changed it to send PERST# signals during
add_bus operation. That altered the timings.

Also, HiKey 970 require a little more waiting time for
the PCI bridge - which is outside the SoC - to finish
the PERST# reset, and then initialize the eye diagram.

So, increase the waiting time for the PERST# signals to
what's required for it to also work with HiKey 970.

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

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index de375795a3b8..bc329673632a 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -113,7 +113,7 @@ struct kirin_pcie {
 #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
 
 /* Time for delay */
-#define REF_2_PERST_MIN		20000
+#define REF_2_PERST_MIN		21000
 #define REF_2_PERST_MAX		25000
 #define PERST_2_ACCESS_MIN	10000
 #define PERST_2_ACCESS_MAX	12000
-- 
2.31.1

Re: [PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Lorenzo Pieralisi]

On Tue, Oct 19, 2021 at 07:06:42AM +0100, Mauro Carvalho Chehab wrote:
> Before code refactor, the PERST# signals were sent at the
> end of the power_on logic. Then, the PCI core would probe for
> the buses and add them.
> 
> The new logic changed it to send PERST# signals during
> add_bus operation. That altered the timings.
> 
> Also, HiKey 970 require a little more waiting time for
> the PCI bridge - which is outside the SoC - to finish
> the PERST# reset, and then initialize the eye diagram.
> 

Ok, now you explained it and we should move this explanation
in the commit log that this change is affecting (I mean we
should squash this patch with the patch that actually requires it
- I am not sure whether it is patch 6 or another one).

I can do it for you; I thought it would be a standalone change
but it actually isn't, because it is brought about by the
changes you are making and therefore there it belongs.

Thanks for explaining it and apologies for the churn.

Lorenzo

> So, increase the waiting time for the PERST# signals to
> what's required for it to also work with HiKey 970.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> 
> See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> 
>  drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index de375795a3b8..bc329673632a 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -113,7 +113,7 @@ struct kirin_pcie {
>  #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
>  
>  /* Time for delay */
> -#define REF_2_PERST_MIN		20000
> +#define REF_2_PERST_MIN		21000
>  #define REF_2_PERST_MAX		25000
>  #define PERST_2_ACCESS_MIN	10000
>  #define PERST_2_ACCESS_MAX	12000
> -- 
> 2.31.1
> 

Re: [PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Mauro Carvalho Chehab]

Em Thu, 21 Oct 2021 13:27:29 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> escreveu:

> On Tue, Oct 19, 2021 at 07:06:42AM +0100, Mauro Carvalho Chehab wrote:
> > Before code refactor, the PERST# signals were sent at the
> > end of the power_on logic. Then, the PCI core would probe for
> > the buses and add them.
> > 
> > The new logic changed it to send PERST# signals during
> > add_bus operation. That altered the timings.
> > 
> > Also, HiKey 970 require a little more waiting time for
> > the PCI bridge - which is outside the SoC - to finish
> > the PERST# reset, and then initialize the eye diagram.
> >   
> 
> Ok, now you explained it and we should move this explanation
> in the commit log that this change is affecting (I mean we
> should squash this patch with the patch that actually requires it
> - I am not sure whether it is patch 6 or another one).

IMO, having it on a separate patch has the advantage of better documenting
this single line change, but yeah, this is part of the change needed to
handle PERST# on a more portable way to work for both chipsets.

> I can do it for you; I thought it would be a standalone change
> but it actually isn't, because it is brought about by the
> changes you are making and therefore there it belongs.

Feel free to squash this patch if you prefer so. whatever works best for you.

> Thanks for explaining it and apologies for the churn.

No problem.
 
> 
> Lorenzo
> 
> > So, increase the waiting time for the PERST# signals to
> > what's required for it to also work with HiKey 970.
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > 
> > See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> > 
> >  drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > index de375795a3b8..bc329673632a 100644
> > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > @@ -113,7 +113,7 @@ struct kirin_pcie {
> >  #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
> >  
> >  /* Time for delay */
> > -#define REF_2_PERST_MIN		20000
> > +#define REF_2_PERST_MIN		21000
> >  #define REF_2_PERST_MAX		25000
> >  #define PERST_2_ACCESS_MIN	10000
> >  #define PERST_2_ACCESS_MAX	12000
> > -- 
> > 2.31.1
> >   

Re: [PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Pali Rohár]

On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:
> Before code refactor, the PERST# signals were sent at the
> end of the power_on logic. Then, the PCI core would probe for
> the buses and add them.
> 
> The new logic changed it to send PERST# signals during
> add_bus operation. That altered the timings.
> 
> Also, HiKey 970 require a little more waiting time for
> the PCI bridge - which is outside the SoC - to finish
> the PERST# reset, and then initialize the eye diagram.

Hello! Which PCIe port do you mean by PCI bridge device? Do you mean
PCIe Root Port? Or upstream port on some external PCIe switch connected
via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe
devices are presented as PCI bridge devices, so it is not clear to which
device it refers.

Normally PERST# signal is used to reset endpoint card, other end of PCIe
link and so PERST# signal should not affect PCIe Root Port at all.

> So, increase the waiting time for the PERST# signals to
> what's required for it to also work with HiKey 970.

Because PERST# signal resets endpoint card, this reset timeout should
not be driver or controller specific.

Mauro, if you understand this issue more deeply, could you look at my
email? https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/

I think that kernel PCI subsystem does not properly handle PCIe Warm
Reset and correct initialization of endpoint cards. Because similar
"random PERST# timeout patches" were applied to lot of native controller
drivers.

PS: I'm not opposing this patch, I'm just trying to understand what is
happening here and why particular number "21000" was chosen. It is
defined in some standard? Or was it just randomly chosen and measures
that with this number is initialization working fine?

> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
> 
> See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> 
>  drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> index de375795a3b8..bc329673632a 100644
> --- a/drivers/pci/controller/dwc/pcie-kirin.c
> +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> @@ -113,7 +113,7 @@ struct kirin_pcie {
>  #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
>  
>  /* Time for delay */
> -#define REF_2_PERST_MIN		20000
> +#define REF_2_PERST_MIN		21000
>  #define REF_2_PERST_MAX		25000
>  #define PERST_2_ACCESS_MIN	10000
>  #define PERST_2_ACCESS_MAX	12000
> -- 
> 2.31.1
> 

Re: [PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Mauro Carvalho Chehab]

Hi Pali,

Em Fri, 22 Oct 2021 17:16:24 +0200
Pali Rohár <pali@kernel.org> escreveu:

> On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:
> > Before code refactor, the PERST# signals were sent at the
> > end of the power_on logic. Then, the PCI core would probe for
> > the buses and add them.
> > 
> > The new logic changed it to send PERST# signals during
> > add_bus operation. That altered the timings.
> > 
> > Also, HiKey 970 require a little more waiting time for
> > the PCI bridge - which is outside the SoC - to finish
> > the PERST# reset, and then initialize the eye diagram.  
> 
> Hello! Which PCIe port do you mean by PCI bridge device? Do you mean
> PCIe Root Port? Or upstream port on some external PCIe switch connected
> via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe
> devices are presented as PCI bridge devices, so it is not clear to which
> device it refers.

HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]),
with 3 elements connected to the bus: an Ethernet card, a M.2 slot and
a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal,
as there are 4 independent PERST# signals there:

	- one for PEX 8606 (the PCIe root port);
	- one for Ethernet;
	- one for M.2;
	- one for mini-PCIe.

After sending the PCIe PERST# signals, the device has to wait for 21 ms
before adjusting the eye diagram.

[1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf

> Normally PERST# signal is used to reset endpoint card, other end of PCIe
> link and so PERST# signal should not affect PCIe Root Port at all.

That's not the case, as PEX 8606 needs to complete its reset sequence
for the rest of the devices to be visible. If the wait time is reduced
or removed, the devices behind it won't be detected.

> > So, increase the waiting time for the PERST# signals to
> > what's required for it to also work with HiKey 970.  
> 
> Because PERST# signal resets endpoint card, this reset timeout should
> not be driver or controller specific.

Not sure if it would be possible to implement it at the core without
breaking devices like this one where there's a separate chip to actually
implement the PCIe bus.
 
> Mauro, if you understand this issue more deeply, could you look at my
> email? https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> 
> I think that kernel PCI subsystem does not properly handle PCIe Warm
> Reset and correct initialization of endpoint cards. Because similar
> "random PERST# timeout patches" were applied to lot of native controller
> drivers.

I don't know enough about PCIe documentation in order to help with that.
Yet, if the PCI/PCIe specs doesn't define a maximum time for PERST# to
finish, hardware manufacturers will do whatever they please. So, finding
a common value is impossible. 

Well, even if specs define it, vendors may still violate that. So, whatever 
implementation is done, some quirks may be needed.

Sending PERST# signals to the devices connected to the bridge too early
will cause the bridge to not detect the devices behind it. That's what
happens with HiKey 970: lower reset values cause it to miss devices.

Looking from harware perspective, I'd say that the reset time pretty
much depends on how the PCIe bridges are implemented: if it is FPGA, it is 
probably slower than if it is a dedicated hardware. It can be even slower
if the bridge uses a microcontroller and needs to read the firmware from 
some place.

> PS: I'm not opposing this patch, I'm just trying to understand what is
> happening here and why particular number "21000" was chosen. It is
> defined in some standard? Or was it just randomly chosen and measures
> that with this number is initialization working fine?

It is the value used by the HiKey 970 PCIe out-of-tree driver. The patch
which added support for it at the pcie-kirin increased the time out there.

I tried to preserve the previous value, but that cause some devices to
be missed during PCI probe time.

Btw, PEX 8606 datasheet says:

	`Reset Circuit

	 The PEX 8606BA-AIC1U1D RDK accepts a PERST# from the host PC via card edge connector P1. This signal is
	 OR’d with a manual reset circuit. The manual reset circuit consists of a pushbutton (SW7, upper left corner) that
	 feeds into a reset timer. The reset timer monitors its power rail and reset input. If the reset input is low or the
	 supply rail is out of range, the reset output is held. Once both conditions no longer exist, the reset output will de-
	 assert after a programmable reset timeout period (capacitor adjustable, default value 128 msec). The OR’d reset
	 signal goes to the PEX 8606 device’s PEX_PERST# input pin, and the downstream slots’ PERST# connector
	 pins. PERST# to Slot J1 can be controlled by the PEX 8606 device’s Hot-Plug interface.'

If I understood it well, the PERST# time is hardware-configurable, by
changing the value of a capacitor.

Regards,
Mauro
> 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > ---
> > 
> > See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> > 
> >  drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > index de375795a3b8..bc329673632a 100644
> > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > @@ -113,7 +113,7 @@ struct kirin_pcie {
> >  #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
> >  
> >  /* Time for delay */
> > -#define REF_2_PERST_MIN		20000
> > +#define REF_2_PERST_MIN		21000
> >  #define REF_2_PERST_MAX		25000
> >  #define PERST_2_ACCESS_MIN	10000
> >  #define PERST_2_ACCESS_MAX	12000
> > -- 
> > 2.31.1
> >   

Re: [PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Pali Rohár]

Hello!

On Saturday 23 October 2021 10:30:59 Mauro Carvalho Chehab wrote:
> Hi Pali,
> 
> Em Fri, 22 Oct 2021 17:16:24 +0200
> Pali Rohár <pali@kernel.org> escreveu:
> 
> > On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:
> > > Before code refactor, the PERST# signals were sent at the
> > > end of the power_on logic. Then, the PCI core would probe for
> > > the buses and add them.
> > > 
> > > The new logic changed it to send PERST# signals during
> > > add_bus operation. That altered the timings.
> > > 
> > > Also, HiKey 970 require a little more waiting time for
> > > the PCI bridge - which is outside the SoC - to finish
> > > the PERST# reset, and then initialize the eye diagram.  
> > 
> > Hello! Which PCIe port do you mean by PCI bridge device? Do you mean
> > PCIe Root Port? Or upstream port on some external PCIe switch connected
> > via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe
> > devices are presented as PCI bridge devices, so it is not clear to which
> > device it refers.
> 
> HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]),

Ok! Now I understood. You have probably one PCIe Root Port on your board
and to this port you have connected (external) PCIe switch card from
Broadcom to increase number of PCIe ports for endpoint cards.
It is classic setup for boards with just one PCIe port.

> with 3 elements connected to the bus: an Ethernet card, a M.2 slot and
> a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal,
> as there are 4 independent PERST# signals there:
> 
> 	- one for PEX 8606 (the PCIe root port);
> 	- one for Ethernet;
> 	- one for M.2;
> 	- one for mini-PCIe.

This is not unique setup, its pretty normal. Every PCIe card has (own)
PERST# pin and obviously you want to control each pin separately via SW.
And because PCIe switch is also (upstream) PCIe device it has also
PERST# pin.

> After sending the PCIe PERST# signals, the device has to wait for 21 ms
> before adjusting the eye diagram.

"the device" which has to wait is HiKey970 or PEX8606?

> [1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf
> 
> > Normally PERST# signal is used to reset endpoint card, other end of PCIe
> > link and so PERST# signal should not affect PCIe Root Port at all.
> 
> That's not the case, as PEX 8606 needs to complete its reset sequence
> for the rest of the devices to be visible. If the wait time is reduced
> or removed, the devices behind it won't be detected.

Well, "endpoint card" for HiKey970 PCIe link is here PEX8606. And if you
connect PEX8606 to any other board (which could have totally different
PCIe controller), it means that same wait timeouts are required for that
other board.

So this wait timeout 21 ms is not HiKey970 specific, but rather PEX8606
specific, right?

> > > So, increase the waiting time for the PERST# signals to
> > > what's required for it to also work with HiKey 970.  
> > 
> > Because PERST# signal resets endpoint card, this reset timeout should
> > not be driver or controller specific.
> 
> Not sure if it would be possible to implement it at the core without
> breaking devices like this one where there's a separate chip to actually
> implement the PCIe bus.

I think it should be possible. Probably not so easy, would need more
testing, etc... But as I wrote above, this setup is not unique, it is
really normal and kernel is prepared to work PCI and PCIe topologies
when one or more PCIe switches, PCIe-to-PCI bridges or even more
PCI-to-PCI bridges are used and connected to system board.

I send email with proposal / idea how could be PCI subsystem extended to
handle initialization of native PCIe controller drivers:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
(if you have some more points, feel free to reply)

> > Mauro, if you understand this issue more deeply, could you look at my
> > email? https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> > 
> > I think that kernel PCI subsystem does not properly handle PCIe Warm
> > Reset and correct initialization of endpoint cards. Because similar
> > "random PERST# timeout patches" were applied to lot of native controller
> > drivers.
> 
> I don't know enough about PCIe documentation in order to help with that.
> Yet, if the PCI/PCIe specs doesn't define a maximum time for PERST# to
> finish, hardware manufacturers will do whatever they please. So, finding
> a common value is impossible. 

Well, it is possible that just I was not able to "find and decode" this
timeout from specifications. So I'm just asking if somebody else was
able to do it :-)

> Well, even if specs define it, vendors may still violate that. So, whatever 
> implementation is done, some quirks may be needed.

Of course, we know it and kernel has hooks and corrections for such
situation. Fixes are in most cases in one place: drivers/pci/quirks.c

> Sending PERST# signals to the devices connected to the bridge too early
> will cause the bridge to not detect the devices behind it. That's what
> happens with HiKey 970: lower reset values cause it to miss devices.

Just to make sure, that I understand your problem. Is your setup looks
like this?

          +-------------------------------------PERST#--+---+
          |                                             |eth|
          |                 +------PERST#--+  +--PCIe---+---+
          |                 |              |  |
  +-------------+ +-------------+        +-------+      +---+
  |GPIO-HiKey970| |PCIe-HiKey970|--PCIe--|PEX8606|-PCIe-|m.2|
  +-------------+ +-------------+        +-------+      +---+
          |           |          +-----+        |         |
          |           +--PERST#--|mPCIe|--PCIe--+         |
          |                      +-----+                  |
          +---------------------------------------PERST#--+

And if yes, in which order you need to assert individual PERST# signals
and in which order to de-assert them?

> Looking from harware perspective, I'd say that the reset time pretty
> much depends on how the PCIe bridges are implemented: if it is FPGA, it is 
> probably slower than if it is a dedicated hardware. It can be even slower
> if the bridge uses a microcontroller and needs to read the firmware from 
> some place.
> 
> > PS: I'm not opposing this patch, I'm just trying to understand what is
> > happening here and why particular number "21000" was chosen. It is
> > defined in some standard? Or was it just randomly chosen and measures
> > that with this number is initialization working fine?
> 
> It is the value used by the HiKey 970 PCIe out-of-tree driver. The patch
> which added support for it at the pcie-kirin increased the time out there.
> 
> I tried to preserve the previous value, but that cause some devices to
> be missed during PCI probe time.
> 
> Btw, PEX 8606 datasheet says:
> 
> 	`Reset Circuit
> 
> 	 The PEX 8606BA-AIC1U1D RDK accepts a PERST# from the host PC via card edge connector P1. This signal is
> 	 OR’d with a manual reset circuit. The manual reset circuit consists of a pushbutton (SW7, upper left corner) that
> 	 feeds into a reset timer. The reset timer monitors its power rail and reset input. If the reset input is low or the
> 	 supply rail is out of range, the reset output is held. Once both conditions no longer exist, the reset output will de-
> 	 assert after a programmable reset timeout period (capacitor adjustable, default value 128 msec). The OR’d reset
> 	 signal goes to the PEX 8606 device’s PEX_PERST# input pin, and the downstream slots’ PERST# connector
> 	 pins. PERST# to Slot J1 can be controlled by the PEX 8606 device’s Hot-Plug interface.'
> 
> If I understood it well, the PERST# time is hardware-configurable, by
> changing the value of a capacitor.

Hm... this is something different. It says: "Once both conditions no
longer exist, the reset output will de-assert after a programmable reset
timeout period (capacitor adjustable, default value 128 msec)."

I understand this part that if signal is no longer in reset that then
this capacitor cause that reset is held for another 128 ms. So if host
stops reset signal then it has to wait 128 ms prior doing something
(to ensure that reset finished), right?

> Regards,
> Mauro
> > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> > > ---
> > > 
> > > See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/
> > > 
> > >  drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
> > > index de375795a3b8..bc329673632a 100644
> > > --- a/drivers/pci/controller/dwc/pcie-kirin.c
> > > +++ b/drivers/pci/controller/dwc/pcie-kirin.c
> > > @@ -113,7 +113,7 @@ struct kirin_pcie {
> > >  #define CRGCTRL_PCIE_ASSERT_BIT		0x8c000000
> > >  
> > >  /* Time for delay */
> > > -#define REF_2_PERST_MIN		20000
> > > +#define REF_2_PERST_MIN		21000
> > >  #define REF_2_PERST_MAX		25000
> > >  #define PERST_2_ACCESS_MIN	10000
> > >  #define PERST_2_ACCESS_MAX	12000
> > > -- 
> > > 2.31.1
> > >   

Re: [PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Mauro Carvalho Chehab]

Em Sat, 23 Oct 2021 12:40:11 +0200
Pali Rohár <pali@kernel.org> escreveu:

> Hello!
> 
> On Saturday 23 October 2021 10:30:59 Mauro Carvalho Chehab wrote:
> > Hi Pali,
> > 
> > Em Fri, 22 Oct 2021 17:16:24 +0200
> > Pali Rohár <pali@kernel.org> escreveu:
> >   
> > > On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:  
> > > > Before code refactor, the PERST# signals were sent at the
> > > > end of the power_on logic. Then, the PCI core would probe for
> > > > the buses and add them.
> > > > 
> > > > The new logic changed it to send PERST# signals during
> > > > add_bus operation. That altered the timings.
> > > > 
> > > > Also, HiKey 970 require a little more waiting time for
> > > > the PCI bridge - which is outside the SoC - to finish
> > > > the PERST# reset, and then initialize the eye diagram.    
> > > 
> > > Hello! Which PCIe port do you mean by PCI bridge device? Do you mean
> > > PCIe Root Port? Or upstream port on some external PCIe switch connected
> > > via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe
> > > devices are presented as PCI bridge devices, so it is not clear to which
> > > device it refers.  
> > 
> > HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]),  
> 
> Ok! Now I understood. You have probably one PCIe Root Port on your board
> and to this port you have connected (external) PCIe switch card from
> Broadcom to increase number of PCIe ports for endpoint cards.

Yes.

> It is classic setup for boards with just one PCIe port.
> 
> > with 3 elements connected to the bus: an Ethernet card, a M.2 slot and
> > a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal,
> > as there are 4 independent PERST# signals there:
> > 
> > 	- one for PEX 8606 (the PCIe root port);
> > 	- one for Ethernet;
> > 	- one for M.2;
> > 	- one for mini-PCIe.  
> 
> This is not unique setup, its pretty normal. Every PCIe card has (own)
> PERST# pin and obviously you want to control each pin separately via SW.
> And because PCIe switch is also (upstream) PCIe device it has also
> PERST# pin.

Based on the discussions we had to add per-port DT PERST# gpios, it
sounded to me that this is was not a typical setup ;-)

It seems that the typical setup is to have a single PERST# connected
to all devices inside the bus.

> 
> > After sending the PCIe PERST# signals, the device has to wait for 21 ms
> > before adjusting the eye diagram.  
> 
> "the device" which has to wait is HiKey970 or PEX8606?

I guess both need, but not really sure. 

Kirin 970 (the SoC used on HiKey 970 board) needs to wait for PEX8606 to 
reset, in order to adjust the eye diagram.

The bus probing code needs to wait after sending the PERST# signals to 
the devices behind PEX8606, as otherwise they aren't detected.

> > [1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf
> >   
> > > Normally PERST# signal is used to reset endpoint card, other end of PCIe
> > > link and so PERST# signal should not affect PCIe Root Port at all.  
> > 
> > That's not the case, as PEX 8606 needs to complete its reset sequence
> > for the rest of the devices to be visible. If the wait time is reduced
> > or removed, the devices behind it won't be detected.  
> 
> Well, "endpoint card" for HiKey970 PCIe link is here PEX8606. And if you
> connect PEX8606 to any other board (which could have totally different
> PCIe controller), it means that same wait timeouts are required for that
> other board.
> 
> So this wait timeout 21 ms is not HiKey970 specific, but rather PEX8606
> specific, right?

I guess so.

> > > > So, increase the waiting time for the PERST# signals to
> > > > what's required for it to also work with HiKey 970.    
> > > 
> > > Because PERST# signal resets endpoint card, this reset timeout should
> > > not be driver or controller specific.  
> > 
> > Not sure if it would be possible to implement it at the core without
> > breaking devices like this one where there's a separate chip to actually
> > implement the PCIe bus.  
> 
> I think it should be possible. Probably not so easy, would need more
> testing, etc... But as I wrote above, this setup is not unique, it is
> really normal and kernel is prepared to work PCI and PCIe topologies
> when one or more PCIe switches, PCIe-to-PCI bridges or even more
> PCI-to-PCI bridges are used and connected to system board.

Yeah, technically, it is doable, but applying change like that requires
testing the code with all affected devices in order to avoid regressions.

> I send email with proposal / idea how could be PCI subsystem extended to
> handle initialization of native PCIe controller drivers:
> https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/
> (if you have some more points, feel free to reply)

Ok. Will try to reply to it later.

> > > Mauro, if you understand this issue more deeply, could you look at my
> > > email? https://lore.kernel.org/linux-pci/20210310110535.zh4pnn4vpmvzwl5q@pali/
> > > 
> > > I think that kernel PCI subsystem does not properly handle PCIe Warm
> > > Reset and correct initialization of endpoint cards. Because similar
> > > "random PERST# timeout patches" were applied to lot of native controller
> > > drivers.  
> > 
> > I don't know enough about PCIe documentation in order to help with that.
> > Yet, if the PCI/PCIe specs doesn't define a maximum time for PERST# to
> > finish, hardware manufacturers will do whatever they please. So, finding
> > a common value is impossible.   
> 
> Well, it is possible that just I was not able to "find and decode" this
> timeout from specifications. So I'm just asking if somebody else was
> able to do it :-)

My past experiences with other drivers/devices show that, even for things
that are clearly documented at the specs, hardware vendors end missing
some things. That's why there are lots of quicks at USB and PCI code
all over the drivers.

> > Well, even if specs define it, vendors may still violate that. So, whatever 
> > implementation is done, some quirks may be needed.  
> 
> Of course, we know it and kernel has hooks and corrections for such
> situation. Fixes are in most cases in one place: drivers/pci/quirks.c

There are PCI quirks outside PCI core. For instance, on media, we had to add
several quicks to avoid PCI2PCI data transfers with some broken hardware,
where enabing it would cause disk data corruption (see bt8xx, saa7134 and
cx88 drivers, for instance).

> > Sending PERST# signals to the devices connected to the bridge too early
> > will cause the bridge to not detect the devices behind it. That's what
> > happens with HiKey 970: lower reset values cause it to miss devices.  
> 
> Just to make sure, that I understand your problem. Is your setup looks
> like this?
> 
>           +-------------------------------------PERST#--+---+
>           |                                             |eth|
>           |                 +------PERST#--+  +--PCIe---+---+
>           |                 |              |  |
>   +-------------+ +-------------+        +-------+      +---+
>   |GPIO-HiKey970| |PCIe-HiKey970|--PCIe--|PEX8606|-PCIe-|m.2|
>   +-------------+ +-------------+        +-------+      +---+
>           |           |          +-----+        |         |
>           |           +--PERST#--|mPCIe|--PCIe--+         |
>           |                      +-----+                  |
>           +---------------------------------------PERST#--+

It is:


           +-------------------------------------PERST#--+---+
           |                                             |eth|
           |   +--------------------PERST#--+  +--PCIe---+---+
           |   |                            |  |
   +-------------+ +------------+        +-------+      +---+
   |Kirin970 GPIO| |Kirin970    |--PCIe--|PEX8606|-PCIe-|m.2|
   |             | |DWC and PHY |        +-------+      +---+
   +-------------+ +------------+                |         |
           |  |                   +-----+        |         |
           |  +---PERST#----------|mPCIe|--PCIe--+         |
           |                      +-----+                  |
           +--------------------------------------PERST#---+

You can see more details by looking at the schematics of the board. 
It is available at:

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

> And if yes, in which order you need to assert individual PERST# signals
> and in which order to de-assert them?

As requested by Rob, the current code triggers a PERST# signal to 
PEX8606 at the end of the power-on sequence:

        if (!gpio_request(kirin_pcie->gpio_id_dwc_perst, "pcie_perst_bridge")) {
                ret = gpio_direction_output(kirin_pcie->gpio_id_dwc_perst, 1);
                if (ret)
                        goto err;
        }

        usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);

Then, it sends a per-slot reset during add_bus ops:

	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;
	
	        if (!kirin_pcie->num_slots)
	                return 0;
	
	        /* 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) {
	                        dev_err(pci->dev, "PERST# %s error: %d\n",
	                                kirin_pcie->reset_names[i], ret);
	                }
	        }
	        usleep_range(PERST_2_ACCESS_MIN, PERST_2_ACCESS_MAX);
	
	        return 0;
	}

> > Looking from harware perspective, I'd say that the reset time pretty
> > much depends on how the PCIe bridges are implemented: if it is FPGA, it is 
> > probably slower than if it is a dedicated hardware. It can be even slower
> > if the bridge uses a microcontroller and needs to read the firmware from 
> > some place.
> >   
> > > PS: I'm not opposing this patch, I'm just trying to understand what is
> > > happening here and why particular number "21000" was chosen. It is
> > > defined in some standard? Or was it just randomly chosen and measures
> > > that with this number is initialization working fine?  
> > 
> > It is the value used by the HiKey 970 PCIe out-of-tree driver. The patch
> > which added support for it at the pcie-kirin increased the time out there.
> > 
> > I tried to preserve the previous value, but that cause some devices to
> > be missed during PCI probe time.
> > 
> > Btw, PEX 8606 datasheet says:
> > 
> > 	`Reset Circuit
> > 
> > 	 The PEX 8606BA-AIC1U1D RDK accepts a PERST# from the host PC via card edge connector P1. This signal is
> > 	 OR’d with a manual reset circuit. The manual reset circuit consists of a pushbutton (SW7, upper left corner) that
> > 	 feeds into a reset timer. The reset timer monitors its power rail and reset input. If the reset input is low or the
> > 	 supply rail is out of range, the reset output is held. Once both conditions no longer exist, the reset output will de-
> > 	 assert after a programmable reset timeout period (capacitor adjustable, default value 128 msec). The OR’d reset
> > 	 signal goes to the PEX 8606 device’s PEX_PERST# input pin, and the downstream slots’ PERST# connector
> > 	 pins. PERST# to Slot J1 can be controlled by the PEX 8606 device’s Hot-Plug interface.'
> > 
> > If I understood it well, the PERST# time is hardware-configurable, by
> > changing the value of a capacitor.  
> 
> Hm... this is something different. It says: "Once both conditions no
> longer exist, the reset output will de-assert after a programmable reset
> timeout period (capacitor adjustable, default value 128 msec)."
> 
> I understand this part that if signal is no longer in reset that then
> this capacitor cause that reset is held for another 128 ms. So if host
> stops reset signal then it has to wait 128 ms prior doing something
> (to ensure that reset finished), right?

It seems that a 0 -> 1 transition at the GPIO triggers PEX8606 code to
send the PERST# signal to the time configured via the capacitor (128 ms
by default). There's no need to change the GPIO back to 0.

Regards,
Mauro

Re: [PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Pali Rohár]

On Saturday 23 October 2021 14:45:34 Mauro Carvalho Chehab wrote:
> Em Sat, 23 Oct 2021 12:40:11 +0200
> Pali Rohár <pali@kernel.org> escreveu:
> > It is classic setup for boards with just one PCIe port.
> > 
> > > with 3 elements connected to the bus: an Ethernet card, a M.2 slot and
> > > a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal,
> > > as there are 4 independent PERST# signals there:
> > > 
> > > 	- one for PEX 8606 (the PCIe root port);
> > > 	- one for Ethernet;
> > > 	- one for M.2;
> > > 	- one for mini-PCIe.  
> > 
> > This is not unique setup, its pretty normal. Every PCIe card has (own)
> > PERST# pin and obviously you want to control each pin separately via SW.
> > And because PCIe switch is also (upstream) PCIe device it has also
> > PERST# pin.
> 
> Based on the discussions we had to add per-port DT PERST# gpios, it
> sounded to me that this is was not a typical setup ;-)
> 
> It seems that the typical setup is to have a single PERST# connected
> to all devices inside the bus.

Hello!

I'm sure it is not unique :) Just seems that these boards either do not
use device tree (x86-based) or do not have specified reset-gpios in DTS
at all.

Sometimes there is no need to touch PERST# gpio as either firmware
during boot handles it (x86 BIOS/UEFI case, or U-Boot for arm case) or
because board/cpu reset toggle PERST# in a way that is compatible for
cards init.

Looks like you have non-x86 board, which does not have PCIe init code in
firmware, needs special handling of PERST# and you are doing it with
upstream kernel :-) So maybe all these conditions are unique... But HW
design not.

As this setup with reset-gpios per card in DTS nodes is something which
I will need too, I sent email to Rob with proposal how to universally
declare it in DTS, independently of PCIe controller (you are on CC):
https://lore.kernel.org/linux-pci/20211023144252.z7ou2l2tvm6cvtf7@pali/

Due to how PCIe cards are broken, PERST# signal is sometimes the only
one option how to reset card at runtime. So requirement for separate
PERST# per card configurable at runtime by OS will be requirement for
more and more boards.

Re: [PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Lorenzo Pieralisi]

On Sat, Oct 23, 2021 at 10:30:59AM +0100, Mauro Carvalho Chehab wrote:
> Hi Pali,
> 
> Em Fri, 22 Oct 2021 17:16:24 +0200
> Pali Rohár <pali@kernel.org> escreveu:
> 
> > On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:
> > > Before code refactor, the PERST# signals were sent at the
> > > end of the power_on logic. Then, the PCI core would probe for
> > > the buses and add them.
> > > 
> > > The new logic changed it to send PERST# signals during
> > > add_bus operation. That altered the timings.
> > > 
> > > Also, HiKey 970 require a little more waiting time for
> > > the PCI bridge - which is outside the SoC - to finish
> > > the PERST# reset, and then initialize the eye diagram.  
> > 
> > Hello! Which PCIe port do you mean by PCI bridge device? Do you mean
> > PCIe Root Port? Or upstream port on some external PCIe switch connected
> > via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe
> > devices are presented as PCI bridge devices, so it is not clear to which
> > device it refers.
> 
> HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]),
> with 3 elements connected to the bus: an Ethernet card, a M.2 slot and
> a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal,
> as there are 4 independent PERST# signals there:
> 
> 	- one for PEX 8606 (the PCIe root port);
> 	- one for Ethernet;
> 	- one for M.2;
> 	- one for mini-PCIe.
> 
> After sending the PCIe PERST# signals, the device has to wait for 21 ms
> before adjusting the eye diagram.
> 
> [1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf
> 
> > Normally PERST# signal is used to reset endpoint card, other end of PCIe
> > link and so PERST# signal should not affect PCIe Root Port at all.
> 
> That's not the case, as PEX 8606 needs to complete its reset sequence
> for the rest of the devices to be visible. If the wait time is reduced
> or removed, the devices behind it won't be detected.

These pieces of information should go into the commit log (or I can add
a Link: tag to this discussion) - it is fundamental to understand these
changes.

I believe we can merge this series but we have to document this
discussion appropriately.

Lorenzo

Re: [PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Mauro Carvalho Chehab]

Em Mon, 25 Oct 2021 11:25:11 +0100
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> escreveu:

> On Sat, Oct 23, 2021 at 10:30:59AM +0100, Mauro Carvalho Chehab wrote:
> > Hi Pali,
> > 
> > Em Fri, 22 Oct 2021 17:16:24 +0200
> > Pali Rohár <pali@kernel.org> escreveu:
> >   
> > > On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:  
> > > > Before code refactor, the PERST# signals were sent at the
> > > > end of the power_on logic. Then, the PCI core would probe for
> > > > the buses and add them.
> > > > 
> > > > The new logic changed it to send PERST# signals during
> > > > add_bus operation. That altered the timings.
> > > > 
> > > > Also, HiKey 970 require a little more waiting time for
> > > > the PCI bridge - which is outside the SoC - to finish
> > > > the PERST# reset, and then initialize the eye diagram.    
> > > 
> > > Hello! Which PCIe port do you mean by PCI bridge device? Do you mean
> > > PCIe Root Port? Or upstream port on some external PCIe switch connected
> > > via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe
> > > devices are presented as PCI bridge devices, so it is not clear to which
> > > device it refers.  
> > 
> > HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]),
> > with 3 elements connected to the bus: an Ethernet card, a M.2 slot and
> > a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal,
> > as there are 4 independent PERST# signals there:
> > 
> > 	- one for PEX 8606 (the PCIe root port);
> > 	- one for Ethernet;
> > 	- one for M.2;
> > 	- one for mini-PCIe.
> > 
> > After sending the PCIe PERST# signals, the device has to wait for 21 ms
> > before adjusting the eye diagram.
> > 
> > [1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf
> >   
> > > Normally PERST# signal is used to reset endpoint card, other end of PCIe
> > > link and so PERST# signal should not affect PCIe Root Port at all.  
> > 
> > That's not the case, as PEX 8606 needs to complete its reset sequence
> > for the rest of the devices to be visible. If the wait time is reduced
> > or removed, the devices behind it won't be detected.  
> 
> These pieces of information should go into the commit log (or I can add
> a Link: tag to this discussion) - it is fundamental to understand these
> changes.
> 
> I believe we can merge this series but we have to document this
> discussion appropriately.

IMO, the best is to add a Link: to the discussion:

Link: https://lore.kernel.org/all/9a365cffe5af9ec5a1f79638968c3a2efa979b65.1634622716.git.mchehab+huawei@kernel.org/

But if you prefer otherwise and want me to re-submit the series, please
let me know.

Regards,
Mauro

Re: [PATCH v14 05/11] PCI: kirin: give more time for PERST# reset to finish

[Lorenzo Pieralisi]

On Mon, Oct 25, 2021 at 11:40:11AM +0100, Mauro Carvalho Chehab wrote:
> Em Mon, 25 Oct 2021 11:25:11 +0100
> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> escreveu:
> 
> > On Sat, Oct 23, 2021 at 10:30:59AM +0100, Mauro Carvalho Chehab wrote:
> > > Hi Pali,
> > > 
> > > Em Fri, 22 Oct 2021 17:16:24 +0200
> > > Pali Rohár <pali@kernel.org> escreveu:
> > >   
> > > > On Tuesday 19 October 2021 07:06:42 Mauro Carvalho Chehab wrote:  
> > > > > Before code refactor, the PERST# signals were sent at the
> > > > > end of the power_on logic. Then, the PCI core would probe for
> > > > > the buses and add them.
> > > > > 
> > > > > The new logic changed it to send PERST# signals during
> > > > > add_bus operation. That altered the timings.
> > > > > 
> > > > > Also, HiKey 970 require a little more waiting time for
> > > > > the PCI bridge - which is outside the SoC - to finish
> > > > > the PERST# reset, and then initialize the eye diagram.    
> > > > 
> > > > Hello! Which PCIe port do you mean by PCI bridge device? Do you mean
> > > > PCIe Root Port? Or upstream port on some external PCIe switch connected
> > > > via PCIe bus to the PCIe Root Port? Because all of these (virtual) PCIe
> > > > devices are presented as PCI bridge devices, so it is not clear to which
> > > > device it refers.  
> > > 
> > > HiKey 970 uses an external PCI bridge chipset (a Broadcom PEX 8606[1]),
> > > with 3 elements connected to the bus: an Ethernet card, a M.2 slot and
> > > a mini PCIe slot. It seems HiKey 970 is unique with regards to PERST# signal,
> > > as there are 4 independent PERST# signals there:
> > > 
> > > 	- one for PEX 8606 (the PCIe root port);
> > > 	- one for Ethernet;
> > > 	- one for M.2;
> > > 	- one for mini-PCIe.
> > > 
> > > After sending the PCIe PERST# signals, the device has to wait for 21 ms
> > > before adjusting the eye diagram.
> > > 
> > > [1] https://docs.broadcom.com/docs/PEX_8606_AIC_RDK_HRM_v1.3_06Aug10.pdf
> > >   
> > > > Normally PERST# signal is used to reset endpoint card, other end of PCIe
> > > > link and so PERST# signal should not affect PCIe Root Port at all.  
> > > 
> > > That's not the case, as PEX 8606 needs to complete its reset sequence
> > > for the rest of the devices to be visible. If the wait time is reduced
> > > or removed, the devices behind it won't be detected.  
> > 
> > These pieces of information should go into the commit log (or I can add
> > a Link: tag to this discussion) - it is fundamental to understand these
> > changes.
> > 
> > I believe we can merge this series but we have to document this
> > discussion appropriately.
> 
> IMO, the best is to add a Link: to the discussion:
> 
> Link: https://lore.kernel.org/all/9a365cffe5af9ec5a1f79638968c3a2efa979b65.1634622716.git.mchehab+huawei@kernel.org/
> 
> But if you prefer otherwise and want me to re-submit the series, please
> let me know.

I will squash this patch with the previous one (that describes
the bridge PERST# requirements) and add the Link above to the
commit log.

Lorenzo

[PATCH v14 06/11] PCI: kirin: Add Kirin 970 compatible

[Mauro Carvalho Chehab]

Now that everything is in place, add a compatible for Kirin 970.

Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index bc329673632a..475ef57d5261 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -738,6 +738,10 @@ static const struct of_device_id kirin_pcie_match[] = {
 		.compatible = "hisilicon,kirin960-pcie",
 		.data = (void *)PCIE_KIRIN_INTERNAL_PHY
 	},
+	{
+		.compatible = "hisilicon,kirin970-pcie",
+		.data = (void *)PCIE_KIRIN_EXTERNAL_PHY
+	},
 	{},
 };
 
-- 
2.31.1

[PATCH v14 07/11] PCI: kirin: Add MODULE_* macros

[Mauro Carvalho Chehab]

This driver misses the MODULE_* macros. Add them.

Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 475ef57d5261..b55830d2a19b 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -804,3 +804,8 @@ static struct platform_driver kirin_pcie_driver = {
 	},
 };
 builtin_platform_driver(kirin_pcie_driver);
+
+MODULE_DEVICE_TABLE(of, kirin_pcie_match);
+MODULE_DESCRIPTION("PCIe host controller driver for Kirin Phone SoCs");
+MODULE_AUTHOR("Xiaowei Song <songxiaowei@huawei.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.31.1

[PATCH v14 08/11] PCI: kirin: Allow building it as a module

[Mauro Carvalho Chehab]

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

Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index 071a1fb12beb..62ce3abf0f19 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -274,7 +274,7 @@ config PCIE_KEEMBAY_EP
 
 config PCIE_KIRIN
 	depends on OF && (ARM64 || COMPILE_TEST)
-	bool "HiSilicon Kirin series SoCs PCIe controllers"
+	tristate "HiSilicon Kirin series SoCs PCIe controllers"
 	depends on PCI_MSI_IRQ_DOMAIN
 	select PCIE_DW_HOST
 	help
-- 
2.31.1

[PATCH v14 09/11] PCI: kirin: Add power_off support for Kirin 960 PHY

[Mauro Carvalho Chehab]

In order to prepare for module unload, add a power_off method
for HiKey 960.

Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index b55830d2a19b..64221a204db2 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -343,6 +343,18 @@ static int hi3660_pcie_phy_init(struct platform_device *pdev,
 	return hi3660_pcie_phy_get_resource(phy);
 }
 
+static int hi3660_pcie_phy_power_off(struct kirin_pcie *pcie)
+{
+	struct hi3660_pcie_phy *phy = pcie->phy_priv;
+
+	/* Drop power supply for Host */
+	regmap_write(phy->sysctrl, SCTRL_PCIE_CMOS_OFFSET, 0x00);
+
+	hi3660_pcie_phy_clk_ctrl(phy, false);
+
+	return 0;
+}
+
 /*
  * The non-PHY part starts here
  */
@@ -560,7 +572,6 @@ static int kirin_pcie_add_bus(struct pci_bus *bus)
 	return 0;
 }
 
-
 static struct pci_ops kirin_pci_ops = {
 	.read = kirin_pcie_rd_own_conf,
 	.write = kirin_pcie_wr_own_conf,
@@ -714,8 +725,12 @@ static int kirin_pcie_power_on(struct platform_device *pdev,
 
 	return 0;
 err:
-	if (kirin_pcie->type != PCIE_KIRIN_INTERNAL_PHY)
+	if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY) {
+		hi3660_pcie_phy_power_off(kirin_pcie);
+	} else {
+		phy_power_off(kirin_pcie->phy);
 		phy_exit(kirin_pcie->phy);
+	}
 
 	return ret;
 }
@@ -725,7 +740,7 @@ static int __exit kirin_pcie_remove(struct platform_device *pdev)
 	struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
 
 	if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
-		return 0;
+		return hi3660_pcie_phy_power_off(kirin_pcie);
 
 	phy_power_off(kirin_pcie->phy);
 	phy_exit(kirin_pcie->phy);
-- 
2.31.1

[PATCH v14 10/11] PCI: kirin: fix poweroff sequence

[Mauro Carvalho Chehab]

This driver currently doesn't call dw_pcie_host_deinit()
at the .remove() callback. This can cause an OOPS if the driver
is unbound.

While here, add a poweroff function, in order to abstract
between the internal and external PHY logic.

Fixes: fc5165db245a ("PCI: kirin: Add HiSilicon Kirin SoC PCIe controller driver")
Cc: stable@vger.kernel.org
Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 29 ++++++++++++++++---------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index 64221a204db2..fea4d717fff3 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -680,6 +680,22 @@ static const struct dw_pcie_host_ops kirin_pcie_host_ops = {
 	.host_init = kirin_pcie_host_init,
 };
 
+static int kirin_pcie_power_off(struct kirin_pcie *kirin_pcie)
+{
+	int i;
+
+	if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
+		return hi3660_pcie_phy_power_off(kirin_pcie);
+
+	for (i = 0; i < kirin_pcie->n_gpio_clkreq; i++)
+		gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 1);
+
+	phy_power_off(kirin_pcie->phy);
+	phy_exit(kirin_pcie->phy);
+
+	return 0;
+}
+
 static int kirin_pcie_power_on(struct platform_device *pdev,
 			       struct kirin_pcie *kirin_pcie)
 {
@@ -725,12 +741,7 @@ static int kirin_pcie_power_on(struct platform_device *pdev,
 
 	return 0;
 err:
-	if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY) {
-		hi3660_pcie_phy_power_off(kirin_pcie);
-	} else {
-		phy_power_off(kirin_pcie->phy);
-		phy_exit(kirin_pcie->phy);
-	}
+	kirin_pcie_power_off(kirin_pcie);
 
 	return ret;
 }
@@ -739,11 +750,9 @@ static int __exit kirin_pcie_remove(struct platform_device *pdev)
 {
 	struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
 
-	if (kirin_pcie->type == PCIE_KIRIN_INTERNAL_PHY)
-		return hi3660_pcie_phy_power_off(kirin_pcie);
+	dw_pcie_host_deinit(&kirin_pcie->pci->pp);
 
-	phy_power_off(kirin_pcie->phy);
-	phy_exit(kirin_pcie->phy);
+	kirin_pcie_power_off(kirin_pcie);
 
 	return 0;
 }
-- 
2.31.1

[PATCH v14 11/11] PCI: kirin: Allow removing the driver

[Mauro Carvalho Chehab]

Now that everything is in place at the poweroff sequence,
this driver can use module_platform_driver(), which allows
it to be removed.

Acked-by: Xiaowei Song <songxiaowei@hisilicon.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---

See [PATCH v14 00/11] at: https://lore.kernel.org/all/cover.1634622716.git.mchehab+huawei@kernel.org/

 drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-kirin.c b/drivers/pci/controller/dwc/pcie-kirin.c
index fea4d717fff3..cdf568ea0f68 100644
--- a/drivers/pci/controller/dwc/pcie-kirin.c
+++ b/drivers/pci/controller/dwc/pcie-kirin.c
@@ -827,7 +827,7 @@ static struct platform_driver kirin_pcie_driver = {
 		.suppress_bind_attrs	= true,
 	},
 };
-builtin_platform_driver(kirin_pcie_driver);
+module_platform_driver(kirin_pcie_driver);
 
 MODULE_DEVICE_TABLE(of, kirin_pcie_match);
 MODULE_DESCRIPTION("PCIe host controller driver for Kirin Phone SoCs");
-- 
2.31.1

Re: [PATCH v14 00/11] Add support for Hikey 970 PCIe

[Bjorn Helgaas]

On Tue, Oct 19, 2021 at 07:06:37AM +0100, Mauro Carvalho Chehab wrote:

> Mauro Carvalho Chehab (11):
>   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 support for bridge slot DT schema
>   PCI: kirin: give more time for PERST# reset to finish
>   PCI: kirin: Add Kirin 970 compatible
>   PCI: kirin: Add MODULE_* macros
>   PCI: kirin: Allow building it as a module
>   PCI: kirin: Add power_off support for Kirin 960 PHY
>   PCI: kirin: fix poweroff sequence
>   PCI: kirin: Allow removing the driver

Don't repost for this, but if you have occasion to repost for other
reasons, two of these are not capitalized like the others:

>   PCI: kirin: give more time for PERST# reset to finish
>   PCI: kirin: fix poweroff sequence

These are write-once for you, but I'll be reading them many times in
the future and they're minor distractions.

Re: [PATCH v14 00/11] Add support for Hikey 970 PCIe

[Mauro Carvalho Chehab]

Em Tue, 19 Oct 2021 14:27:58 -0500
Bjorn Helgaas <helgaas@kernel.org> escreveu:

> On Tue, Oct 19, 2021 at 07:06:37AM +0100, Mauro Carvalho Chehab wrote:
> 
> > Mauro Carvalho Chehab (11):
> >   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 support for bridge slot DT schema
> >   PCI: kirin: give more time for PERST# reset to finish
> >   PCI: kirin: Add Kirin 970 compatible
> >   PCI: kirin: Add MODULE_* macros
> >   PCI: kirin: Allow building it as a module
> >   PCI: kirin: Add power_off support for Kirin 960 PHY
> >   PCI: kirin: fix poweroff sequence
> >   PCI: kirin: Allow removing the driver  
> 
> Don't repost for this, but if you have occasion to repost for other
> reasons, two of these are not capitalized like the others:
> 
> >   PCI: kirin: give more time for PERST# reset to finish
> >   PCI: kirin: fix poweroff sequence  
> 
> These are write-once for you, but I'll be reading them many times in
> the future and they're minor distractions.

Ok, changed on my working repository. On media and on other subsystems
I contribute regularly, the practice is just the opposite: to use
lowercase after colons.

Regards,
Mauro

Re: [PATCH v14 00/11] Add support for Hikey 970 PCIe

[Bjorn Helgaas]

On Wed, Oct 20, 2021 at 06:41:42AM +0100, Mauro Carvalho Chehab wrote:
> Em Tue, 19 Oct 2021 14:27:58 -0500
> Bjorn Helgaas <helgaas@kernel.org> escreveu:
> 
> > On Tue, Oct 19, 2021 at 07:06:37AM +0100, Mauro Carvalho Chehab wrote:
> > 
> > > Mauro Carvalho Chehab (11):
> > >   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 support for bridge slot DT schema
> > >   PCI: kirin: give more time for PERST# reset to finish
> > >   PCI: kirin: Add Kirin 970 compatible
> > >   PCI: kirin: Add MODULE_* macros
> > >   PCI: kirin: Allow building it as a module
> > >   PCI: kirin: Add power_off support for Kirin 960 PHY
> > >   PCI: kirin: fix poweroff sequence
> > >   PCI: kirin: Allow removing the driver  
> > 
> > Don't repost for this, but if you have occasion to repost for other
> > reasons, two of these are not capitalized like the others:
> > 
> > >   PCI: kirin: give more time for PERST# reset to finish
> > >   PCI: kirin: fix poweroff sequence  
> > 
> > These are write-once for you, but I'll be reading them many times in
> > the future and they're minor distractions.
> 
> Ok, changed on my working repository. On media and on other subsystems
> I contribute regularly, the practice is just the opposite: to use
> lowercase after colons.

Thanks.  I wouldn't mind a kernel-wide convention, but there isn't
one.  I just try to pay attention to whatever convention there is for
a given file or subsystem.

Bjorn