Message-ID: <21ad322f-5abe-4a97-9373-d027b846ee8c@riscstar.com>
Date: Fri, 19 Sep 2025 17:10:33 -0500
From: Alex Elder <elder@...cstar.com>
To: Manivannan Sadhasivam <mani@...nel.org>
Cc: lpieralisi@...nel.org, kwilczynski@...nel.org, robh@...nel.org,
 bhelgaas@...gle.com, krzk+dt@...nel.org, conor+dt@...nel.org,
 vkoul@...nel.org, kishon@...nel.org, dlan@...too.org,
 paul.walmsley@...ive.com, palmer@...belt.com, aou@...s.berkeley.edu,
 alex@...ti.fr, p.zabel@...gutronix.de, tglx@...utronix.de,
 johan+linaro@...nel.org, thippeswamy.havalige@....com, namcao@...utronix.de,
 mayank.rana@....qualcomm.com, shradha.t@...sung.com, inochiama@...il.com,
 quic_schintav@...cinc.com, fan.ni@...sung.com, devicetree@...r.kernel.org,
 linux-phy@...ts.infradead.org, linux-pci@...r.kernel.org,
 spacemit@...ts.linux.dev, linux-riscv@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/6] PCI: spacemit: introduce SpacemiT PCIe host driver

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@...cstar.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
> 

Powered by blists - more mailing lists