Introduce a driver for the PCIe root complex found in the SpacemiT
K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
The driver supports three PCIe ports that operate at PCIe v2 transfer
rates (5 GT/sec).  The first port uses a combo PHY, which may be
configured for use for USB 3 instead.

Signed-off-by: Alex Elder <elder@riscstar.com>
---
 drivers/pci/controller/dwc/Kconfig   |  10 +
 drivers/pci/controller/dwc/Makefile  |   1 +
 drivers/pci/controller/dwc/pcie-k1.c | 355 +++++++++++++++++++++++++++
 3 files changed, 366 insertions(+)
 create mode 100644 drivers/pci/controller/dwc/pcie-k1.c

diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
index ff6b6d9e18ecf..ca5782c041ce8 100644
--- a/drivers/pci/controller/dwc/Kconfig
+++ b/drivers/pci/controller/dwc/Kconfig
@@ -492,4 +492,14 @@ config PCIE_VISCONTI_HOST
 	  Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
 	  This driver supports TMPV7708 SoC.
 
+config PCIE_K1
+	bool "SpacemiT K1 host mode PCIe controller"
+	depends on ARCH_SPACEMIT || COMPILE_TEST
+	depends on PCI && OF && HAS_IOMEM
+	select PCIE_DW_HOST
+	default ARCH_SPACEMIT
+	help
+	  Enables support for the PCIe controller in the K1 SoC operating
+	  in host mode.
+
 endmenu
diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
index 6919d27798d13..62d9d4e7dd4d3 100644
--- a/drivers/pci/controller/dwc/Makefile
+++ b/drivers/pci/controller/dwc/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
 obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
 obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
 obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
+obj-$(CONFIG_PCIE_K1) += pcie-k1.o
 
 # The following drivers are for devices that use the generic ACPI
 # pci_root.c driver but don't support standard ECAM config access.
diff --git a/drivers/pci/controller/dwc/pcie-k1.c b/drivers/pci/controller/dwc/pcie-k1.c
new file mode 100644
index 0000000000000..e9b1df3428d16
--- /dev/null
+++ b/drivers/pci/controller/dwc/pcie-k1.c
@@ -0,0 +1,355 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * SpacemiT K1 PCIe host driver
+ *
+ * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
+ * Copyright (c) 2023, spacemit Corporation.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gfp.h>
+#include <linux/irq.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/pci.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+#include <linux/types.h>
+
+#include "pcie-designware.h"
+
+#define K1_PCIE_VENDOR_ID	0x201f
+#define K1_PCIE_DEVICE_ID	0x0001
+
+/* Offsets and field definitions of link management registers */
+
+#define K1_PHY_AHB_IRQ_EN			0x0000
+#define PCIE_INTERRUPT_EN		BIT(0)
+
+#define K1_PHY_AHB_LINK_STS			0x0004
+#define SMLH_LINK_UP			BIT(1)
+#define RDLH_LINK_UP			BIT(12)
+
+#define INTR_ENABLE				0x0014
+#define MSI_CTRL_INT			BIT(11)
+
+/* Offsets and field definitions for PMU registers */
+
+#define PCIE_CLK_RESET_CONTROL			0x0000
+#define LTSSM_EN			BIT(6)
+#define PCIE_AUX_PWR_DET		BIT(9)
+#define PCIE_RC_PERST			BIT(12)	/* 0: PERST# high; 1: low */
+#define APP_HOLD_PHY_RST		BIT(30)
+#define DEVICE_TYPE_RC			BIT(31)	/* 0: endpoint; 1: RC */
+
+#define PCIE_CONTROL_LOGIC			0x0004
+#define PCIE_SOFT_RESET			BIT(0)
+
+struct k1_pcie {
+	struct dw_pcie pci;
+	void __iomem *link;
+	struct regmap *pmu;
+	u32 pmu_off;
+	struct phy *phy;
+	struct reset_control *global_reset;
+};
+
+#define to_k1_pcie(dw_pcie)	dev_get_drvdata((dw_pcie)->dev)
+
+static int k1_pcie_toggle_soft_reset(struct k1_pcie *k1)
+{
+	u32 offset = k1->pmu_off + PCIE_CONTROL_LOGIC;
+	const u32 mask = PCIE_SOFT_RESET;
+	int ret;
+
+	ret = regmap_set_bits(k1->pmu, offset, mask);
+	if (ret)
+		return ret;
+
+	mdelay(2);
+
+	return regmap_clear_bits(k1->pmu, offset, mask);
+}
+
+/* Enable app clocks, deassert app resets */
+static int k1_pcie_app_enable(struct k1_pcie *k1)
+{
+	struct dw_pcie *pci = &k1->pci;
+	u32 clock_count;
+	u32 reset_count;
+	int ret;
+
+	clock_count = ARRAY_SIZE(pci->app_clks);
+	ret = clk_bulk_prepare_enable(clock_count, pci->app_clks);
+	if (ret)
+		return ret;
+
+	reset_count = ARRAY_SIZE(pci->app_rsts);
+	ret = reset_control_bulk_deassert(reset_count, pci->app_rsts);
+	if (ret)
+		goto err_disable_clks;
+
+	ret = reset_control_deassert(k1->global_reset);
+	if (ret)
+		goto err_assert_resets;
+
+	return 0;
+
+err_assert_resets:
+	(void)reset_control_bulk_assert(reset_count, pci->app_rsts);
+err_disable_clks:
+	clk_bulk_disable_unprepare(clock_count, pci->app_clks);
+
+	return ret;
+}
+
+/* Disable app clocks, assert app resets */
+static void k1_pcie_app_disable(struct k1_pcie *k1)
+{
+	struct dw_pcie *pci = &k1->pci;
+	u32 count;
+	int ret;
+
+	(void)reset_control_assert(k1->global_reset);
+
+	count = ARRAY_SIZE(pci->app_rsts);
+	ret = reset_control_bulk_assert(count, pci->app_rsts);
+	if (ret)
+		dev_err(pci->dev, "app reset assert failed (%d)\n", ret);
+
+	count = ARRAY_SIZE(pci->app_clks);
+	clk_bulk_disable_unprepare(count, pci->app_clks);
+}
+
+static int k1_pcie_init(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+	u32 offset;
+	u32 mask;
+	int ret;
+
+	ret = k1_pcie_toggle_soft_reset(k1);
+	if (ret)
+		goto err_app_disable;
+
+	ret = k1_pcie_app_enable(k1);
+	if (ret)
+		return ret;
+
+	ret = phy_init(k1->phy);
+	if (ret)
+		goto err_app_disable;
+
+	/* Set the PCI vendor and device ID */
+	dw_pcie_dbi_ro_wr_en(pci);
+	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, K1_PCIE_VENDOR_ID);
+	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, K1_PCIE_DEVICE_ID);
+	dw_pcie_dbi_ro_wr_dis(pci);
+
+	/*
+	 * Put the port in root complex mode, record that Vaux is present.
+	 * Assert fundamental reset (drive PERST# low).
+	 */
+	offset = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
+	mask = DEVICE_TYPE_RC | PCIE_AUX_PWR_DET;
+	mask |= PCIE_RC_PERST;
+	ret = regmap_set_bits(k1->pmu, offset, mask);
+	if (ret)
+		goto err_phy_exit;
+
+	/* Wait the PCIe-mandated 100 msec before deasserting PERST# */
+	mdelay(100);
+
+	ret = regmap_clear_bits(k1->pmu, offset, PCIE_RC_PERST);
+	if (!ret)
+		return 0;	/* Success! */
+
+err_phy_exit:
+	(void)phy_exit(k1->phy);
+err_app_disable:
+	k1_pcie_app_disable(k1);
+
+	return ret;
+}
+
+/* Silently ignore any errors */
+static void k1_pcie_deinit(struct dw_pcie_rp *pp)
+{
+	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+
+	/* Re-assert fundamental reset (drive PERST# low) */
+	(void)regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
+			      PCIE_RC_PERST);
+
+	(void)phy_exit(k1->phy);
+
+	k1_pcie_app_disable(k1);
+}
+
+static const struct dw_pcie_host_ops k1_pcie_host_ops = {
+	.init		= k1_pcie_init,
+	.deinit		= k1_pcie_deinit,
+};
+
+static void k1_pcie_enable_interrupts(struct k1_pcie *k1)
+{
+	void __iomem *virt;
+	u32 val;
+
+	/* Enable the MSI interrupt */
+	writel(MSI_CTRL_INT, k1->link + INTR_ENABLE);
+
+	/* Top-level interrupt enable */
+	virt = k1->link + K1_PHY_AHB_IRQ_EN;
+	val = readl(virt);
+	val |= PCIE_INTERRUPT_EN;
+	writel(val, virt);
+}
+
+static void k1_pcie_disable_interrupts(struct k1_pcie *k1)
+{
+	void __iomem *virt;
+	u32 val;
+
+	virt = k1->link + K1_PHY_AHB_IRQ_EN;
+	val = readl(virt);
+	val &= ~PCIE_INTERRUPT_EN;
+	writel(val, virt);
+
+	writel(0, k1->link + INTR_ENABLE);
+}
+
+static bool k1_pcie_link_up(struct dw_pcie *pci)
+{
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+	u32 val;
+
+	val = readl(k1->link + K1_PHY_AHB_LINK_STS);
+
+	return (val & RDLH_LINK_UP) && (val & SMLH_LINK_UP);
+}
+
+static int k1_pcie_start_link(struct dw_pcie *pci)
+{
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+	int ret;
+
+	/* Stop holding the PHY in reset, and enable link training */
+	ret = regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
+				 APP_HOLD_PHY_RST | LTSSM_EN, LTSSM_EN);
+	if (ret)
+		return ret;
+
+	k1_pcie_enable_interrupts(k1);
+
+	return 0;
+}
+
+static void k1_pcie_stop_link(struct dw_pcie *pci)
+{
+	struct k1_pcie *k1 = to_k1_pcie(pci);
+	int ret;
+
+	k1_pcie_disable_interrupts(k1);
+
+	/* Disable the link and hold the PHY in reset */
+	ret = regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
+				 APP_HOLD_PHY_RST | LTSSM_EN, APP_HOLD_PHY_RST);
+	if (ret)
+		dev_err(pci->dev, "disable LTSSM failed (%d)\n", ret);
+}
+
+static const struct dw_pcie_ops k1_pcie_ops = {
+	.link_up	= k1_pcie_link_up,
+	.start_link	= k1_pcie_start_link,
+	.stop_link	= k1_pcie_stop_link,
+};
+
+static int k1_pcie_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct dw_pcie_rp *pp;
+	struct dw_pcie *pci;
+	struct k1_pcie *k1;
+	int ret;
+
+	k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
+	if (!k1)
+		return -ENOMEM;
+	dev_set_drvdata(dev, k1);
+
+	k1->pmu = syscon_regmap_lookup_by_phandle_args(dev_of_node(dev),
+						       "spacemit,syscon-pmu",
+						       1, &k1->pmu_off);
+	if (IS_ERR(k1->pmu))
+		return dev_err_probe(dev, PTR_ERR(k1->pmu),
+				     "lookup PMU regmap failed\n");
+
+	k1->link = devm_platform_ioremap_resource_byname(pdev, "link");
+	if (!k1->link)
+		return dev_err_probe(dev, -ENOMEM, "map link regs failed\n");
+
+	k1->global_reset = devm_reset_control_get_shared(dev, "global");
+	if (IS_ERR(k1->global_reset))
+		return dev_err_probe(dev, PTR_ERR(k1->global_reset),
+				     "get global reset failed\n");
+
+	/* Hold the PHY in reset until we start the link */
+	ret = regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
+			      APP_HOLD_PHY_RST);
+	if (ret)
+		return dev_err_probe(dev, ret, "hold PHY in reset failed\n");
+
+	k1->phy = devm_phy_get(dev, NULL);
+	if (IS_ERR(k1->phy))
+		return dev_err_probe(dev, PTR_ERR(k1->phy), "get PHY failed\n");
+
+	pci = &k1->pci;
+	dw_pcie_cap_set(pci, REQ_RES);
+	pci->dev = dev;
+	pci->ops = &k1_pcie_ops;
+
+	pp = &pci->pp;
+	pp->num_vectors = MAX_MSI_IRQS;
+	pp->ops = &k1_pcie_host_ops;
+
+	ret = dw_pcie_host_init(pp);
+	if (ret)
+		return dev_err_probe(dev, ret, "host init failed\n");
+
+	return 0;
+}
+
+static void k1_pcie_remove(struct platform_device *pdev)
+{
+	struct k1_pcie *k1 = dev_get_drvdata(&pdev->dev);
+	struct dw_pcie_rp *pp = &k1->pci.pp;
+
+	dw_pcie_host_deinit(pp);
+}
+
+static const struct of_device_id k1_pcie_of_match_table[] = {
+	{ .compatible = "spacemit,k1-pcie-rc", },
+	{ },
+};
+
+static struct platform_driver k1_pcie_driver = {
+	.probe	= k1_pcie_probe,
+	.remove	= k1_pcie_remove,
+	.driver = {
+		.name			= "k1-dwc-pcie",
+		.of_match_table		= k1_pcie_of_match_table,
+		.suppress_bind_attrs	= true,
+	},
+};
+module_platform_driver(k1_pcie_driver);
-- 
2.48.1

On Wed, Aug 13, 2025 at 01:46:59PM GMT, Alex Elder wrote:
> Introduce a driver for the PCIe root complex found in the SpacemiT
> K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
> The driver supports three PCIe ports that operate at PCIe v2 transfer
> rates (5 GT/sec).  The first port uses a combo PHY, which may be
> configured for use for USB 3 instead.
> 
> Signed-off-by: Alex Elder <elder@riscstar.com>
> ---
>  drivers/pci/controller/dwc/Kconfig   |  10 +
>  drivers/pci/controller/dwc/Makefile  |   1 +
>  drivers/pci/controller/dwc/pcie-k1.c | 355 +++++++++++++++++++++++++++
>  3 files changed, 366 insertions(+)
>  create mode 100644 drivers/pci/controller/dwc/pcie-k1.c
> 
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index ff6b6d9e18ecf..ca5782c041ce8 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -492,4 +492,14 @@ config PCIE_VISCONTI_HOST
>  	  Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
>  	  This driver supports TMPV7708 SoC.
>  
> +config PCIE_K1
> +	bool "SpacemiT K1 host mode PCIe controller"

No need to make it bool, build it as a module. Only the PCI controller drivers
implementing irqchip need to be kept bool for irq disposal concerns.

> +	depends on ARCH_SPACEMIT || COMPILE_TEST
> +	depends on PCI && OF && HAS_IOMEM
> +	select PCIE_DW_HOST
> +	default ARCH_SPACEMIT
> +	help
> +	  Enables support for the PCIe controller in the K1 SoC operating
> +	  in host mode.

Is the driver only applicable for K1 SoCs or other SoCs from spacemit? Even if
it is the former, I would suggest renaming to 'pcie-spacemit-k1.c'

> +
>  endmenu
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index 6919d27798d13..62d9d4e7dd4d3 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>  obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
>  obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
>  obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
> +obj-$(CONFIG_PCIE_K1) += pcie-k1.o
>  
>  # The following drivers are for devices that use the generic ACPI
>  # pci_root.c driver but don't support standard ECAM config access.
> diff --git a/drivers/pci/controller/dwc/pcie-k1.c b/drivers/pci/controller/dwc/pcie-k1.c
> new file mode 100644
> index 0000000000000..e9b1df3428d16
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-k1.c
> @@ -0,0 +1,355 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * SpacemiT K1 PCIe host driver
> + *
> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
> + * Copyright (c) 2023, spacemit Corporation.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gfp.h>
> +#include <linux/irq.h>

unused?

> +#include <linux/mfd/syscon.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/of.h>
> +#include <linux/pci.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +#include <linux/types.h>
> +
> +#include "pcie-designware.h"
> +
> +#define K1_PCIE_VENDOR_ID	0x201f
> +#define K1_PCIE_DEVICE_ID	0x0001
> +
> +/* Offsets and field definitions of link management registers */
> +
> +#define K1_PHY_AHB_IRQ_EN			0x0000
> +#define PCIE_INTERRUPT_EN		BIT(0)
> +
> +#define K1_PHY_AHB_LINK_STS			0x0004
> +#define SMLH_LINK_UP			BIT(1)
> +#define RDLH_LINK_UP			BIT(12)
> +
> +#define INTR_ENABLE				0x0014
> +#define MSI_CTRL_INT			BIT(11)
> +
> +/* Offsets and field definitions for PMU registers */
> +
> +#define PCIE_CLK_RESET_CONTROL			0x0000
> +#define LTSSM_EN			BIT(6)
> +#define PCIE_AUX_PWR_DET		BIT(9)
> +#define PCIE_RC_PERST			BIT(12)	/* 0: PERST# high; 1: low */
> +#define APP_HOLD_PHY_RST		BIT(30)
> +#define DEVICE_TYPE_RC			BIT(31)	/* 0: endpoint; 1: RC */
> +
> +#define PCIE_CONTROL_LOGIC			0x0004
> +#define PCIE_SOFT_RESET			BIT(0)
> +
> +struct k1_pcie {
> +	struct dw_pcie pci;
> +	void __iomem *link;
> +	struct regmap *pmu;
> +	u32 pmu_off;
> +	struct phy *phy;
> +	struct reset_control *global_reset;
> +};
> +
> +#define to_k1_pcie(dw_pcie)	dev_get_drvdata((dw_pcie)->dev)
> +
> +static int k1_pcie_toggle_soft_reset(struct k1_pcie *k1)
> +{
> +	u32 offset = k1->pmu_off + PCIE_CONTROL_LOGIC;
> +	const u32 mask = PCIE_SOFT_RESET;
> +	int ret;
> +
> +	ret = regmap_set_bits(k1->pmu, offset, mask);

For MMIO, it is OK to skip the error handling.

> +	if (ret)
> +		return ret;
> +
> +	mdelay(2);

If the previous write to the PMU got stuck in the CPU cache, there is no
guarantee that this delay of 2ms between write and clear will be enforced. So
you should do a dummy read after write to ensure that the previous write has
reached the PMU (or any device) and then clear the bits.

> +
> +	return regmap_clear_bits(k1->pmu, offset, mask);
> +}
> +
> +/* Enable app clocks, deassert app resets */
> +static int k1_pcie_app_enable(struct k1_pcie *k1)
> +{
> +	struct dw_pcie *pci = &k1->pci;
> +	u32 clock_count;
> +	u32 reset_count;
> +	int ret;
> +
> +	clock_count = ARRAY_SIZE(pci->app_clks);

Just use ARRAY_SIZE() directly below.

> +	ret = clk_bulk_prepare_enable(clock_count, pci->app_clks);
> +	if (ret)
> +		return ret;
> +
> +	reset_count = ARRAY_SIZE(pci->app_rsts);

Same here.

> +	ret = reset_control_bulk_deassert(reset_count, pci->app_rsts);
> +	if (ret)
> +		goto err_disable_clks;
> +
> +	ret = reset_control_deassert(k1->global_reset);
> +	if (ret)
> +		goto err_assert_resets;
> +
> +	return 0;
> +
> +err_assert_resets:
> +	(void)reset_control_bulk_assert(reset_count, pci->app_rsts);

Why void cast? Here and in other places.

> +err_disable_clks:
> +	clk_bulk_disable_unprepare(clock_count, pci->app_clks);
> +
> +	return ret;
> +}
> +
> +/* Disable app clocks, assert app resets */
> +static void k1_pcie_app_disable(struct k1_pcie *k1)
> +{
> +	struct dw_pcie *pci = &k1->pci;
> +	u32 count;
> +	int ret;
> +
> +	(void)reset_control_assert(k1->global_reset);
> +
> +	count = ARRAY_SIZE(pci->app_rsts);
> +	ret = reset_control_bulk_assert(count, pci->app_rsts);
> +	if (ret)
> +		dev_err(pci->dev, "app reset assert failed (%d)\n", ret);
> +
> +	count = ARRAY_SIZE(pci->app_clks);
> +	clk_bulk_disable_unprepare(count, pci->app_clks);
> +}

Same comments as k1_pcie_app_enable().

> +
> +static int k1_pcie_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct k1_pcie *k1 = to_k1_pcie(pci);
> +	u32 offset;
> +	u32 mask;
> +	int ret;
> +
> +	ret = k1_pcie_toggle_soft_reset(k1);
> +	if (ret)
> +		goto err_app_disable;
> +
> +	ret = k1_pcie_app_enable(k1);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_init(k1->phy);
> +	if (ret)
> +		goto err_app_disable;
> +
> +	/* Set the PCI vendor and device ID */
> +	dw_pcie_dbi_ro_wr_en(pci);
> +	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, K1_PCIE_VENDOR_ID);
> +	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, K1_PCIE_DEVICE_ID);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	/*
> +	 * Put the port in root complex mode, record that Vaux is present.

There is no 3.3Vaux supply present in the binding. So the supply is guaranteed
to be present and enabled always by the platform?

> +	 * Assert fundamental reset (drive PERST# low).
> +	 */
> +	offset = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
> +	mask = DEVICE_TYPE_RC | PCIE_AUX_PWR_DET;
> +	mask |= PCIE_RC_PERST;
> +	ret = regmap_set_bits(k1->pmu, offset, mask);
> +	if (ret)
> +		goto err_phy_exit;
> +
> +	/* Wait the PCIe-mandated 100 msec before deasserting PERST# */
> +	mdelay(100);

Same comment as k1_pcie_toggle_soft_reset() applies here.

> +
> +	ret = regmap_clear_bits(k1->pmu, offset, PCIE_RC_PERST);
> +	if (!ret)
> +		return 0;	/* Success! */

Please use common pattern to return success:

	regmap_clear_bits()

	return 0;

> +
> +err_phy_exit:
> +	(void)phy_exit(k1->phy);
> +err_app_disable:
> +	k1_pcie_app_disable(k1);
> +
> +	return ret;
> +}
> +
> +/* Silently ignore any errors */
> +static void k1_pcie_deinit(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct k1_pcie *k1 = to_k1_pcie(pci);
> +
> +	/* Re-assert fundamental reset (drive PERST# low) */

s/Re-assert/Assert

> +	(void)regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> +			      PCIE_RC_PERST);
> +
> +	(void)phy_exit(k1->phy);
> +
> +	k1_pcie_app_disable(k1);
> +}
> +
> +static const struct dw_pcie_host_ops k1_pcie_host_ops = {
> +	.init		= k1_pcie_init,
> +	.deinit		= k1_pcie_deinit,
> +};
> +
> +static void k1_pcie_enable_interrupts(struct k1_pcie *k1)
> +{
> +	void __iomem *virt;
> +	u32 val;
> +
> +	/* Enable the MSI interrupt */
> +	writel(MSI_CTRL_INT, k1->link + INTR_ENABLE);

If there are no ordering issues (I guess so), you can very well use the _relaxed
variants throughout the driver.

> +
> +	/* Top-level interrupt enable */
> +	virt = k1->link + K1_PHY_AHB_IRQ_EN;
> +	val = readl(virt);
> +	val |= PCIE_INTERRUPT_EN;
> +	writel(val, virt);
> +}
> +
> +static void k1_pcie_disable_interrupts(struct k1_pcie *k1)
> +{
> +	void __iomem *virt;
> +	u32 val;
> +
> +	virt = k1->link + K1_PHY_AHB_IRQ_EN;
> +	val = readl(virt);
> +	val &= ~PCIE_INTERRUPT_EN;
> +	writel(val, virt);
> +
> +	writel(0, k1->link + INTR_ENABLE);
> +}
> +
> +static bool k1_pcie_link_up(struct dw_pcie *pci)
> +{
> +	struct k1_pcie *k1 = to_k1_pcie(pci);
> +	u32 val;
> +
> +	val = readl(k1->link + K1_PHY_AHB_LINK_STS);
> +
> +	return (val & RDLH_LINK_UP) && (val & SMLH_LINK_UP);
> +}
> +
> +static int k1_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct k1_pcie *k1 = to_k1_pcie(pci);
> +	int ret;
> +
> +	/* Stop holding the PHY in reset, and enable link training */
> +	ret = regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> +				 APP_HOLD_PHY_RST | LTSSM_EN, LTSSM_EN);
> +	if (ret)
> +		return ret;
> +
> +	k1_pcie_enable_interrupts(k1);
> +
> +	return 0;
> +}
> +
> +static void k1_pcie_stop_link(struct dw_pcie *pci)
> +{
> +	struct k1_pcie *k1 = to_k1_pcie(pci);
> +	int ret;
> +
> +	k1_pcie_disable_interrupts(k1);
> +
> +	/* Disable the link and hold the PHY in reset */
> +	ret = regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> +				 APP_HOLD_PHY_RST | LTSSM_EN, APP_HOLD_PHY_RST);
> +	if (ret)
> +		dev_err(pci->dev, "disable LTSSM failed (%d)\n", ret);
> +}
> +
> +static const struct dw_pcie_ops k1_pcie_ops = {
> +	.link_up	= k1_pcie_link_up,
> +	.start_link	= k1_pcie_start_link,
> +	.stop_link	= k1_pcie_stop_link,
> +};
> +
> +static int k1_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie_rp *pp;
> +	struct dw_pcie *pci;
> +	struct k1_pcie *k1;
> +	int ret;
> +
> +	k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
> +	if (!k1)
> +		return -ENOMEM;
> +	dev_set_drvdata(dev, k1);
> +
> +	k1->pmu = syscon_regmap_lookup_by_phandle_args(dev_of_node(dev),
> +						       "spacemit,syscon-pmu",
> +						       1, &k1->pmu_off);
> +	if (IS_ERR(k1->pmu))
> +		return dev_err_probe(dev, PTR_ERR(k1->pmu),
> +				     "lookup PMU regmap failed\n");

'Failed to lookup \"PMU\" registers'

> +
> +	k1->link = devm_platform_ioremap_resource_byname(pdev, "link");
> +	if (!k1->link)
> +		return dev_err_probe(dev, -ENOMEM, "map link regs failed\n");

'Failed to map \"link\" registers

Same for below error prints as well.

> +
> +	k1->global_reset = devm_reset_control_get_shared(dev, "global");
> +	if (IS_ERR(k1->global_reset))
> +		return dev_err_probe(dev, PTR_ERR(k1->global_reset),
> +				     "get global reset failed\n");
> +
> +	/* Hold the PHY in reset until we start the link */
> +	ret = regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
> +			      APP_HOLD_PHY_RST);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "hold PHY in reset failed\n");
> +
> +	k1->phy = devm_phy_get(dev, NULL);
> +	if (IS_ERR(k1->phy))
> +		return dev_err_probe(dev, PTR_ERR(k1->phy), "get PHY failed\n");
> +
> +	pci = &k1->pci;
> +	dw_pcie_cap_set(pci, REQ_RES);
> +	pci->dev = dev;
> +	pci->ops = &k1_pcie_ops;
> +
> +	pp = &pci->pp;
> +	pp->num_vectors = MAX_MSI_IRQS;

I don't understand how MSI is implemented in this platform. If the controller
relies on an external interrupt controller for handling MSIs (I think it does),
then there should be either 'msi-parent' or 'msi-map' existed in the binding.

But I see none, other than 'interrupts-extended'. So I don't know how MSI works
at all.

> +	pp->ops = &k1_pcie_host_ops;
> +
> +	ret = dw_pcie_host_init(pp);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "host init failed\n");
> +
> +	return 0;
> +}
> +
> +static void k1_pcie_remove(struct platform_device *pdev)
> +{
> +	struct k1_pcie *k1 = dev_get_drvdata(&pdev->dev);
> +	struct dw_pcie_rp *pp = &k1->pci.pp;
> +
> +	dw_pcie_host_deinit(pp);
> +}
> +
> +static const struct of_device_id k1_pcie_of_match_table[] = {
> +	{ .compatible = "spacemit,k1-pcie-rc", },
> +	{ },
> +};
> +
> +static struct platform_driver k1_pcie_driver = {
> +	.probe	= k1_pcie_probe,
> +	.remove	= k1_pcie_remove,
> +	.driver = {
> +		.name			= "k1-dwc-pcie",
> +		.of_match_table		= k1_pcie_of_match_table,
> +		.suppress_bind_attrs	= true,

No need of this flag for the reason I mentioned in the Kcofig change.

You should also set,

	.probe_type = PROBE_PREFER_ASYNCHRONOUS,

to make use of the async probing of the devices during boot. This does save some
boot time.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

On 9/15/25 3:09 AM, Manivannan Sadhasivam wrote:
> On Wed, Aug 13, 2025 at 01:46:59PM GMT, Alex Elder wrote:
>> Introduce a driver for the PCIe root complex found in the SpacemiT
>> K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
>> The driver supports three PCIe ports that operate at PCIe v2 transfer
>> rates (5 GT/sec).  The first port uses a combo PHY, which may be
>> configured for use for USB 3 instead.
>>
>> Signed-off-by: Alex Elder <elder@riscstar.com>
>> ---
>>   drivers/pci/controller/dwc/Kconfig   |  10 +
>>   drivers/pci/controller/dwc/Makefile  |   1 +
>>   drivers/pci/controller/dwc/pcie-k1.c | 355 +++++++++++++++++++++++++++
>>   3 files changed, 366 insertions(+)
>>   create mode 100644 drivers/pci/controller/dwc/pcie-k1.c
>>
>> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
>> index ff6b6d9e18ecf..ca5782c041ce8 100644
>> --- a/drivers/pci/controller/dwc/Kconfig
>> +++ b/drivers/pci/controller/dwc/Kconfig
>> @@ -492,4 +492,14 @@ config PCIE_VISCONTI_HOST
>>   	  Say Y here if you want PCIe controller support on Toshiba Visconti SoC.
>>   	  This driver supports TMPV7708 SoC.
>>   
>> +config PCIE_K1
>> +	bool "SpacemiT K1 host mode PCIe controller"
> 
> No need to make it bool, build it as a module. Only the PCI controller drivers
> implementing irqchip need to be kept bool for irq disposal concerns.

OK.

>> +	depends on ARCH_SPACEMIT || COMPILE_TEST
>> +	depends on PCI && OF && HAS_IOMEM
>> +	select PCIE_DW_HOST
>> +	default ARCH_SPACEMIT
>> +	help
>> +	  Enables support for the PCIe controller in the K1 SoC operating
>> +	  in host mode.
> 
> Is the driver only applicable for K1 SoCs or other SoCs from spacemit? Even if
> it is the former, I would suggest renaming to 'pcie-spacemit-k1.c'

Yes, I will do that.

>>   endmenu
>> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
>> index 6919d27798d13..62d9d4e7dd4d3 100644
>> --- a/drivers/pci/controller/dwc/Makefile
>> +++ b/drivers/pci/controller/dwc/Makefile
>> @@ -31,6 +31,7 @@ obj-$(CONFIG_PCIE_UNIPHIER) += pcie-uniphier.o
>>   obj-$(CONFIG_PCIE_UNIPHIER_EP) += pcie-uniphier-ep.o
>>   obj-$(CONFIG_PCIE_VISCONTI_HOST) += pcie-visconti.o
>>   obj-$(CONFIG_PCIE_RCAR_GEN4) += pcie-rcar-gen4.o
>> +obj-$(CONFIG_PCIE_K1) += pcie-k1.o
>>   
>>   # The following drivers are for devices that use the generic ACPI
>>   # pci_root.c driver but don't support standard ECAM config access.
>> diff --git a/drivers/pci/controller/dwc/pcie-k1.c b/drivers/pci/controller/dwc/pcie-k1.c
>> new file mode 100644
>> index 0000000000000..e9b1df3428d16
>> --- /dev/null
>> +++ b/drivers/pci/controller/dwc/pcie-k1.c
>> @@ -0,0 +1,355 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * SpacemiT K1 PCIe host driver
>> + *
>> + * Copyright (C) 2025 by RISCstar Solutions Corporation.  All rights reserved.
>> + * Copyright (c) 2023, spacemit Corporation.
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/clk.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/gfp.h>
>> +#include <linux/irq.h>
> 
> unused?

Yes, and there are a few others I can get rid of too.

>> +#include <linux/mfd/syscon.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/of.h>
>> +#include <linux/pci.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
>> +#include <linux/types.h>
>> +
>> +#include "pcie-designware.h"
>> +
>> +#define K1_PCIE_VENDOR_ID	0x201f
>> +#define K1_PCIE_DEVICE_ID	0x0001
>> +
>> +/* Offsets and field definitions of link management registers */
>> +
>> +#define K1_PHY_AHB_IRQ_EN			0x0000
>> +#define PCIE_INTERRUPT_EN		BIT(0)
>> +
>> +#define K1_PHY_AHB_LINK_STS			0x0004
>> +#define SMLH_LINK_UP			BIT(1)
>> +#define RDLH_LINK_UP			BIT(12)
>> +
>> +#define INTR_ENABLE				0x0014
>> +#define MSI_CTRL_INT			BIT(11)
>> +
>> +/* Offsets and field definitions for PMU registers */
>> +
>> +#define PCIE_CLK_RESET_CONTROL			0x0000
>> +#define LTSSM_EN			BIT(6)
>> +#define PCIE_AUX_PWR_DET		BIT(9)
>> +#define PCIE_RC_PERST			BIT(12)	/* 0: PERST# high; 1: low */
>> +#define APP_HOLD_PHY_RST		BIT(30)
>> +#define DEVICE_TYPE_RC			BIT(31)	/* 0: endpoint; 1: RC */
>> +
>> +#define PCIE_CONTROL_LOGIC			0x0004
>> +#define PCIE_SOFT_RESET			BIT(0)
>> +
>> +struct k1_pcie {
>> +	struct dw_pcie pci;
>> +	void __iomem *link;
>> +	struct regmap *pmu;
>> +	u32 pmu_off;
>> +	struct phy *phy;
>> +	struct reset_control *global_reset;
>> +};
>> +
>> +#define to_k1_pcie(dw_pcie)	dev_get_drvdata((dw_pcie)->dev)
>> +
>> +static int k1_pcie_toggle_soft_reset(struct k1_pcie *k1)
>> +{
>> +	u32 offset = k1->pmu_off + PCIE_CONTROL_LOGIC;
>> +	const u32 mask = PCIE_SOFT_RESET;
>> +	int ret;
>> +
>> +	ret = regmap_set_bits(k1->pmu, offset, mask);
> 
> For MMIO, it is OK to skip the error handling.

You mean even though the regmap API returns an error,
it never will with MMIO?
- regmap_mmio_read() and regmap_mmio_write() always
   return 0 unless there's an error enabling its clock.

Sounds good, I'll simplify places that use this.

>> +	if (ret)
>> +		return ret;
>> +
>> +	mdelay(2);
> 
> If the previous write to the PMU got stuck in the CPU cache, there is no
> guarantee that this delay of 2ms between write and clear will be enforced. So
> you should do a dummy read after write to ensure that the previous write has
> reached the PMU (or any device) and then clear the bits.

Wow, really?  I was aware of this being possible for I/O
writes but it seems like something regmap might handle.

I'll add a regmap_read() for the same offset and discard
the result *before* the delay.  I'll do the same for this:

         mdelay(PCIE_T_PVPERL_MS);

>> +	return regmap_clear_bits(k1->pmu, offset, mask);
>> +}
>> +
>> +/* Enable app clocks, deassert app resets */
>> +static int k1_pcie_app_enable(struct k1_pcie *k1)
>> +{
>> +	struct dw_pcie *pci = &k1->pci;
>> +	u32 clock_count;
>> +	u32 reset_count;
>> +	int ret;
>> +
>> +	clock_count = ARRAY_SIZE(pci->app_clks);
> 
> Just use ARRAY_SIZE() directly below.

OK.

>> +	ret = clk_bulk_prepare_enable(clock_count, pci->app_clks);
>> +	if (ret)
>> +		return ret;
>> +
>> +	reset_count = ARRAY_SIZE(pci->app_rsts);
> 
> Same here.

OK.

>> +	ret = reset_control_bulk_deassert(reset_count, pci->app_rsts);
>> +	if (ret)
>> +		goto err_disable_clks;
>> +
>> +	ret = reset_control_deassert(k1->global_reset);
>> +	if (ret)
>> +		goto err_assert_resets;
>> +
>> +	return 0;
>> +
>> +err_assert_resets:
>> +	(void)reset_control_bulk_assert(reset_count, pci->app_rsts);
> 
> Why void cast? Here and in other places.

I put void casts when I'm ignoring a returned value.
It's not necessary, but it reminds me that the function
returns a value, and at some point I decided to ignore it.
I can drop those if you find them offensive.

If you're suggesting I should issue a warning here if
an error is returned here, tell me that.

>> +err_disable_clks:
>> +	clk_bulk_disable_unprepare(clock_count, pci->app_clks);
>> +
>> +	return ret;
>> +}
>> +
>> +/* Disable app clocks, assert app resets */
>> +static void k1_pcie_app_disable(struct k1_pcie *k1)
>> +{
>> +	struct dw_pcie *pci = &k1->pci;
>> +	u32 count;
>> +	int ret;
>> +
>> +	(void)reset_control_assert(k1->global_reset);
>> +
>> +	count = ARRAY_SIZE(pci->app_rsts);
>> +	ret = reset_control_bulk_assert(count, pci->app_rsts);
>> +	if (ret)
>> +		dev_err(pci->dev, "app reset assert failed (%d)\n", ret);
>> +
>> +	count = ARRAY_SIZE(pci->app_clks);
>> +	clk_bulk_disable_unprepare(count, pci->app_clks);
>> +}
> 
> Same comments as k1_pcie_app_enable().

OK.

>> +static int k1_pcie_init(struct dw_pcie_rp *pp)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	struct k1_pcie *k1 = to_k1_pcie(pci);
>> +	u32 offset;
>> +	u32 mask;
>> +	int ret;
>> +
>> +	ret = k1_pcie_toggle_soft_reset(k1);
>> +	if (ret)
>> +		goto err_app_disable;
>> +
>> +	ret = k1_pcie_app_enable(k1);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = phy_init(k1->phy);
>> +	if (ret)
>> +		goto err_app_disable;
>> +
>> +	/* Set the PCI vendor and device ID */
>> +	dw_pcie_dbi_ro_wr_en(pci);
>> +	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, K1_PCIE_VENDOR_ID);
>> +	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, K1_PCIE_DEVICE_ID);
>> +	dw_pcie_dbi_ro_wr_dis(pci);
>> +
>> +	/*
>> +	 * Put the port in root complex mode, record that Vaux is present.
> 
> There is no 3.3Vaux supply present in the binding. So the supply is guaranteed
> to be present and enabled always by the platform?

Actually, I don't know, I'll ask.  Thank you for pointing this out.

>> +	 * Assert fundamental reset (drive PERST# low).
>> +	 */
>> +	offset = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
>> +	mask = DEVICE_TYPE_RC | PCIE_AUX_PWR_DET;
>> +	mask |= PCIE_RC_PERST;
>> +	ret = regmap_set_bits(k1->pmu, offset, mask);
>> +	if (ret)
>> +		goto err_phy_exit;
>> +
>> +	/* Wait the PCIe-mandated 100 msec before deasserting PERST# */
>> +	mdelay(100);
> 
> Same comment as k1_pcie_toggle_soft_reset() applies here.

Yes, understood.

>> +
>> +	ret = regmap_clear_bits(k1->pmu, offset, PCIE_RC_PERST);
>> +	if (!ret)
>> +		return 0;	/* Success! */
> 
> Please use common pattern to return success:
> 
> 	regmap_clear_bits()
> 
> 	return 0;

Now that I won't be checking return values this will come
naturally.  So yes, it will look like this, and there are
no other instances of this return pattern in this file.

> 
>> +
>> +err_phy_exit:
>> +	(void)phy_exit(k1->phy);
>> +err_app_disable:
>> +	k1_pcie_app_disable(k1);
>> +
>> +	return ret;
>> +}
>> +
>> +/* Silently ignore any errors */
>> +static void k1_pcie_deinit(struct dw_pcie_rp *pp)
>> +{
>> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>> +	struct k1_pcie *k1 = to_k1_pcie(pci);
>> +
>> +	/* Re-assert fundamental reset (drive PERST# low) */
> 
> s/Re-assert/Assert
> 
>> +	(void)regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>> +			      PCIE_RC_PERST);
>> +
>> +	(void)phy_exit(k1->phy);
>> +
>> +	k1_pcie_app_disable(k1);
>> +}
>> +
>> +static const struct dw_pcie_host_ops k1_pcie_host_ops = {
>> +	.init		= k1_pcie_init,
>> +	.deinit		= k1_pcie_deinit,
>> +};
>> +
>> +static void k1_pcie_enable_interrupts(struct k1_pcie *k1)
>> +{
>> +	void __iomem *virt;
>> +	u32 val;
>> +
>> +	/* Enable the MSI interrupt */
>> +	writel(MSI_CTRL_INT, k1->link + INTR_ENABLE);
> 
> If there are no ordering issues (I guess so), you can very well use the _relaxed
> variants throughout the driver.

The only writel() calls are related to updating
these interrupt bits.  I think you're right.

>> +	/* Top-level interrupt enable */
>> +	virt = k1->link + K1_PHY_AHB_IRQ_EN;
>> +	val = readl(virt);
>> +	val |= PCIE_INTERRUPT_EN;
>> +	writel(val, virt);
>> +}
>> +
>> +static void k1_pcie_disable_interrupts(struct k1_pcie *k1)
>> +{
>> +	void __iomem *virt;
>> +	u32 val;
>> +
>> +	virt = k1->link + K1_PHY_AHB_IRQ_EN;
>> +	val = readl(virt);
>> +	val &= ~PCIE_INTERRUPT_EN;
>> +	writel(val, virt);
>> +
>> +	writel(0, k1->link + INTR_ENABLE);
>> +}
>> +
>> +static bool k1_pcie_link_up(struct dw_pcie *pci)
>> +{
>> +	struct k1_pcie *k1 = to_k1_pcie(pci);
>> +	u32 val;
>> +
>> +	val = readl(k1->link + K1_PHY_AHB_LINK_STS);
>> +
>> +	return (val & RDLH_LINK_UP) && (val & SMLH_LINK_UP);
>> +}
>> +
>> +static int k1_pcie_start_link(struct dw_pcie *pci)
>> +{
>> +	struct k1_pcie *k1 = to_k1_pcie(pci);
>> +	int ret;
>> +
>> +	/* Stop holding the PHY in reset, and enable link training */
>> +	ret = regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>> +				 APP_HOLD_PHY_RST | LTSSM_EN, LTSSM_EN);
>> +	if (ret)
>> +		return ret;
>> +
>> +	k1_pcie_enable_interrupts(k1);
>> +
>> +	return 0;
>> +}
>> +
>> +static void k1_pcie_stop_link(struct dw_pcie *pci)
>> +{
>> +	struct k1_pcie *k1 = to_k1_pcie(pci);
>> +	int ret;
>> +
>> +	k1_pcie_disable_interrupts(k1);
>> +
>> +	/* Disable the link and hold the PHY in reset */
>> +	ret = regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>> +				 APP_HOLD_PHY_RST | LTSSM_EN, APP_HOLD_PHY_RST);
>> +	if (ret)
>> +		dev_err(pci->dev, "disable LTSSM failed (%d)\n", ret);
>> +}
>> +
>> +static const struct dw_pcie_ops k1_pcie_ops = {
>> +	.link_up	= k1_pcie_link_up,
>> +	.start_link	= k1_pcie_start_link,
>> +	.stop_link	= k1_pcie_stop_link,
>> +};
>> +
>> +static int k1_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct dw_pcie_rp *pp;
>> +	struct dw_pcie *pci;
>> +	struct k1_pcie *k1;
>> +	int ret;
>> +
>> +	k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
>> +	if (!k1)
>> +		return -ENOMEM;
>> +	dev_set_drvdata(dev, k1);
>> +
>> +	k1->pmu = syscon_regmap_lookup_by_phandle_args(dev_of_node(dev),
>> +						       "spacemit,syscon-pmu",
>> +						       1, &k1->pmu_off);
>> +	if (IS_ERR(k1->pmu))
>> +		return dev_err_probe(dev, PTR_ERR(k1->pmu),
>> +				     "lookup PMU regmap failed\n");
> 
> 'Failed to lookup \"PMU\" registers'
OK.

>> +
>> +	k1->link = devm_platform_ioremap_resource_byname(pdev, "link");
>> +	if (!k1->link)
>> +		return dev_err_probe(dev, -ENOMEM, "map link regs failed\n");
> 
> 'Failed to map \"link\" registers
> 
> Same for below error prints as well.

OK.

>> +
>> +	k1->global_reset = devm_reset_control_get_shared(dev, "global");
>> +	if (IS_ERR(k1->global_reset))
>> +		return dev_err_probe(dev, PTR_ERR(k1->global_reset),
>> +				     "get global reset failed\n");
>> +
>> +	/* Hold the PHY in reset until we start the link */
>> +	ret = regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CLK_RESET_CONTROL,
>> +			      APP_HOLD_PHY_RST);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "hold PHY in reset failed\n");
>> +
>> +	k1->phy = devm_phy_get(dev, NULL);
>> +	if (IS_ERR(k1->phy))
>> +		return dev_err_probe(dev, PTR_ERR(k1->phy), "get PHY failed\n");
>> +
>> +	pci = &k1->pci;
>> +	dw_pcie_cap_set(pci, REQ_RES);
>> +	pci->dev = dev;
>> +	pci->ops = &k1_pcie_ops;
>> +
>> +	pp = &pci->pp;
>> +	pp->num_vectors = MAX_MSI_IRQS;
> 
> I don't understand how MSI is implemented in this platform. If the controller
> relies on an external interrupt controller for handling MSIs (I think it does),
> then there should be either 'msi-parent' or 'msi-map' existed in the binding.

OK your comment earlier made me realize I had more to do here.
I'll work to improve that.

> But I see none, other than 'interrupts-extended'. So I don't know how MSI works
> at all.
> 
>> +	pp->ops = &k1_pcie_host_ops;
>> +
>> +	ret = dw_pcie_host_init(pp);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "host init failed\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static void k1_pcie_remove(struct platform_device *pdev)
>> +{
>> +	struct k1_pcie *k1 = dev_get_drvdata(&pdev->dev);
>> +	struct dw_pcie_rp *pp = &k1->pci.pp;
>> +
>> +	dw_pcie_host_deinit(pp);
>> +}
>> +
>> +static const struct of_device_id k1_pcie_of_match_table[] = {
>> +	{ .compatible = "spacemit,k1-pcie-rc", },
>> +	{ },
>> +};
>> +
>> +static struct platform_driver k1_pcie_driver = {
>> +	.probe	= k1_pcie_probe,
>> +	.remove	= k1_pcie_remove,
>> +	.driver = {
>> +		.name			= "k1-dwc-pcie",
>> +		.of_match_table		= k1_pcie_of_match_table,
>> +		.suppress_bind_attrs	= true,
> 
> No need of this flag for the reason I mentioned in the Kcofig change.

Because this doesn't implement an irqchip?

> You should also set,
> 
> 	.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> 
> to make use of the async probing of the devices during boot. This does save some
> boot time.

That's great to know, there is a noticeable delay during probe.

Thank you very much for your careful review, Mani.

					-Alex

> - Mani
>

On 9/19/25 5:10 PM, Alex Elder wrote:
>>> +static int k1_pcie_init(struct dw_pcie_rp *pp)
>>> +{
>>> +    struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>>> +    struct k1_pcie *k1 = to_k1_pcie(pci);
>>> +    u32 offset;
>>> +    u32 mask;
>>> +    int ret;
>>> +
>>> +    ret = k1_pcie_toggle_soft_reset(k1);
>>> +    if (ret)
>>> +        goto err_app_disable;
>>> +
>>> +    ret = k1_pcie_app_enable(k1);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = phy_init(k1->phy);
>>> +    if (ret)
>>> +        goto err_app_disable;
>>> +
>>> +    /* Set the PCI vendor and device ID */
>>> +    dw_pcie_dbi_ro_wr_en(pci);
>>> +    dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, K1_PCIE_VENDOR_ID);
>>> +    dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, K1_PCIE_DEVICE_ID);
>>> +    dw_pcie_dbi_ro_wr_dis(pci);
>>> +
>>> +    /*
>>> +     * Put the port in root complex mode, record that Vaux is present.
>>
>> There is no 3.3Vaux supply present in the binding. So the supply is 
>> guaranteed
>> to be present and enabled always by the platform?
> 
> Actually, I don't know, I'll ask.  Thank you for pointing this out.

On the Banana Pi BPI-F3 platform, this supply is always on.

There do exist other (SpacemiT K1-based) platforms that enable
this supply using a GPIO.  I am not able to test that now.

However I will add a property in the DT binding to indicate the
3.3v supply.  I see "vpcie3v3-supply" used as a property name
and unless someone suggests doing something else, that's what
I'll use.

					-Alex

On Wed, Aug 13, 2025 at 01:46:59PM -0500, Alex Elder wrote:
> Introduce a driver for the PCIe root complex found in the SpacemiT
> K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
> The driver supports three PCIe ports that operate at PCIe v2 transfer
> rates (5 GT/sec).  The first port uses a combo PHY, which may be
> configured for use for USB 3 instead.

I assume "PCIe v2" means what most people call "PCIe gen2", but the
spec encourages avoidance "genX" because it's ambiguous.

> +config PCIE_K1
> +	bool "SpacemiT K1 host mode PCIe controller"

Style of nearby entries is:

  "SpacemiT K1 PCIe controller (host mode)"

Please alphabetize by the company name ("SpacemiT") in the menu entry.

> +#define K1_PCIE_VENDOR_ID	0x201f
> +#define K1_PCIE_DEVICE_ID	0x0001

I assume this (0x201f) has been reserved by the PCI-SIG?  I don't see
it at:

  https://pcisig.com/membership/member-companies?combine=0x201f

Possibly rename this to PCI_VENDOR_ID_K1 (or maybe
PCI_VENDOR_ID_SPACEMIT?) to match the usual format in
include/linux/pci_ids.h, since it seems likely to end up there
eventually.

> +#define PCIE_RC_PERST			BIT(12)	/* 0: PERST# high; 1: low */

Maybe avoid confusion by describing as "1: assert PERST#" or similar?

> +	/* Wait the PCIe-mandated 100 msec before deasserting PERST# */
> +	mdelay(100);

I think this is PCIE_T_PVPERL_MS.  Comment is superfluous then.

> +static int k1_pcie_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct dw_pcie_rp *pp;
> +	struct dw_pcie *pci;
> +	struct k1_pcie *k1;
> +	int ret;
> +
> +	k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
> +	if (!k1)
> +		return -ENOMEM;
> +	dev_set_drvdata(dev, k1);

Most neighboring drivers use platform_set_drvdata().  Personally, I
would set drvdata after initializing k1 because I don't like to
advertise pointers to uninitialized things.

> +static void k1_pcie_remove(struct platform_device *pdev)
> +{
> +	struct k1_pcie *k1 = dev_get_drvdata(&pdev->dev);

Neighbors use platform_get_drvdata().

> +	struct dw_pcie_rp *pp = &k1->pci.pp;
> +
> +	dw_pcie_host_deinit(pp);
> +}

On 8/13/25 4:22 PM, Bjorn Helgaas wrote:
> On Wed, Aug 13, 2025 at 01:46:59PM -0500, Alex Elder wrote:
>> Introduce a driver for the PCIe root complex found in the SpacemiT
>> K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
>> The driver supports three PCIe ports that operate at PCIe v2 transfer
>> rates (5 GT/sec).  The first port uses a combo PHY, which may be
>> configured for use for USB 3 instead.
> 
> I assume "PCIe v2" means what most people call "PCIe gen2", but the
> spec encourages avoidance "genX" because it's ambiguous.

Yes, that's what I meant, but I did try to clarify with the
transfer rate.

>> +config PCIE_K1
>> +	bool "SpacemiT K1 host mode PCIe controller"
> 
> Style of nearby entries is:
> 
>    "SpacemiT K1 PCIe controller (host mode)"

OK I'll fix that.

> Please alphabetize by the company name ("SpacemiT") in the menu entry.

OK.

>> +#define K1_PCIE_VENDOR_ID	0x201f
>> +#define K1_PCIE_DEVICE_ID	0x0001
> 
> I assume this (0x201f) has been reserved by the PCI-SIG?  I don't see
> it at:
> 
>    https://pcisig.com/membership/member-companies?combine=0x201f

I hadn't even thought to check that.  I will follow up.  Thanks
for pointing this out.

> Possibly rename this to PCI_VENDOR_ID_K1 (or maybe
> PCI_VENDOR_ID_SPACEMIT?) to match the usual format in
> include/linux/pci_ids.h, since it seems likely to end up there
> eventually.

OK.

>> +#define PCIE_RC_PERST			BIT(12)	/* 0: PERST# high; 1: low */
> 
> Maybe avoid confusion by describing as "1: assert PERST#" or similar?

OK.  I struggled with how to express this to avoid confusion.
But I do think "assert PERST#" is better.

>> +	/* Wait the PCIe-mandated 100 msec before deasserting PERST# */
>> +	mdelay(100);
> 
> I think this is PCIE_T_PVPERL_MS.  Comment is superfluous then.

Excellent, thank you, I'll use that.

>> +static int k1_pcie_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct dw_pcie_rp *pp;
>> +	struct dw_pcie *pci;
>> +	struct k1_pcie *k1;
>> +	int ret;
>> +
>> +	k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
>> +	if (!k1)
>> +		return -ENOMEM;
>> +	dev_set_drvdata(dev, k1);
> 
> Most neighboring drivers use platform_set_drvdata().  Personally, I
> would set drvdata after initializing k1 because I don't like to
> advertise pointers to uninitialized things.

OK, I understand that and will do it the way you suggest.

>> +static void k1_pcie_remove(struct platform_device *pdev)
>> +{
>> +	struct k1_pcie *k1 = dev_get_drvdata(&pdev->dev);
> 
> Neighbors use platform_get_drvdata().

Yes, that goes with platform_set_drvdata().

>> +	struct dw_pcie_rp *pp = &k1->pci.pp;
>> +
>> +	dw_pcie_host_deinit(pp);
>> +}

Thank you very much for your review.

					-Alex

On 8/13/25 4:27 PM, Alex Elder wrote:
> On 8/13/25 4:22 PM, Bjorn Helgaas wrote:
>> On Wed, Aug 13, 2025 at 01:46:59PM -0500, Alex Elder wrote:
>>> Introduce a driver for the PCIe root complex found in the SpacemiT
>>> K1 SoC.  The hardware is derived from the Synopsys DesignWare PCIe IP.
>>> The driver supports three PCIe ports that operate at PCIe v2 transfer
>>> rates (5 GT/sec).  The first port uses a combo PHY, which may be
>>> configured for use for USB 3 instead.

I'm following up on a few things I said last month.

>> I assume "PCIe v2" means what most people call "PCIe gen2", but the
>> spec encourages avoidance "genX" because it's ambiguous.
> 
> Yes, that's what I meant, but I did try to clarify with the
> transfer rate.
> 
>>> +config PCIE_K1
>>> +    bool "SpacemiT K1 host mode PCIe controller"
>>
>> Style of nearby entries is:
>>
>>    "SpacemiT K1 PCIe controller (host mode)"
> 
> OK I'll fix that.
> 
>> Please alphabetize by the company name ("SpacemiT") in the menu entry.
> 
> OK.

I will be renaming the Kconfig option to be PCIE_SPACEMIT_K1
(instead of just PCIE_K1).  I'm renaming the source file to
be "pcie-spacemit-k1.c" instead of "pcie-k1.c" as well.

>>> +#define K1_PCIE_VENDOR_ID    0x201f
>>> +#define K1_PCIE_DEVICE_ID    0x0001
>>
>> I assume this (0x201f) has been reserved by the PCI-SIG?  I don't see
>> it at:
>>
>>    https://pcisig.com/membership/member-companies?combine=0x201f
> 
> I hadn't even thought to check that.  I will follow up.  Thanks
> for pointing this out.

I inquired yesterday about this, and was told that this
will be finalized next week.  I told them that the driver
would not be accepted upstream unless the vendor ID had
been properly reserved by PCI-SIG.

>> Possibly rename this to PCI_VENDOR_ID_K1 (or maybe
>> PCI_VENDOR_ID_SPACEMIT?) to match the usual format in
>> include/linux/pci_ids.h, since it seems likely to end up there
>> eventually.
> 
> OK.

I will use PCI_VENDOR_ID_SPACEMIT and PCI_DEVICE_ID_SPACEMIT_K1.

>>> +#define PCIE_RC_PERST            BIT(12)    /* 0: PERST# high; 1: 
>>> low */
>>
>> Maybe avoid confusion by describing as "1: assert PERST#" or similar?
> 
> OK.  I struggled with how to express this to avoid confusion.
> But I do think "assert PERST#" is better.
> 
>>> +    /* Wait the PCIe-mandated 100 msec before deasserting PERST# */
>>> +    mdelay(100);
>>
>> I think this is PCIE_T_PVPERL_MS.  Comment is superfluous then.
> 
> Excellent, thank you, I'll use that.
> 
>>> +static int k1_pcie_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct dw_pcie_rp *pp;
>>> +    struct dw_pcie *pci;
>>> +    struct k1_pcie *k1;
>>> +    int ret;
>>> +
>>> +    k1 = devm_kzalloc(dev, sizeof(*k1), GFP_KERNEL);
>>> +    if (!k1)
>>> +        return -ENOMEM;
>>> +    dev_set_drvdata(dev, k1);
>>
>> Most neighboring drivers use platform_set_drvdata().  Personally, I
>> would set drvdata after initializing k1 because I don't like to
>> advertise pointers to uninitialized things.
> 
> OK, I understand that and will do it the way you suggest.
> 
>>> +static void k1_pcie_remove(struct platform_device *pdev)
>>> +{
>>> +    struct k1_pcie *k1 = dev_get_drvdata(&pdev->dev);
>>
>> Neighbors use platform_get_drvdata().
> 
> Yes, that goes with platform_set_drvdata().
Actually, many of them use dev_get_drvdata().  And I think
that's why I used dev_set_drvdata() in the first place, to
match dev_get_drvdata().

But in any case, I'll switch to setting and getting platform
driver data.

					-Alex

> 
>>> +    struct dw_pcie_rp *pp = &k1->pci.pp;
>>> +
>>> +    dw_pcie_host_deinit(pp);
>>> +}
> 
> Thank you very much for your review.
> 
>                      -Alex