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

[Inochi Amaoto <inochiama@gmail.com>]

On Sun, May 17, 2026 at 03:16:20AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] MMIO registers are accessed before the controller's clocks are enabled, leading to a synchronous external abort or system crash.
> - [High] Incorrect use of `u32_replace_bits()` combined with bitwise OR (`|=`) fails to clear existing bits.
> - [High] Missing Root Complex mode configuration for the K3 controller due to a copy-paste error.
> - [Medium] A bitmask macro for a completely different field is incorrectly passed as the replacement value to `u32_replace_bits()`, writing unexpected values.
> - [Medium] Asymmetric teardown: `k1_pcie_deinit()` fails to assert reset on the K3 controller because it uses a reset line the controller is explicitly configured to ignore.
> --
> 
> commit 2826ac7fe9e0d0c0dd248a7fc412ce12f4d7b491
> Author: Inochi Amaoto <inochiama@gmail.com>
> 
> PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support
> 
> This commit adds host controller support for the Spacemit K3 PCIe IP. It
> introduces K3-specific initialization, PHY enablement, and MSI host setup,
> hooking these into the existing spacemit-k1 driver.
> 
> > diff --git a/drivers/pci/controller/dwc/pcie-spacemit-k1.c b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> > index 7f6f1df31cd81..7854d26220a98 100644
> > --- a/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> > +++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> [ ... ]
> > @@ -262,6 +278,152 @@ static const struct dw_pcie_ops k1_pcie_ops = {
> [ ... ]
> > +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);
> 
> Does this code configure the controller as a Root Complex? 
> 
> The comment mentions putting the controller in root complex mode, but the 
> register write only sets the PERSTN bits. In the K1 initialization, this is 
> done by setting DEVICE_TYPE_RC, which appears to be missing here.
> 

Unlike K1, K3 does not needs this.

> > +
> > +	val = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
> > +	val = u32_replace_bits(val, GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE,
> > +			       GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);
> 
> Is the replacement value intended to be GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE? 
> 
> This passes a bitmask macro for a completely different field into 
> u32_replace_bits(), which shifts the value into the target mask and writes an 
> unexpected value into the register.
> 

The GEN3_EQ_CONTROL_OFF_PHASE23_EXIT_MODE should be BIT(7),
it is a mistake.

> > +	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, val);
> [ ... ]
> > +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);
> 
> Will this clear the existing bits correctly? 
> 
> The u32_replace_bits() macro returns the fully modified 32-bit word. By using 
> the bitwise OR assignment (|=), the original bits from val are ORed with the 
> new bits, leaving any originally set bits as 1 instead of clearing them.
> 

Yes, it is wrong, I will fix it.

> > +	dw_pcie_writel_dbi(pci, COHERENCY_CONTROL_3_OFF, val);
> [ ... ]
> > +static const struct dw_pcie_host_ops k3_pcie_host_ops = {
> > +	.init		= k3_pcie_init,
> > +	.deinit		= k1_pcie_deinit,
> 
> Can k1_pcie_deinit() successfully assert the reset for the K3 controller? 
> 
> During initialization, k3_pcie_init() sets PCIE_IGNORE_PERSTN in 
> PCIE_CONTROL_LOGIC. If k1_pcie_deinit() attempts to assert reset using 
> PCIE_RC_PERST, the hardware might ignore it, leaving the device improperly 
> reset during teardown.
> 
> > +	.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)
> > +{
> [ ... ]
> > +	k1->phy_count = i;
> > +	if (k1->phy_count == 0)
> > +		return -EINVAL;
> > +
> > +	k3_pcie_clear_irq_status(k1, &status0, &status1, &status2);
> > +
> > +	return 0;
> > +}
> 
> Could accessing these MMIO registers before the clocks are enabled cause a
> synchronous external abort?
> 

No problems, these register is always ready.

> The probe function calls parse_port() before dw_pcie_host_init(). This means
> k3_pcie_clear_irq_status() performs direct MMIO reads and writes to the 
> k1->link registers before k1_pcie_enable_resources() is called later in 
> k3_pcie_init(). 
> 
> Accessing an unclocked peripheral can cause a bus stall on ARM/RISC-V 
> architectures.
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260517014841.254085-1-inochiama@gmail.com?part=5