Re: [PATCH 5/5] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

[Bjorn Helgaas <helgaas@kernel.org>]

On Sat, May 02, 2026 at 06:13:18PM +0800, Inochi Amaoto wrote:
> The PCIe controller on Spacemit K3 is almost a standard Synopsys
> Designware PCIe IP with extra link and reset control. Unlike
> the PCIe controller on K1, this controller supports external MSI
> interrupt controller and can use multiple phy at the same time.
> 
> Add driver to support PCIe controller on Spacemit K3 PCIe.
> 
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>

Sashiko had some good questions:
https://sashiko.dev/#/patchset/20260502101319.2364052-1-inochiama%40gmail.com

Looks like the CONFIG_PCIE_SPACEMIT_K1 menu item and help text
drivers/pci/controller/dwc/Kconfig should be updated to include K3.

The "CONFIG_PCIE_SPACEMIT_K1" name itself should stay the same.

s/Designware/DesignWare/, also in 4/5 commit log
s/phy/PHY/ here and other patches and subject lines
s/msi/MSI/ in 3/5 subject and commit log when it's a stand-alone word
s/pci:/PCI:/ in 4/5 subject to match history (and patch 3/5)

> +++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c

> +#define INTR_STATUS				0x0010
> +
>  #define INTR_ENABLE				0x0014
>  #define MSI_CTRL_INT			BIT(11)
> +#define RDLH_LINK_UP_INT		BIT(20)
> +
> +#define K3_PHY_AHB_IRQSTATUS_INTX		0x0008
> +
> +#define K3_PHY_AHB_IRQENABLE_SET_INTX		0x000c
> +#define LEG_EP_INTERRUPTS		(BIT(6) | BIT(7) | BIT(8) | BIT(9))

Would be nicer to use "INTX" rather than "LEG" here since we use
"INTX" in K3_PHY_AHB_IRQENABLE_SET_INTX, in the comments, etc.

> +#define K3_PHY_AHB_IRQENABLE_SET_MSI		0x0014
> +/* MSI defined as BIT(11) in existing INTR_ENABLE, reusing */
> +
> +#define K3_ADDR_INTR_STATUS1			0x0018
> +
> +#define K3_ADDR_INTR_ENABLE1			0x001C

You're using a mix of upper- and lower-case hex here.  Be consistent
and match the existing code.

Seems a little weird to have a mix of "IRQ" names (e.g.,
K1_PHY_AHB_IRQ_EN, K3_PHY_AHB_IRQSTATUS_INTX,
K3_PHY_AHB_IRQENABLE_SET_INTX) and "INTR" names (e.g., INTR_STATUS,
INTR_ENABLE, K3_ADDR_INTR_STATUS1, K3_ADDR_INTR_ENABLE1) when I think
they're really talking about the same concept.

And why do the new K3 names have "ADDR" in the middle when the
existing "INTR_ENABLE" names don't?  It's obvious these are addresses
(well, actually I think they're *offsets*, but no need to be that
detailed).

> +static int k3_pcie_init(struct dw_pcie_rp *pp)
> +{
> ...
> +	val = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
> +	val &= ~(0xffff << 8);
> +	val |= ((0x1 << 4) << 8);

Can you use FIELD_MODIFY and some #defines here?

> +	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, val);
> +
> +	/* Set the PCI vendor and device ID */

Superfluous comment since the code is obvious.

> +	dw_pcie_dbi_ro_wr_en(pci);
> +	dw_pcie_writew_dbi(pci, PCI_VENDOR_ID, PCI_VENDOR_ID_SPACEMIT);
> +	dw_pcie_writew_dbi(pci, PCI_DEVICE_ID, PCI_DEVICE_ID_SPACEMIT_K3);
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	/* Finally, as a workaround, disable ASPM L1 */

I guess this means a device erratum?  It advertises L1 but it doesn't
actually work?

> +	k1_pcie_disable_aspm_l1(k1);

> +static int k3_pcie_msi_host_init(struct dw_pcie_rp *pp)
> +{
> ...
> +	val = dw_pcie_readl_dbi(pci, COHERENCY_CONTROL_3_OFF);
> +	val |= (0xf << 11);

FIELD_MODIFY and some #defines here?

> +static int k3_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct k1_pcie *k1 = to_k1_pcie(pci);
> +	u32 val;
> +
> +	k1_pcie_start_link(pci);
> +
> +	/* Enable INTx */
> +	val = readl_relaxed(k1->link + K3_PHY_AHB_IRQENABLE_SET_INTX);
> +	val |= LEG_EP_INTERRUPTS;
> +	writel_relaxed(val, k1->link + K3_PHY_AHB_IRQENABLE_SET_INTX);
> +
> +	/* Enable MSI/MSIX specific to K3 */

s/MSIX/MSI-X/ to match spec usage.

> +	val = readl_relaxed(k1->link + K3_ADDR_INTR_ENABLE1);
> +	val |= (MSI_INT | MSIX_INT);
> +	writel_relaxed(val, k1->link + K3_ADDR_INTR_ENABLE1);

Generally speaking I think the interrupt setup belongs somewhere other
than .start_link().  Usually .start_link() only enables LTSSM.

> +	return 0;
> +}

> +static irqreturn_t k3_pcie_irq_thread(int irq, void *data)
> +{
> +	struct k1_pcie *k1 = data;
> +	struct dw_pcie_rp *pp = &k1->pci.pp;
> +	struct device *dev = k1->pci.dev;
> +	u32 status0, status1, status2;
> +
> +	k3_pcie_clear_irq_status(k1, &status0, &status1, &status2);
> +
> +	writel_relaxed(status0, k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
> +	writel_relaxed(status1, k1->link + INTR_STATUS);
> +	writel_relaxed(status2, k1->link + K3_ADDR_INTR_STATUS1);
> +
> +	if (FIELD_GET(RDLH_LINK_UP_INT, status1)) {
> +		msleep(PCIE_RESET_CONFIG_WAIT_MS);
> +		/* Rescan the bus to enumerate endpoint devices */
> +		pci_lock_rescan_remove();
> +		pci_rescan_bus(pp->bridge->bus);

This is the *only* driver that uses pci_rescan_bus() this way, which
automatically makes it suspicous.  Maybe it's the first hardware that
implements or is willing to use RDLH_LINK_UP_INT for this, but somehow
I doubt it.

> +		pci_unlock_rescan_remove();
> +	} else if (!status0 && !status1 && !status2)
> +		dev_WARN_ONCE(dev, true,
> +			      "Received unknown event. status0=0x%08x status1=0x%08x status2=0x%08x\n",
> +			      status0, status1, status2);
> +
> +	return IRQ_HANDLED;
> +}