Hi Bjorn Helgaas,
Thank you for the feedback. I will update the changes accordingly.
Rn Wed, Aug 20, 2025 at 10:54:44AM -0500, Bjorn Helgaas wrote:
> [EXTERNAL MAIL]
>
> On Wed, Aug 20, 2025 at 07:18:42PM +0800, Randolph Lin wrote:
> > Add driver support for DesignWare based PCIe controller in Andes
> > QiLai SoC. The driver only supports the Root Complex mode.
> >
> > 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"
> > + 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
> > + 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
>
> Are IOCP_ARCACHE and IOCP_AWCACHE supposed to be identical values with
> identical comments?
>
> > +#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;
>
> Sorry, we can't do this. We're removing .cpu_addr_fixup() because
> it's a workaround for defects in the DT description. See these
> commits, for example:
>
> befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
> b9812179f601 ("PCI: imx6: Remove imx_pcie_cpu_addr_fixup()")
> 07ae413e169d ("PCI: intel-gw: Remove intel_pcie_cpu_addr()")
>
I’m a bit confused about the following question:
After removing cpu_addr_fixup, should we use pci->parent_bus_offset to store the
offset value, or should pci->parent_bus_offset remain 0?
In the commit message:
befc86a0b354 ("PCI: dwc: Use parent_bus_offset to remove need for .cpu_addr_fixup()")
We know the parent_bus_offset, either computed from a DT reg property (the
offset is the CPU physical addr - the 'config'/'addr_space' address on the
parent bus) or from a .cpu_addr_fixup() (which may have used a host bridge
window offset).
We know that "the offset is the CPU physical addr - the 'config'/'addr_space'
address on the parent bus".
However, in dw_pcie_host_get_resources(), it passes pp->cfg0_base, which is
parsed from the device tree using "config", as the cpu_phys_addr parameter to
dw_pcie_parent_bus_offset(). It also passes "config" as the 2nd parameter to
dw_pcie_parent_bus_offset().
In dw_pcie_parent_bus_offset(), the 2nd parameter is used to get the index
from the devicetree "reg-names" field, and the result is used as the
'config'/'addr_space' address.
It seems that the same value is being obtained through a different method,
and the return value appears to be 0.
Could I be misunderstanding something?
> > +}
> > +
> > +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;
>
> I'm a little dubious about this because the knowledge about 1:1
> mapping feels like something we should know from devicetree. But I'm
> not an expert in this area.
>
> If you do need this, write it like this, which I think is slightly
> simpler than the if/else:
>
> if (...)
> return -EINVAL;
>
> return 0;
>
> > +}
> > +
> > +static bool qilai_pcie_link_up(struct dw_pcie *pci)
> > +{
> > + struct qilai_pcie *qlpci = to_qilai_pcie(pci);
>
> Suggest using "pcie" as the name for "struct qilai_pcie *" pointers
> because that's a common pattern in other drivers.
>
> > + 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 */
> > + 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;
>
> Doesn't seem worth a separate function to me. I see this is copied
> from bt1_pcie_create_data(), and kudos for paying attention to how
> other drivers do things. But I don't think it's really worth it
> there, either, and it's the only driver that does that.
>
> Most drivers do it directly in *_pcie_probe(), which also makes it
> possible to delay the platform_set_drvdata() until after qlpci is
> initialized (and also makes it obvious that you do the
> platform_set_drvdata() *twice* :)).
>
> > +}
> > +
> > +/*
> > + * Setup the Qilai PCIe IOCP (IO Coherence Port) Read/Write Behaviors to the
> > + * Write-Back, Read and Write Allocate mode.
> > + */
> > +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);
>
> Since you never clear anything in "val", it looks like this assumes
> the value you read from PCIE_LOGIC_COHERENCY_CONTROL3 contains zeroes
> where you insert the new values. Possibly use FIELD_MODIFY() instead?
>
> > + 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);
>
> Apparently you don't need dw_pcie_dbi_ro_wr_en() and
> dw_pcie_dbi_ro_wr_dis() around this? Or maybe this relies on the DWC
> core taking care of this, I dunno.
>
Maybe these are not the DBI registers.
I think the API is wrong, that's why you confused.
I will use readl() and writel() to instead of it.
> > +}
> > +
> > +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;
> > + 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);
> > +
> > + 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,
> > + },
> > +};
> > +
> > +module_platform_driver(qilai_pcie_driver);
> > +
> > +MODULE_AUTHOR("Randolph Lin <randolph@xxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Andes Qilai PCIe driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.34.1
> >
Sincerely,
Randolph Lin
