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

[Inochi Amaoto <inochiama@gmail.com>]

On Fri, Jul 03, 2026 at 02:20:57AM +0000, sashiko-bot@kernel.org wrote:
> 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?
> 

Yes

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

No

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

The PCIe controller is broken as it does not support INTx in this driver.

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

The K3 has handled this properly. You should treat the K3 and K1
 totally differently.

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

No, it is possible to access.

> 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