Hi Manivannan,
On Mon, Sep 08, 2025 at 12:32:28PM +0530, Manivannan Sadhasivam wrote:
> [EXTERNAL MAIL]
>
> On Wed, Aug 20, 2025 at 07:18:42PM GMT, Randolph Lin wrote:
>
> On top of Bjorn's review...
>
> > Add driver support for DesignWare based PCIe controller in Andes
> > QiLai SoC. The driver only supports the Root Complex mode.
> >
>
> Add more info about the controller. Like data rate supported, lanes, DWC IP
> version, any quirks etc...
>
It's PCIe 4.0,
One Port is 4 lanes (at 0x80000000),
Two port is 2 lanes (at 0xA0000000 and 0xC0000000).
The result of dw_pcie_readl_dbi(pci, PCIE_VERSION_NUMBER) is 0x3539372a.
Should I write it as 5.97a (version from documenmt) or
597A (version macro in pcie-designware.h)?
> > Signed-off-by: Randolph Lin <randolph@xxxxxxxxxxxxx>
> > ---
> > drivers/pci/controller/dwc/Kconfig | 13 +
> > drivers/pci/controller/dwc/Makefile | 1 +
> > drivers/pci/controller/dwc/pcie-andes-qilai.c | 227 ++++++++++++++++++
> > 3 files changed, 241 insertions(+)
> > create mode 100644 drivers/pci/controller/dwc/pcie-andes-qilai.c
> >
> > diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> > index ff6b6d9e18ec..a9c5a43f648b 100644
> > --- a/drivers/pci/controller/dwc/Kconfig
> > +++ 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"
>
> No need to make it always built-in. You can make it as a loadable module, but
> prevent unloading during runtime (by using builtin_platform_driver) as it
> implements MSI controller. We do not allows irqchip controllers to be unloaded
> during runtime.
>
Ok.
> > + depends on OF && (RISCV || COMPILE_TEST)
> > + depends on PCI_MSI
> > + depends on ARCH_ANDES
> > + select PCIE_DW_HOST
> > + default y if ARCH_ANDES
> > + help
> > + Say Y here if you want to enable PCIe controller support on Andes
>
> Mention 'Root Complex' mode.
>
Ok.
> > + QiLai SoCs. The Andes QiLai SoCs PCIe controller is based on
> > + DesignWare IP and therefore the driver re-uses the DesignWare
> > + core functions to implement the driver.
> > +
> > config PCI_MESON
> > tristate "Amlogic Meson PCIe controller"
> > default m if ARCH_MESON
> > diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> > index 6919d27798d1..de9583cbd675 100644
> > --- a/drivers/pci/controller/dwc/Makefile
> > +++ b/drivers/pci/controller/dwc/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_PCIE_DW_HOST) += pcie-designware-host.o
> > obj-$(CONFIG_PCIE_DW_EP) += pcie-designware-ep.o
> > obj-$(CONFIG_PCIE_DW_PLAT) += pcie-designware-plat.o
> > obj-$(CONFIG_PCIE_AMD_MDB) += pcie-amd-mdb.o
> > +obj-$(CONFIG_PCIE_ANDES_QILAI) += pcie-andes-qilai.o
> > obj-$(CONFIG_PCIE_BT1) += pcie-bt1.o
> > obj-$(CONFIG_PCI_DRA7XX) += pci-dra7xx.o
> > obj-$(CONFIG_PCI_EXYNOS) += pci-exynos.o
> > diff --git a/drivers/pci/controller/dwc/pcie-andes-qilai.c b/drivers/pci/controller/dwc/pcie-andes-qilai.c
> > new file mode 100644
> > index 000000000000..dd06eee82cac
> > --- /dev/null
> > +++ b/drivers/pci/controller/dwc/pcie-andes-qilai.c
> > @@ -0,0 +1,227 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for the PCIe Controller in QiLai from Andes
> > + *
> > + * Copyright (C) 2025 Andes Technology Corporation
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/pci.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/types.h>
> > +
> > +#include "pcie-designware.h"
> > +
> > +#define PCIE_INTR_CONTROL1 0x15c
> > +#define PCIE_MSI_CTRL_INT_EN BIT(28)
> > +
> > +#define PCIE_LOGIC_COHERENCY_CONTROL3 0x8e8
> > +/* Write-Back, Read and Write Allocate */
> > +#define IOCP_ARCACHE 0xf
> > +/* Write-Back, Read and Write Allocate */
> > +#define IOCP_AWCACHE 0xf
> > +#define PCIE_CFG_MSTR_ARCACHE_MODE GENMASK(6, 3)
> > +#define PCIE_CFG_MSTR_AWCACHE_MODE GENMASK(14, 11)
> > +#define PCIE_CFG_MSTR_ARCACHE_VALUE GENMASK(22, 19)
> > +#define PCIE_CFG_MSTR_AWCACHE_VALUE GENMASK(30, 27)
> > +
> > +#define PCIE_GEN_CONTROL2 0x54
> > +#define PCIE_CFG_LTSSM_EN BIT(0)
> > +
> > +#define PCIE_REGS_PCIE_SII_PM_STATE 0xc0
> > +#define SMLH_LINK_UP BIT(6)
> > +#define RDLH_LINK_UP BIT(7)
> > +#define PCIE_REGS_PCIE_SII_LINK_UP (SMLH_LINK_UP | RDLH_LINK_UP)
> > +
> > +struct qilai_pcie {
> > + struct dw_pcie dw;
> > + struct platform_device *pdev;
> > + void __iomem *apb_base;
> > +};
> > +
> > +#define to_qilai_pcie(_dw) container_of(_dw, struct qilai_pcie, dw)
> > +
> > +static u64 qilai_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 cpu_addr)
> > +{
> > + struct dw_pcie_rp *pp = &pci->pp;
> > +
> > + return cpu_addr - pp->cfg0_base;
> > +}
> > +
> > +static u32 qilai_pcie_outbound_atu_check(struct dw_pcie *pci,
> > + const struct dw_pcie_ob_atu_cfg *atu,
> > + u64 *limit_addr)
> > +{
> > + u64 parent_bus_addr = atu->parent_bus_addr;
> > +
> > + *limit_addr = parent_bus_addr + atu->size - 1;
> > +
> > + /*
> > + * Addresses below 4 GB are not 1:1 mapped; therefore, range checks
> > + * only need to ensure addresses below 4 GB match pci->region_limit.
> > + */
> > + if (lower_32_bits(*limit_addr & ~pci->region_limit) !=
> > + lower_32_bits(parent_bus_addr & ~pci->region_limit) ||
> > + !IS_ALIGNED(parent_bus_addr, pci->region_align) ||
> > + !IS_ALIGNED(atu->pci_addr, pci->region_align) || !atu->size)
> > + return -EINVAL;
> > + else
> > + return 0;
> > +}
> > +
> > +static bool qilai_pcie_link_up(struct dw_pcie *pci)
> > +{
> > + struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> > + u32 val;
> > +
> > + /* Read smlh & rdlh link up by checking debug port */
> > + dw_pcie_read(qlpci->apb_base + PCIE_REGS_PCIE_SII_PM_STATE, 0x4, &val);
> > +
> > + return (val & PCIE_REGS_PCIE_SII_LINK_UP) == PCIE_REGS_PCIE_SII_LINK_UP;
> > +}
> > +
> > +static int qilai_pcie_start_link(struct dw_pcie *pci)
> > +{
> > + struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> > + u32 val;
> > +
> > + /* Do phy link up */
>
> Drop the comment.
>
Ok.
> > + dw_pcie_read(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, &val);
> > + val |= PCIE_CFG_LTSSM_EN;
> > + dw_pcie_write(qlpci->apb_base + PCIE_GEN_CONTROL2, 0x4, val);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dw_pcie_ops qilai_pcie_ops = {
> > + .cpu_addr_fixup = qilai_pcie_cpu_addr_fixup,
> > + .outbound_atu_check = qilai_pcie_outbound_atu_check,
> > + .link_up = qilai_pcie_link_up,
> > + .start_link = qilai_pcie_start_link,
> > +};
> > +
> > +static struct qilai_pcie *qilai_pcie_create_data(struct platform_device *pdev)
> > +{
> > + struct qilai_pcie *qlpci;
> > +
> > + qlpci = devm_kzalloc(&pdev->dev, sizeof(*qlpci), GFP_KERNEL);
> > + if (!qlpci)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + qlpci->pdev = pdev;
> > + platform_set_drvdata(pdev, qlpci);
> > +
> > + return qlpci;
> > +}
> > +
> > +/*
> > + * Setup the Qilai PCIe IOCP (IO Coherence Port) Read/Write Behaviors to the
> > + * Write-Back, Read and Write Allocate mode.
> > + */
>
> What caches does the IOCP target? Is it the shared system cache? Does it allow
> the PCIe transactions to snoop the cache?
>
The target is SoC last-level cache (L2 Cache), which serves as the system cache.
The IOCP hardware helps maintain cache monitoring, ensuring that the device can
snoop data from/to the cache. The PCIe interface is connected behind the IOCP.
In this case, the PCIe does not directly snoop the cache; it achieves the same
purpose through the IOCP hardware.
> > +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);
> > + val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_MODE, IOCP_ARCACHE);
> > + val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_MODE, IOCP_AWCACHE);
> > + val |= FIELD_PREP(PCIE_CFG_MSTR_ARCACHE_VALUE, IOCP_ARCACHE);
> > + val |= FIELD_PREP(PCIE_CFG_MSTR_AWCACHE_VALUE, IOCP_AWCACHE);
> > + dw_pcie_write(pci->dbi_base + PCIE_LOGIC_COHERENCY_CONTROL3,
> > + sizeof(val), val);
> > +
> > + dw_pcie_dbi_ro_wr_dis(pci);
> > +}
> > +
> > +static void qilai_pcie_enable_msi(struct qilai_pcie *qlpci)
> > +{
> > + u32 val;
> > +
> > + dw_pcie_read(qlpci->apb_base + PCIE_INTR_CONTROL1,
> > + sizeof(val), &val);
> > + val |= PCIE_MSI_CTRL_INT_EN;
> > + dw_pcie_write(qlpci->apb_base + PCIE_INTR_CONTROL1,
> > + sizeof(val), val);
> > +}
> > +
> > +static int qilai_pcie_host_init(struct dw_pcie_rp *pp)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct qilai_pcie *qlpci = to_qilai_pcie(pci);
> > +
> > + qilai_pcie_enable_msi(qlpci);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct dw_pcie_host_ops qilai_pcie_host_ops = {
> > + .init = qilai_pcie_host_init,
> > +};
> > +
> > +static int qilai_pcie_add_port(struct qilai_pcie *qlpci)
> > +{
> > + struct device *dev = &qlpci->pdev->dev;
> > + struct platform_device *pdev = qlpci->pdev;
> > + int ret;
> > +
> > + qlpci->dw.dev = dev;
> > + qlpci->dw.ops = &qilai_pcie_ops;
> > + qlpci->dw.pp.num_vectors = MAX_MSI_IRQS;
>
> Does your platform really supports 256 MSIs? You have defined only a single
> 'msi' SPI interrupt in binding. So that means, your platform can only support 32
> MSIs.
>
> So if that's the case, you can drop this assignment since DWC uses 32 MSIs by
> default.
>
It should use the default value, I will remove this assignment.
> > + qlpci->dw.pp.ops = &qilai_pcie_host_ops;
> > +
> > + dw_pcie_cap_set(&qlpci->dw, REQ_RES);
> > +
> > + qlpci->apb_base = devm_platform_ioremap_resource_byname(pdev, "apb");
> > + if (IS_ERR(qlpci->apb_base))
> > + return PTR_ERR(qlpci->apb_base);
> > +
> > + ret = dw_pcie_host_init(&qlpci->dw.pp);
> > + if (ret) {
> > + dev_err_probe(dev, ret, "Failed to initialize PCIe host\n");
> > + return ret;
> > + }
> > +
> > + qilai_pcie_iocp_cache_setup(&qlpci->dw.pp);
>
> Are you sure that you need to set it up after enumerating devices?
>
Yes, it must be set up.
If the IOCP cache attribute is not configured, the EP device will be treated as
a non-coherent device. For better performance and compatibility, it is therefore
treated as a coherent device. Of course, it also provides flexibility for users
to configure the attribute themselves. Here, we set it up with the commonly used
settings.
> > +
> > + return 0;
> > +}
> > +
> > +static int qilai_pcie_probe(struct platform_device *pdev)
> > +{
> > + struct qilai_pcie *qlpci;
> > +
> > + qlpci = qilai_pcie_create_data(pdev);
> > + if (IS_ERR(qlpci))
> > + return PTR_ERR(qlpci);
> > +
> > + platform_set_drvdata(pdev, qlpci);
> > +
> > + return qilai_pcie_add_port(qlpci);
> > +}
> > +
> > +static const struct of_device_id qilai_pcie_of_match[] = {
> > + { .compatible = "andestech,qilai-pcie" },
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, qilai_pcie_of_match);
> > +
> > +static struct platform_driver qilai_pcie_driver = {
> > + .probe = qilai_pcie_probe,
> > + .driver = {
> > + .name = "qilai-pcie",
> > + .of_match_table = qilai_pcie_of_match,
> > + },
>
> You should start using PROBE_PREFER_ASYNCHRONOUS.
>
Can I keep using the PROBE_DEFAULT_STRATEGY?
I will leave a comment noting that the tests only passed with
PROBE_DEFAULT_STRATEGY.
When using PROBE_PREFER_ASYNCHRONOUS, numerous errors were encountered in
the EP device probe flow.
> > +};
> > +
> > +module_platform_driver(qilai_pcie_driver);
>
> builtin_platform_driver() since this driver is not going to be removed.
>
ok.
> - Mani
>
> --
> மணிவண்ணன் சதாசிவம்
Sincerely,
Randolph Lin
