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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] k3_pcie_parse_port() accesses memory-mapped registers before the hardware clocks are enabled and resets are deasserted, causing a synchronous external abort.
- [High] k3_pcie_msi_host_init() misuses the .msi_init callback to configure general AXI cache coherency, which breaks DMA coherency if MSI is disabled.
- [Medium] The K3 controller reuses k1_pcie_deinit() for its .deinit callback, which uses an incompatible mechanism to assert PERST#, leaving the PCIe endpoint without a proper hard reset upon driver unbind or probe failure.
- [Low] Misleading copy-pasted comment regarding root complex mode and Vaux configuration.
--

--- Patch [6]: [PATCH v4 6/6] PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support ---
commit b7987f86bd7e30a14ad7fab3615f89d71ad70fed
Author: Inochi Amaoto <inochiama@gmail.com>

PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

This commit adds support for the PCIe controller on the Spacemit K3 SoC. It
introduces the K3-specific initialization sequence, operations, and interrupt
handling, while reusing the existing Spacemit K1 driver structure.

> diff --git a/drivers/pci/controller/dwc/pcie-spacemit-k1.c b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> index 31aac056b68ed..680acc93f5395 100644
> --- a/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> +++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c

[ ... ]

> @@ -340,6 +354,109 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)
>  	return ret;
>  }
>  
> +static int k3_pcie_init(struct dw_pcie_rp *pp)
> +{

[ ... ]

> +	/*
> +	 * Put the controller in root complex mode, and indicate that
> +	 * Vaux (3.3v) is present.
> +	 */
> +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
> +			PCIE_PERSTN_OUT | PCIE_PERSTN_OE);

[Severity: Low]
This isn't a bug, but does this comment match the code beneath it?

The code seems to only toggle PERST# via PCIE_PERSTN_OUT and PCIE_PERSTN_OE,
whereas the comment states it is putting the controller in root complex
mode and indicating Vaux is present.

> +
> +	val = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);

[ ... ]

> +static int k3_pcie_msi_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	u32 val;
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +
> +	val = dw_pcie_readl_dbi(pci, COHERENCY_CONTROL_3_OFF);
> +	val = u32_replace_bits(val, K3_CACHE_MSTR_AWCACHE_BEHAVIOR,
> +			       K3_CACHE_MSTR_AWCACHE_MODE);
> +	dw_pcie_writel_dbi(pci, COHERENCY_CONTROL_3_OFF, val);

[Severity: High]
Does configuring general AXI DMA coherency inside an MSI-specific hook break
DMA coherency if the system is booted with 'pci=nomsi'?

dw_pcie_host_init() only calls the .msi_init callback if pci_msi_enabled() is
true. If MSI is disabled, this initialization is skipped and standard DMA
coherency attributes might not be applied.

> +
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_host_ops k3_pcie_host_ops = {
> +	.init		= k3_pcie_init,
> +	.deinit		= k1_pcie_deinit,

[Severity: Medium]
Will this properly hard-reset the K3 PCIe endpoint during driver unbind or
probe failure?

The K3 controller reuses k1_pcie_deinit() here, which asserts PCIE_RC_PERST
in PCIE_CLK_RESET_CONTROL. However, k3_pcie_init() shows that K3 requires
PERST# to be asserted via PCIE_PERSTN_OE and PCIE_PERSTN_OUT in
PCIE_CONTROL_LOGIC.

> +	.msi_init	= k3_pcie_msi_host_init,
> +};

[ ... ]

> +static void k3_pcie_clear_irq_status(struct k1_pcie *k1,
> +				     u32 *status0, u32 *status1, u32 *status2)
> +{
> +	*status0 = readl_relaxed(k1->link + K3_PHY_AHB_IRQSTATUS_INTX);
> +	*status1 = readl_relaxed(k1->link + INTR_STATUS);
> +	*status2 = readl_relaxed(k1->link + K3_ADDR_INTR_STATUS1);
> +
> +	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);
> +}
> +
> +static int k3_pcie_parse_port(struct k1_pcie *k1)
> +{
> +	u32 status0, status1, status2;
> +
> +	k3_pcie_clear_irq_status(k1, &status0, &status1, &status2);

[Severity: High]
Can accessing these memory-mapped registers here cause a synchronous
external abort?

During probe, k1_pcie_probe() calls data->parse_port() before calling
dw_pcie_host_init(). However, the hardware clocks are enabled and resets are
deasserted later, inside dw_pcie_host_init(), when it invokes the .init
callback (k3_pcie_init()). Accessing the k1->link registers before they are
clocked may crash the system.

> +
> +	return k1_pcie_parse_port(k1);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709040027.958400-1-inochiama@gmail.com?part=6