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

[sashiko-bot@kernel.org]

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.

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

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

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

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