On 9/15/25 3:09 AM, Manivannan Sadhasivam wrote:

On Wed, Aug 13, 2025 at 01:46:59PM GMT, Alex Elder wrote:

quoted

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 <redacted>
---
  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.

quoted

+	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.

quoted

  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.

quoted

+#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.

quoted

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

quoted

+	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.

quoted

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

Same here.

OK.

quoted

+	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.

quoted

+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.

quoted

+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.

quoted

+	 * 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.

quoted

+
+	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.

quoted

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

quoted

+	(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.

quoted

+	/* 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.

quoted

+
+	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.

quoted

+
+	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.

quoted

+	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