Re: [PATCH v7 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 3 potential issue(s) to consider:
- [High] System crash on probe due to unclocked MMIO access
- [High] Logic error in equalization preset request configures non-existent lane
- [Low] Build failure due to missing PHY bulk APIs
--

commit 0e2b713f89ed74d3eac594b76efb0fd900f80b44
Author: Inochi Amaoto <inochiama@gmail.com>

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

This commit adds support for the Spacemit K3 PCIe host controller. It extends
the existing K1 driver to handle external MSI controllers and multiple PHYs
required by the K3 architecture.

> diff --git a/drivers/pci/controller/dwc/pcie-spacemit-k1.c b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> index ec59ccb710ec1..61b430f2294a7 100644
> --- a/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> +++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
[ ... ]
> @@ -315,6 +328,123 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)
[ ... ]
> +	regmap_set_bits(k1->pmu, reset_ctrl, PCIE_AUX_PWR_DET);
> +	regmap_clear_bits(k1->pmu, reset_ctrl, APP_HOLD_PHY_RST);
> +
> +	ret = phy_bulk_init(k1->phy_count, k1->phys);

[Severity: Low]
Does this code rely on a custom or out-of-tree API? The generic PHY subsystem
does not contain a phy_bulk_init() or devm_of_phy_bulk_get_all() function.
Calling these non-existent bulk PHY APIs causes an immediate build failure.

> +	if (ret)
> +		goto failed_phy_init;
[ ... ]
> +	msleep(PCIE_T_PVPERL_MS);
> +
> +	regmap_set_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
> +			PCIE_PERSTN_OUT | PCIE_PERSTN_OE);
> +
> +	val = dw_pcie_readl_dbi(pci, GEN3_EQ_CONTROL_OFF);
> +	val = u32_replace_bits(val, BIT(7),
> +			       GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC);

[Severity: High]
Is BIT(7) the correct value here? 

GEN3_EQ_CONTROL_OFF_PSET_REQ_VEC is a 16-bit vector where each bit corresponds
to a PCIe lane. Passing BIT(7) (128) into u32_replace_bits() sets the 8th bit
of the vector (Lane 7).

Since the K3 controller only has a maximum of 6 lanes, doesn't this issue an
invalid preset request for a non-existent lane and skip equalization for the
actual active lanes? This can potentially lead to link training failure or
incorrect functional behavior.

> +	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, val);
> +
> +	k1_pcie_set_device_id(k1);
[ ... ]
> +static const struct dw_pcie_host_ops k3_pcie_host_ops = {
> +	.init		= k3_pcie_init,
> +	.deinit		= k3_pcie_deinit,
> +	.msi_init	= k3_pcie_msi_host_init,
> +};
> +
> +static int k3_pcie_parse_port(struct k1_pcie *k1)
> +{
> +	u32 status0, status1, status2;
> +
> +	/* Clear the status registers */
> +	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);

[Severity: High]
Can accessing these MMIO registers crash the system during boot?

Looking at k1_pcie_probe(), it calls data->parse_port(k1) before
dw_pcie_host_init(). The application clocks and resets are only enabled later
in the sequence when dw_pcie_host_init() calls k3_pcie_init() and then
k1_pcie_enable_resources().

Accessing these hardware registers unconditionally while the PCIe controller
is still unclocked and in reset causes a synchronous external abort.

> +
> +	writel_relaxed(status0, k1->link + K3_PHY_AHB_IRQSTATUS_INTX);

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