Re: [PATCH v3 4/5] PCI: andes: Add Andes QiLai SoC PCIe host driver support

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Sep 23, 2025 at 07:36:46PM +0800, Randolph Lin wrote:
> Add driver support for DesignWare based PCIe controller in Andes
> QiLai SoC. The driver only supports the Root Complex mode.

> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -49,6 +49,19 @@ config PCIE_AMD_MDB
>  	  DesignWare IP and therefore the driver re-uses the DesignWare
>  	  core functions to implement the driver.
>  
> +config PCIE_ANDES_QILAI
> +	bool "ANDES QiLai PCIe controller"
> +	depends on ARCH_ANDES || COMPILE_TEST
> +	depends on PCI_MSI
> +	select PCIE_DW_HOST
> +	help
> +	  Say Y here to enable PCIe controller support on Andes QiLai SoCs,
> +	  which operate in Root Complex mode. The Andes QiLai SoCs PCIe
> +	  controller is based on DesignWare IP (5.97a version) and therefore
> +	  the driver re-uses the DesignWare core functions to implement the
> +	  driver. The Andes QiLai SoC features three Root Complexes, each
> +	  operating on PCIe 4.0.

Sort these by vendor name:

  AMD MDB Versal2 PCIe controller
  Amlogic Meson PCIe controller
  ANDES QiLai PCIe controller
  Axis ARTPEC-6 PCIe controller (host mode)

>  config PCI_MESON
>  	tristate "Amlogic Meson PCIe controller"

> + * Refer to Table A4-5 (Memory type encoding) in the
> + * AMBA AXI and ACE Protocol Specification.
> + *
> + * The selected value corresponds to the Memory type field:
> + * "Write-back, Read and Write-allocate".
> + */
> +#define IOCP_ARCACHE				0b1111
> +#define IOCP_AWCACHE				0b1111

Deserves a note about why these values are identical.

> +struct qilai_pcie {
> +	struct dw_pcie pci;
> +	struct platform_device *pdev;

"pdev" appears to be set but never used; drop it if you don't need it.

> +/*
> + * Setup the Qilai PCIe IOCP (IO Coherence Port) Read/Write Behaviors to the
> + * Write-Back, Read and Write Allocate mode.

Add blank line or rewrap into single paragraph.

> + * The IOCP HW target is SoC last-level cache (L2 Cache), which serves as the
> + * system cache. The IOCP HW helps maintain cache monitoring, ensuring that
> + * the device can snoop data from/to the cache.
> + */
> +static void qilai_pcie_iocp_cache_setup(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	u32 val;
> +
> +	dw_pcie_dbi_ro_wr_en(pci);
> +
> +	dw_pcie_read(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> +		     sizeof(val), &val);
> +	FIELD_MODIFY(PCIE_CFG_MSTR_ARCACHE_MODE, &val, IOCP_ARCACHE);
> +	FIELD_MODIFY(PCIE_CFG_MSTR_AWCACHE_MODE, &val, IOCP_AWCACHE);
> +	FIELD_MODIFY(PCIE_CFG_MSTR_ARCACHE_VALUE, &val, IOCP_ARCACHE);
> +	FIELD_MODIFY(PCIE_CFG_MSTR_AWCACHE_VALUE, &val, IOCP_AWCACHE);
> +	dw_pcie_write(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> +		      sizeof(val), val);
> +
> +	dw_pcie_dbi_ro_wr_dis(pci);
> +}

> +static int qilai_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct qilai_pcie *pcie = to_qilai_pcie(pci);
> +
> +	qilai_pcie_enable_msi(pcie);
> +
> +	return 0;
> +}
> +
> +static const struct dw_pcie_host_ops qilai_pcie_host_ops = {
> +	.init = qilai_pcie_host_init,
> +};
> +
> +static int qilai_pcie_probe(struct platform_device *pdev)
> +{
> +	struct qilai_pcie *pcie;
> +	struct dw_pcie *pci;
> +	struct device *dev;
> +	int ret;
> +
> +	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	pcie->pdev = pdev;
> +	platform_set_drvdata(pdev, pcie);
> +
> +	pci = &pcie->pci;
> +	dev = &pcie->pdev->dev;
> +	pcie->pci.dev = dev;
> +	pcie->pci.ops = &qilai_pcie_ops;
> +	pcie->pci.pp.ops = &qilai_pcie_host_ops;
> +	pci->use_parent_dt_ranges = true;
> +
> +	dw_pcie_cap_set(&pcie->pci, REQ_RES);
> +
> +	pcie->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
> +	if (IS_ERR(pcie->apb_base))
> +		return PTR_ERR(pcie->apb_base);
> +
> +	ret = dw_pcie_host_init(&pcie->pci.pp);
> +	if (ret) {
> +		dev_err_probe(dev, ret, "Failed to initialize PCIe host\n");
> +		return ret;
> +	}
> +
> +	qilai_pcie_iocp_cache_setup(&pcie->pci.pp);

I don't think we should be doing anything after dw_pcie_host_init()
because by the time we get here, we've already enumerated downstream
devices and potentially bound drivers to them.

If you need things done in dw_pcie_host_init() before enumeration,
qilai_pcie_host_init() and similar hooks are possibilities.

> +	return 0;
> +}