Re: [PATCH v3 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] Unclocked MMIO access during probe triggers a synchronous bus abort.
- [High] Incorrect `.deinit` callback for K3 prevents PERST# assertion on teardown.
- [High] AXI cache coherency initialization is bypassed when MSIs are disabled.
- [Medium] Misleading comment and potential missing initialization for Root Complex mode.
--

commit 93fac4ee5b6c6ef1d779cae9b2ff5efed7a8fb39
Author: Inochi Amaoto <inochiama@gmail.com>

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

The PCIe controller on Spacemit K3 is almost a standard Synopsys
DesignWare PCIe IP with extra link and reset control. Unlike
the PCIe controller on K1, this controller supports external MSI
interrupt controller and can use multiple PHYs at the same time.

Add driver to support PCIe controller on Spacemit K3 PCIe.

> 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: Medium]
Will this omit initializing the controller into root complex mode?

Unlike k1_pcie_init(), this code block only drives the PERST# reset pins and
seems to skip setting DEVICE_TYPE_RC.

> +
> +	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);
> +
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +
> +	return 0;
> +}

[Severity: High]
Does this code miss initializing AXI cache coherency when MSIs are disabled?

Since k3_pcie_msi_host_init() is only called if pci_msi_enabled() is true,
booting with pci=nomsi would skip this callback, silently skipping general
cache coherency initialization for the controller.

> +
> +static const struct dw_pcie_host_ops k3_pcie_host_ops = {
> +	.init		= k3_pcie_init,
> +	.deinit		= k1_pcie_deinit,

[Severity: High]
Can reusing k1_pcie_deinit() prevent PERST# assertion on teardown?

During initialization, k3_pcie_init() bypasses the K1 mechanism by setting
PCIE_IGNORE_PERSTN in PCIE_CONTROL_LOGIC. Since k1_pcie_deinit() attempts to
toggle PCIE_RC_PERST, won't the K3 hardware ignore it?

> +	.msi_init	= k3_pcie_msi_host_init,
> +};
> +
> +static const struct dw_pcie_ops k3_pcie_ops = {
> +	.link_up	= k1_pcie_link_up,
> +	.start_link	= k1_pcie_start_link,
> +	.stop_link	= k1_pcie_stop_link,
> +};
> +
> +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);

[Severity: High]
Could this read unclocked MMIO memory and trigger a synchronous bus abort?

In k1_pcie_probe(), data->parse_port() is called before dw_pcie_host_init():

k1_pcie_probe() {
    ...
    ret = data->parse_port(k1);
    ...
    ret = dw_pcie_host_init(&k1->pci.pp);
}

This means k3_pcie_parse_port() and k3_pcie_clear_irq_status() are executed
before the required hardware clocks are enabled, accessing hardware registers
while their backing clocks are gated.

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