Re: [PATCH v5 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] Incorrect PCIe Gen3 Equalization Preset Request Vector assignment.
- [High] The driver accesses link MMIO registers before peripheral clocks are enabled, causing a bus fault.
- [Medium] The error path in k3_pcie_init incorrectly de-asserts PERST#, failing to safely reset the device.
- [Low] Use of undefined PHY bulk API functions causing build failure.
--

commit 63c719f7a87ffea307709340b4548f687247044d
Author: Inochi Amaoto <inochiama@gmail.com>
Subject: PCI: spacemit-k1: Add Spacemit K3 PCIe host controller support

This commit adds driver support for the PCIe controller on the Spacemit K3
SoC. The controller is similar to the Synopsys DesignWare PCIe IP but adds
support for an external MSI interrupt controller and multiple PHYs.

> diff --git a/drivers/pci/controller/dwc/pcie-spacemit-k1.c b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> index c610b367412aa..5031b688fdab0 100644
> --- a/drivers/pci/controller/dwc/pcie-spacemit-k1.c
> +++ b/drivers/pci/controller/dwc/pcie-spacemit-k1.c
[ ... ]
> @@ -303,6 +316,116 @@ static int k1_pcie_parse_port(struct k1_pcie *k1)
>  	return ret;
>  }
>  
> +static int k3_pcie_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct k1_pcie *k1 = to_k1_pcie(pci);
> +	u32 reset_ctrl = k1->pmu_off + PCIE_CLK_RESET_CONTROL;
> +	u32 val;
> +	int ret;
> +
> +	regmap_clear_bits(k1->pmu, reset_ctrl, LTSSM_EN);
> +
> +	k1_pcie_toggle_soft_reset(k1);
> +
> +	/* K3: Set IGNORE_PERSTN and drive PERSTN_OE high (assert reset) */
> +	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
> +			   PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE | PCIE_PERSTN_OUT,
> +			   PCIE_IGNORE_PERSTN | PCIE_PERSTN_OE);
> +
> +	ret = k1_pcie_enable_resources(k1);
> +	if (ret)
> +		goto failed_resources;
> +
> +	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 phy_bulk_init() exist in the mainline kernel? It seems this API might 
be missing, which could cause a build failure here.

> +	if (ret)
> +		goto failed_phy;
> +
> +	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]
Does this assignment set the correct equalization preset? BIT(7) evaluates 
to 128 (0x80), which when inserted into the 16-bit preset request vector 
field, assigns preset 8 to Lane 1 and 0 to all other lanes. Could this 
cause PCIe Gen3 link training to fail or use incorrect presets?

> +	dw_pcie_writel_dbi(pci, GEN3_EQ_CONTROL_OFF, val);
> +
> +	k1_pcie_set_device_id(k1);
> +
> +	/* Finally, as a workaround, disable ASPM L1 */
> +	k1_pcie_disable_aspm_l1(k1);
> +
> +	return 0;
> +
> +failed_phy:
> +	k1_pcie_disable_resources(k1);
> +failed_resources:
> +	regmap_update_bits(k1->pmu, k1->pmu_off + PCIE_CONTROL_LOGIC,
> +			   PCIE_PERSTN_OUT | PCIE_PERSTN_OE | PCIE_IGNORE_PERSTN,
> +			   PCIE_PERSTN_OUT | PCIE_PERSTN_OE);

[Severity: Medium]
In this error path, PCIE_PERSTN_OUT is set high, which appears to de-assert
PERST#. Should this be clearing PCIE_PERSTN_OUT instead, to ensure the 
endpoint device is safely held in reset after initialization fails?

> +
> +	return ret;
> +}
[ ... ]
> +static int k3_pcie_parse_port(struct k1_pcie *k1)
> +{
> +	u32 status0, status1, status2;
> +
> +	/* This register require a RAW for cleanup */
> +	status0 = readl_relaxed(k1->link + K3_PHY_AHB_IRQSTATUS_INTX);

[Severity: High]
Can this unclocked read cause a bus hang or synchronous external abort? 
k3_pcie_parse_port() is called unconditionally during k1_pcie_probe():

k1_pcie_probe()
  data->parse_port() -> k3_pcie_parse_port()
  ...
  dw_pcie_host_init()
    k3_pcie_init()
      k1_pcie_enable_resources()

It looks like the peripheral clocks are not enabled until 
k1_pcie_enable_resources() is called later in the host initialization. 
Reading the k1->link registers before the clocks are enabled might fault.

> +	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);
> +
> +	return k1_pcie_parse_port(k1);
> +}

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