Add support for Hikey 970 PCIe

PATCH Applied Mauro Carvalho Chehab 6 participants 27 messages Oct 21, 2021 – Nov 4, 2021

Subsystems (3)

PCI PCI native host bridge and endpoint drivers PCIe driver for HiSilicon kirin

lore

Share & tools

lore

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #0 lore raw

v15 cover Add support for Hikey 970 PCIe

Hi Lorenzo,

I split patch 09/10 from v13 into three patches, in order to have one logical change per patch, adding a proper descriptio to each of them. The final code didn change.

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)

---

v15:
- The power-off fix patch was split into 3, in order to have one logical change per patch.
- Removed Fixes: tag from the poweroff patch;
- Adjusted capitalization of two patch summary lines
- No functional changes. The diff of this series is identical to v14.

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 (13):
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: Move the power-off code to a common routine
PCI: kirin: Disable clkreq during poweroff sequence
PCI: kirin: De-init the dwc driver
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(-)

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #1 lore raw

PATCH v15 1/13 PCI: kirin: Reorganize the PHY logic inside the driver

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 <[email protected]>

Acked-by Xiaowei Song <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

drivers/pci/controller/dwc/pcie-kirin.c +173 ~~-125~~

index 026fd1e42a55..b4063a3434df 100644
@@ -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);

Signature · 1 line

2.31.1

Bjorn Helgaas

2021-11-04 21:48 UTC #2 lore raw

Re: [PATCH v15 01/13] PCI: kirin: Reorganize the PHY logic inside the driver

Quoted 21 lines · 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 <[email protected]>
Acked-by Xiaowei Song <[email protected]>
Signed-off-by Mauro Carvalho Chehab <[email protected]>
drivers/pci/controller/dwc/pcie-kirin.c +6 -0
+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;

drivers/pci/controller/dwc/pcie-kirin.c static int kirin_pcie_phy_init(struct kirin_pcie *kirin_pcie) · line 321

Quoted 10 lines

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

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

Is there something missing here? "pdev" isn't referenced below. Noticed by Krzysztof.

Quoted 6 lines

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

+	ret = hi3660_pcie_phy_get_clk(phy);
+	if (ret)
+		return ret;
+
+	return hi3660_pcie_phy_get_resource(phy);
+}

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #3 lore raw

PATCH v15 2/13 PCI: kirin: Add support for a PHY layer

In reply to 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 <[email protected]>

Acked-by Xiaowei Song <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

drivers/pci/controller/dwc/pcie-kirin.c +80 ~~-15~~

index b4063a3434df..91a7c096bf8f 100644
@@ -8,16 +8,18 @@
  * Author: Xiaowei Song <[email protected]>
  */
 
-#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,

Signature · 1 line

2.31.1

Kishon Vijay Abraham I

2021-10-25 08:15 UTC #4 lore raw

Re: [PATCH v15 02/13] PCI: kirin: Add support for a PHY layer

Hi Mauro,

Quoted 111 lines · 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 <[email protected]>

Acked-by Xiaowei Song <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

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

index b4063a3434df..91a7c096bf8f 100644
@@ -8,16 +8,18 @@
  * Author: Xiaowei Song <[email protected]>
  */
 
-#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);

drivers/pci/controller/dwc/pcie-kirin.c static const struct dw_pcie_host_ops kirin_pcie_host_ops = { · line 537

Quoted 10 lines

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

+	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
+	},

Where is PCIE_KIRIN_EXTERNAL_PHY used?

Thanks,
Kishon

Quoted 64 lines

drivers/pci/controller/dwc/pcie-kirin.c +19 ~~-13~~

+	{},
+};
+
 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,

Mauro Carvalho Chehab

author

2021-10-25 08:53 UTC #5 lore raw

Re: [PATCH v15 02/13] PCI: kirin: Add support for a PHY layer

Hi Kishon,

Em Mon, 25 Oct 2021 13:44:57 +0530
Kishon Vijay Abraham I <[email protected]> escreveu:

Quoted 10 lines

Hi Mauro,
drivers/pci/controller/dwc/pcie-kirin.c static const struct dw_pcie_host_ops kirin_pcie_host_ops = { · line 537
Quoted 6 lines
drivers/pci/controller/dwc/pcie-kirin.c +6 -0
+
+static const struct of_device_id kirin_pcie_match[] = {
+	{
+		.compatible = "hisilicon,kirin960-pcie",
+		.data = (void *)PCIE_KIRIN_INTERNAL_PHY
+	},  
Where is PCIE_KIRIN_EXTERNAL_PHY used?

See:

	[PATCH v15 06/13] PCI: kirin: Add Kirin 970 compatible

	https://lore.kernel.org/all/ac8c730c0300b90d96bdaaf387d458d8949241a9.1634812676.git.mchehab+huawei@kernel.org/

Basically, Kirin 970 (and any other devices that would use the same driver) should also use PCIE_KIRIN_EXTERNAL_PHY and place the PHY driver inside drivers/phy, instead of hardcoding it at the driver.

The Kirin 970 PHY driver was already merged at linux-next:

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/phy/hisilicon/phy-hi3670-pcie.c

Regards,
Mauro

Kishon Vijay Abraham I

Reviewed-by

2021-10-25 08:57 UTC #6 lore raw

Re: [PATCH v15 02/13] PCI: kirin: Add support for a PHY layer

Hi Mauro,

Quoted 21 lines · Mauro Carvalho Chehab

Hi Kishon,
Em Mon, 25 Oct 2021 13:44:57 +0530
Kishon Vijay Abraham I <[email protected]> escreveu:
Quoted 10 lines
Hi Mauro,
drivers/pci/controller/dwc/pcie-kirin.c static const struct dw_pcie_host_ops kirin_pcie_host_ops = { · line 537
Quoted 6 lines
drivers/pci/controller/dwc/pcie-kirin.c +6 -0
+
+static const struct of_device_id kirin_pcie_match[] = {
+	{
+		.compatible = "hisilicon,kirin960-pcie",
+		.data = (void *)PCIE_KIRIN_INTERNAL_PHY
+	},  
Where is PCIE_KIRIN_EXTERNAL_PHY used?
See:
	[PATCH v15 06/13] PCI: kirin: Add Kirin 970 compatible

	https://lore.kernel.org/all/ac8c730c0300b90d96bdaaf387d458d8949241a9.1634812676.git.mchehab+huawei@kernel.org/

Quoted 7 lines

Basically, Kirin 970 (and any other devices that would use the same driver) should also use PCIE_KIRIN_EXTERNAL_PHY and place the PHY driver inside drivers/phy, instead of hardcoding it at the driver.
The Kirin 970 PHY driver was already merged at linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/phy/hisilicon/phy-hi3670-pcie.c

Thanks for clarifying.

Reviewed-by Kishon Vijay Abraham I <[email protected]>

Regards,
Kishon

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #7 lore raw

PATCH v15 3/13 PCI: kirin: Use regmap for APB registers

In reply to 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 <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

drivers/pci/controller/dwc/pcie-kirin.c +26 ~~-23~~

index 91a7c096bf8f..86c13661e02d 100644
@@ -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;
 }

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #8 lore raw

PATCH v15 4/13 PCI: kirin: Add support for bridge slot DT schema

In reply to 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 <[email protected]>

Acked-by Xiaowei Song <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

drivers/pci/controller/dwc/pcie-kirin.c +231 ~~-31~~

index 86c13661e02d..de375795a3b8 100644
@@ -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;
 }

Signature · 1 line

2.31.1

Bjorn Helgaas

2021-11-02 16:30 UTC #9 lore raw

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

Quoted 55 lines · 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 <[email protected]>
Acked-by Xiaowei Song <[email protected]>
Signed-off-by Mauro Carvalho Chehab <[email protected]>
---
To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/
 drivers/pci/controller/dwc/pcie-kirin.c | 262 +++++++++++++++++++++---
 1 file changed, 231 insertions(+), 31 deletions(-)
drivers/pci/controller/dwc/pcie-kirin.c +16 -0
index 86c13661e02d..de375795a3b8 100644
@@ -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;

drivers/pci/controller/dwc/pcie-kirin.c struct kirin_pcie { · line 92

Quoted 10 lines

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

+
+	/* 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];

I think there's been previous discussion about this, but I didn't follow it, so I'm just double-checking that this is what we want here.

IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an external PEX 8606 bridge, which seems a little strange to be hard-coded into the kirin driver this way.

I see that "hisilicon,clken-gpios" is optional, but what if some platform connects all 6 lanes? What if there's a different bridge altogether?

I'll assume this is actually the way we want thing unless I hear otherwise.

Quoted 314 lines

drivers/pci/controller/dwc/pcie-kirin.c +205 ~~-31~~

 };
 
 /*

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

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-11-02 17:44 UTC #10 lore raw

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

Hi Bjorn,

Em Tue, 2 Nov 2021 11:06:12 -0500
Bjorn Helgaas <[email protected]> escreveu:

Quoted 13 lines

drivers/pci/controller/dwc/pcie-kirin.c struct kirin_pcie { · line 92
Quoted 5 lines
drivers/pci/controller/dwc/pcie-kirin.c +5 -0
+
+	/* Per-slot clkreq */
+	int		n_gpio_clkreq;
+	int		gpio_id_clkreq[MAX_PCI_SLOTS];
+	const char	*clkreq_names[MAX_PCI_SLOTS];  
I think there's been previous discussion about this, but I didn't follow it, so I'm just double-checking that this is what we want here.
IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an external PEX 8606 bridge, which seems a little strange to be hard-coded into the kirin driver this way.

Quoted 6 lines

I see that "hisilicon,clken-gpios" is optional, but what if some platform connects all 6 lanes? What if there's a different bridge altogether?
I'll assume this is actually the way we want thing unless I hear otherwise.

Yes, there was past discussions about that with Rob, with regards to how the DT would represent it, which got reflected at the code. At the end, it was decided to just add a single property inside PCIe:

		pcie@f4000000 {
                        compatible = "hisilicon,kirin970-pcie";
...
                        hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>,
                                                <&gpio20 6 0>;

I don't think this is a problem, as, if some day another bridge would need a larger number of slots, it is just a matter of changing the number at the MAX_PCI_SLOTS, as this controls only the size of the array (and the check for array overflow when parsing the properties).

Regards,
Mauro

Pali Rohár

2021-11-02 22:08 UTC #11 lore raw

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

Quoted 24 lines · On Tuesday 02 November 2021 17:44:15 Mauro Carvalho Chehab wrote:

Hi Bjorn,
Em Tue, 2 Nov 2021 11:06:12 -0500
Bjorn Helgaas <[email protected]> escreveu:
Quoted 19 lines
drivers/pci/controller/dwc/pcie-kirin.c struct kirin_pcie { · line 92
Quoted 5 lines
drivers/pci/controller/dwc/pcie-kirin.c +5 -0
+
+	/* Per-slot clkreq */
+	int		n_gpio_clkreq;
+	int		gpio_id_clkreq[MAX_PCI_SLOTS];
+	const char	*clkreq_names[MAX_PCI_SLOTS];  
I think there's been previous discussion about this, but I didn't follow it, so I'm just double-checking that this is what we want here.
IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an external PEX 8606 bridge, which seems a little strange to be hard-coded into the kirin driver this way.
I see that "hisilicon,clken-gpios" is optional, but what if some platform connects all 6 lanes? What if there's a different bridge altogether?
I'll assume this is actually the way we want thing unless I hear otherwise.

I proposed alternative approach how to define it:
https://lore.kernel.org/linux-pci/20211023144252.z7ou2l2tvm6cvtf7@pali/

Reason for a my new proposal is that currently there is lot of duplicated code in every native pcie controller driver and every driver is solving same issues by ad-doc code which is not related to host bridge / controller itself. Like configuration of devices (e.g. PCIe switch) to the host bridge itself. That is why I send also another RFC:
https://lore.kernel.org/linux-pci/20211022183808.jdeo7vntnagqkg7g@pali/

I would be happy if we can discuss on them for future drivers. At least if my proposals make sense or completely not.

Quoted 11 lines

Yes, there was past discussions about that with Rob, with regards to how the DT would represent it, which got reflected at the code. At the end, it was decided to just add a single property inside PCIe:
		pcie@f4000000 {
                        compatible = "hisilicon,kirin970-pcie";
...
                        hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>,
                                                <&gpio20 6 0>;

Quoted 4 lines

I don't think this is a problem, as, if some day another bridge would need a larger number of slots, it is just a matter of changing the number at the MAX_PCI_SLOTS, as this controls only the size of the array (and the check for array overflow when parsing the properties).

It is not a problem for this particular pcie controller. And really MAX macro can be increased in this driver if there is need for a larger number of slots. It should work fine.

But if there are going to be added more boards with different hw topology or even with different pcie controllers then there would be new issues. E.g. how to figure out which gpio belongs to which slot? Or even hot-plugging support? Port belongs to either Root Port device or to Downstream device, which does not have to be at root level. It creates tree topology and therefore it is not possible to represent GPIOs in simple list, like it is for kirin DTS. Generally you cannot say to which slot belongs second GPIO defined in reset-gpios list.

That is why I'm saying it needs some better structure and prepare pci core code for it. So native pci controller drivers do not have to invent ad-hoc solutions for specific board setups.

I really think that information about PCIe switch topology should be in DTS and it should work independently of PCIe controller driver, without need for board-specific or PCIe-switch-specific ad-hoc hooks in host bridge drivers, like it is currently.

Bjorn, what do you think?

Rob Herring

reviewer

2021-11-04 15:37 UTC #12 lore raw

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

In reply to Mauro Carvalho Chehab

Quoted 22 lines · Mauro Carvalho Chehab

Hi Bjorn,
Em Tue, 2 Nov 2021 11:06:12 -0500
Bjorn Helgaas <[email protected]> escreveu:
Quoted 16 lines
drivers/pci/controller/dwc/pcie-kirin.c struct kirin_pcie { · line 92
Quoted 5 lines
drivers/pci/controller/dwc/pcie-kirin.c +5 -0
+
+   /* Per-slot clkreq */
+   int             n_gpio_clkreq;
+   int             gpio_id_clkreq[MAX_PCI_SLOTS];
+   const char      *clkreq_names[MAX_PCI_SLOTS];
I think there's been previous discussion about this, but I didn't follow it, so I'm just double-checking that this is what we want here.
IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an external PEX 8606 bridge, which seems a little strange to be hard-coded into the kirin driver this way.
I see that "hisilicon,clken-gpios" is optional, but what if some platform connects all 6 lanes? What if there's a different bridge altogether?

Keep in mind this is HiKey. There's never been a 2nd board upstream for the SoCs, the boards have a short lifespan in terms of both support and availability. And yet Hikey manages to do multiple complicated things on the board design. I have a hard time caring...

Quoted 6 lines

Quoted 3 lines
I'll assume this is actually the way we want thing unless I hear otherwise.
Yes, there was past discussions about that with Rob, with regards to how the DT would represent it, which got reflected at the code.

At first I thought these were CLKREQ connections which absolutely should be per port/bridge like PERST, but they are not. They are simply enables for the refclk's and pretty specific to HiKey.

Quoted 9 lines

At the end, it was decided to just add a single property inside PCIe:

                pcie@f4000000 {
                        compatible = "hisilicon,kirin970-pcie";
...
                        hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>,
                                                <&gpio20 6 0>;

Quoted 4 lines

I don't think this is a problem, as, if some day another bridge would need a larger number of slots, it is just a matter of changing the number at the MAX_PCI_SLOTS, as this controls only the size of the array (and the check for array overflow when parsing the properties).

It is a problem that your host bridge driver has hardcoded board specifics. That's not the first time you've heard that from me. But given the board-to-soc ratio of Hikey is 1:1, I don't care that much.

Rob

Mauro Carvalho Chehab

author

2021-11-04 17:27 UTC #13 lore raw

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

Em Thu, 4 Nov 2021 10:36:52 -0500
Rob Herring <[email protected]> escreveu:

Quoted 50 lines

Quoted 22 lines · Mauro Carvalho Chehab
Hi Bjorn,
Em Tue, 2 Nov 2021 11:06:12 -0500
Bjorn Helgaas <[email protected]> escreveu:
Quoted 16 lines
drivers/pci/controller/dwc/pcie-kirin.c struct kirin_pcie { · line 92
Quoted 5 lines
drivers/pci/controller/dwc/pcie-kirin.c +5 -0
+
+   /* Per-slot clkreq */
+   int             n_gpio_clkreq;
+   int             gpio_id_clkreq[MAX_PCI_SLOTS];
+   const char      *clkreq_names[MAX_PCI_SLOTS];  
I think there's been previous discussion about this, but I didn't follow it, so I'm just double-checking that this is what we want here.
IIUC, this (MAX_PCI_SLOTS, "hisilicon,clken-gpios") applies to an external PEX 8606 bridge, which seems a little strange to be hard-coded into the kirin driver this way.
I see that "hisilicon,clken-gpios" is optional, but what if some platform connects all 6 lanes? What if there's a different bridge altogether?
Keep in mind this is HiKey. There's never been a 2nd board upstream for the SoCs, the boards have a short lifespan in terms of both support and availability. And yet Hikey manages to do multiple complicated things on the board design. I have a hard time caring...
Quoted 6 lines
Quoted 3 lines
I'll assume this is actually the way we want thing unless I hear otherwise.
Yes, there was past discussions about that with Rob, with regards to how the DT would represent it, which got reflected at the code.
At first I thought these were CLKREQ connections which absolutely should be per port/bridge like PERST, but they are not. They are simply enables for the refclk's and pretty specific to HiKey.
Quoted 9 lines
At the end, it was decided to just add a single property inside PCIe:
                pcie@f4000000 {
                        compatible = "hisilicon,kirin970-pcie";
...
                        hisilicon,clken-gpios = <&gpio27 3 0>, <&gpio17 0 0>,
                                                <&gpio20 6 0>;

Quoted 8 lines

Quoted 4 lines
I don't think this is a problem, as, if some day another bridge would need a larger number of slots, it is just a matter of changing the number at the MAX_PCI_SLOTS, as this controls only the size of the array (and the check for array overflow when parsing the properties).
It is a problem that your host bridge driver has hardcoded board specifics. That's not the first time you've heard that from me. But given the board-to-soc ratio of Hikey is 1:1, I don't care that much.

Ok, understood. Yeah, it sounds unlikely that we would get more boards for Kirin 960/970 with PCI support, as this SoC is used for mobile phones, where neither USB nor PCI bridges are needed.

So, AFAIKT, the only the only publicly-available boards that will be using this driver are HiKey 960 and HiKey 970.

Regards,
Mauro

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #14 lore raw

PATCH v15 5/13 PCI: kirin: Give more time for PERST# reset to finish

In reply to 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 <[email protected]>

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

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

index de375795a3b8..bc329673632a 100644
@@ -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

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #15 lore raw

PATCH v15 6/13 PCI: kirin: Add Kirin 970 compatible

In reply to Mauro Carvalho Chehab

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

Acked-by Xiaowei Song <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

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

index bc329673632a..475ef57d5261 100644
@@ -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
+	},
 	{},
 };

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #16 lore raw

PATCH v15 7/13 PCI: kirin: Add MODULE_* macros

In reply to Mauro Carvalho Chehab

This driver misses the MODULE_* macros. Add them.

Acked-by Xiaowei Song <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

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

index 475ef57d5261..b55830d2a19b 100644
@@ -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 <[email protected]>");
+MODULE_LICENSE("GPL v2");

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #17 lore raw

PATCH v15 8/13 PCI: kirin: Allow building it as a module

In reply to 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 <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

drivers/pci/controller/dwc/Kconfig +1 -1

index 071a1fb12beb..62ce3abf0f19 100644
@@ -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

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #18 lore raw

PATCH v15 9/13 PCI: kirin: Add power_off support for Kirin 960 PHY

In reply to Mauro Carvalho Chehab

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

Acked-by Xiaowei Song <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

drivers/pci/controller/dwc/pcie-kirin.c +18 -3

index b55830d2a19b..64221a204db2 100644
@@ -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);

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #19 lore raw

PATCH v15 10/13 PCI: kirin: Move the power-off code to a common routine

In reply to Mauro Carvalho Chehab

Instead of having two copies of the same logic, place the power-off logic on a separate function.

No functional changes.

Acked-by Xiaowei Song <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

drivers/pci/controller/dwc/pcie-kirin.c +15 ~~-11~~

index 64221a204db2..f4ea27b37968 100644
@@ -680,6 +680,19 @@ 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);
+
+	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 +738,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 +747,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 hi3660_pcie_phy_power_off(kirin_pcie);
-
-	phy_power_off(kirin_pcie->phy);
-	phy_exit(kirin_pcie->phy);
+	kirin_pcie_power_off(kirin_pcie);
 
 	return 0;
 }

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #20 lore raw

PATCH v15 11/13 PCI: kirin: Disable clkreq during poweroff sequence

In reply to Mauro Carvalho Chehab

The logic at kirin_pcie_gpio_request() enables some clkreq GPIO lines. Disable them during power-off.

Acked-by Xiaowei Song <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

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

index f4ea27b37968..4c3fa02b7108 100644
@@ -687,6 +687,9 @@ 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++)
+		gpio_direction_output(kirin_pcie->gpio_id_clkreq[i], 1);
+
 	phy_power_off(kirin_pcie->phy);
 	phy_exit(kirin_pcie->phy);

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #21 lore raw

PATCH v15 12/13 PCI: kirin: De-init the dwc driver

In reply to Mauro Carvalho Chehab

The logic under .remove ops is missing a call to dw_pcie_host_deinit(). Add it, in order to allow the DWC core to be properly cleaned up.

Acked-by Xiaowei Song <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

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

index 4c3fa02b7108..fea4d717fff3 100644
@@ -750,6 +750,8 @@ static int __exit kirin_pcie_remove(struct platform_device *pdev)
 {
 	struct kirin_pcie *kirin_pcie = platform_get_drvdata(pdev);
 
+	dw_pcie_host_deinit(&kirin_pcie->pci->pp);
+
 	kirin_pcie_power_off(kirin_pcie);
 
 	return 0;

Signature · 1 line

2.31.1

Mauro Carvalho Chehab

author

2021-10-21 10:45 UTC #22 lore raw

PATCH v15 13/13 PCI: kirin: Allow removing the driver

In reply to 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 <[email protected]>

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

---

To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/

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

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

index fea4d717fff3..cdf568ea0f68 100644
@@ -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");

Signature · 1 line

2.31.1

Pali Rohár

2021-10-31 20:55 UTC #23 lore raw

Re: [PATCH v15 13/13] PCI: kirin: Allow removing the driver

Quoted 19 lines · On Thursday 21 October 2021 11:45:20 Mauro Carvalho Chehab wrote:

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 <[email protected]>
Signed-off-by Mauro Carvalho Chehab <[email protected]>
---
To mailbombing on a large number of people, only mailing lists were C/C on the cover. See [PATCH v15 00/13] at: https://lore.kernel.org/all/[email protected]/
 drivers/pci/controller/dwc/pcie-kirin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/pci/controller/dwc/pcie-kirin.c +0 -0
index fea4d717fff3..cdf568ea0f68 100644

drivers/pci/controller/dwc/pcie-kirin.c static struct platform_driver kirin_pcie_driver = { · line 827

Quoted 1 line

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

@@ -827,7 +827,7 @@ static struct platform_driver kirin_pcie_driver = {
 		.suppress_bind_attrs	= true,

Hello Mauro! I think that you should not set ".suppress_bind_attrs" to "true" if you want to bind and unbind driver at runtime.

Quoted 10 lines

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

 	},
 };
-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");

Signature · 1 line

2.31.1

Lorenzo Pieralisi

2021-10-21 12:17 UTC #24 lore raw

In reply to Mauro Carvalho Chehab

[+Kishon]

Quoted 85 lines · Mauro Carvalho Chehab

Hi Lorenzo,
I split patch 09/10 from v13 into three patches, in order to have one logical change per patch, adding a proper descriptio to each of them. The final code didn change.
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)
---
v15:
- The power-off fix patch was split into 3, in order to have one logical change per patch.
- Removed Fixes: tag from the poweroff patch;
- Adjusted capitalization of two patch summary lines
- No functional changes. The diff of this series is identical to v14.
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 (13):
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: Move the power-off code to a common routine
PCI: kirin: Disable clkreq during poweroff sequence
PCI: kirin: De-init the dwc driver
PCI: kirin: Allow removing the driver

Quoted 3 lines

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

Hi Kishon,

can I ask you please to have a look at patches [1,2] ?

I'd like to merge this series for v5.16, please let me know.

Thanks,
Lorenzo

Lorenzo Pieralisi

Applied

2021-10-26 17:17 UTC #25 lore raw

In reply to Mauro Carvalho Chehab

Quoted 85 lines · Mauro Carvalho Chehab

Hi Lorenzo,
I split patch 09/10 from v13 into three patches, in order to have one logical change per patch, adding a proper descriptio to each of them. The final code didn change.
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)
---
v15:
- The power-off fix patch was split into 3, in order to have one logical change per patch.
- Removed Fixes: tag from the poweroff patch;
- Adjusted capitalization of two patch summary lines
- No functional changes. The diff of this series is identical to v14.
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 (13):
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: Move the power-off code to a common routine
PCI: kirin: Disable clkreq during poweroff sequence
PCI: kirin: De-init the dwc driver
PCI: kirin: Allow removing the driver

Quoted 3 lines

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

Applied to pci/dwc for v5.16, please have a look at the resulting branch and commits.

Thanks,
Lorenzo

Mauro Carvalho Chehab

author

2021-10-27 07:25 UTC #26 lore raw

Em Tue, 26 Oct 2021 18:17:28 +0100
Lorenzo Pieralisi <[email protected]> escreveu:

Quoted 86 lines · Mauro Carvalho Chehab

Quoted 85 lines
Hi Lorenzo,
I split patch 09/10 from v13 into three patches, in order to have one logical change per patch, adding a proper descriptio to each of them. The final code didn change.
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)
---
v15:
- The power-off fix patch was split into 3, in order to have one logical change per patch.
- Removed Fixes: tag from the poweroff patch;
- Adjusted capitalization of two patch summary lines
- No functional changes. The diff of this series is identical to v14.
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 (13):
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: Move the power-off code to a common routine
PCI: kirin: Disable clkreq during poweroff sequence
PCI: kirin: De-init the dwc driver
PCI: kirin: Allow removing the driver

Quoted 6 lines

Quoted 3 lines
 drivers/pci/controller/dwc/Kconfig      |   2 +-
 drivers/pci/controller/dwc/pcie-kirin.c | 643 ++++++++++++++++++------
 2 files changed, 497 insertions(+), 148 deletions(-)  
Applied to pci/dwc for v5.16, please have a look at the resulting branch and commits.

Thanks!

Everything looks fine to me.

Best regards,
Mauro