| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<PN0PR01MB10393134B6BD714C1F070F0BEFEC82@PN0PR01MB10393.INDPRD01.PROD.OUTLOOK.COM>
Date: Wed, 5 Mar 2025 07:57:14 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Inochi Amaoto <inochiama@...il.com>,
Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
Johan Hovold <johan+linaro@...nel.org>,
Shashank Babu Chinta Venkata <quic_schintav@...cinc.com>,
Niklas Cassel <cassel@...nel.org>
Cc: linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
sophgo@...ts.linux.dev, linux-kernel@...r.kernel.org,
linux-riscv@...ts.infradead.org, Yixun Lan <dlan@...too.org>,
Longbin Li <looong.bin@...il.com>
Subject: Re: [PATCH 2/2] PCI: sophgo-dwc: Add Sophgo SG2044 PCIe driver
On 2025/3/4 15:12, Inochi Amaoto wrote:
> Add support for DesignWare-based PCIe controller in SG2044 SoC.
>
> Signed-off-by: Inochi Amaoto <inochiama@...il.com>
> ---
> drivers/pci/controller/dwc/Kconfig | 10 +
> drivers/pci/controller/dwc/Makefile | 1 +
> drivers/pci/controller/dwc/pcie-dw-sophgo.c | 270 ++++++++++++++++++++
Will this driver work for all sophgo chips using dw's IP? If not, it is
recommended to rename it to "pcie-dw-sg2044.c".
> 3 files changed, 281 insertions(+)
> create mode 100644 drivers/pci/controller/dwc/pcie-dw-sophgo.c
>
> diff --git a/drivers/pci/controller/dwc/Kconfig b/drivers/pci/controller/dwc/Kconfig
> index b6d6778b0698..004c384e25ad 100644
> --- a/drivers/pci/controller/dwc/Kconfig
> +++ b/drivers/pci/controller/dwc/Kconfig
> @@ -381,6 +381,16 @@ config PCIE_UNIPHIER_EP
> Say Y here if you want PCIe endpoint controller support on
> UniPhier SoCs. This driver supports Pro5 SoC.
>
> +config PCIE_SOPHGO_DW
Similar to the above question, would it be better to change it to
"PCIE_SG2044_DW"?
> + bool "Sophgo DesignWare PCIe controller"
> + depends on ARCH_SOPHGO || COMPILE_TEST
> + depends on PCI_MSI
> + depends on OF
> + select PCIE_DW_HOST
> + help
> + Enables support for the DesignWare PCIe controller in the
> + Sophgo SoC.
> +
> config PCIE_SPEAR13XX
> bool "STMicroelectronics SPEAr PCIe controller"
> depends on ARCH_SPEAR13XX || COMPILE_TEST
> diff --git a/drivers/pci/controller/dwc/Makefile b/drivers/pci/controller/dwc/Makefile
> index a8308d9ea986..193150056dd3 100644
> --- a/drivers/pci/controller/dwc/Makefile
> +++ b/drivers/pci/controller/dwc/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_PCIE_QCOM_EP) += pcie-qcom-ep.o
> obj-$(CONFIG_PCIE_ARMADA_8K) += pcie-armada8k.o
> obj-$(CONFIG_PCIE_ARTPEC6) += pcie-artpec6.o
> obj-$(CONFIG_PCIE_ROCKCHIP_DW) += pcie-dw-rockchip.o
> +obj-$(CONFIG_PCIE_SOPHGO_DW) += pcie-dw-sophgo.o
> obj-$(CONFIG_PCIE_INTEL_GW) += pcie-intel-gw.o
> obj-$(CONFIG_PCIE_KEEMBAY) += pcie-keembay.o
> obj-$(CONFIG_PCIE_KIRIN) += pcie-kirin.o
> diff --git a/drivers/pci/controller/dwc/pcie-dw-sophgo.c b/drivers/pci/controller/dwc/pcie-dw-sophgo.c
> new file mode 100644
> index 000000000000..3ed7cfe0b361
> --- /dev/null
> +++ b/drivers/pci/controller/dwc/pcie-dw-sophgo.c
> @@ -0,0 +1,270 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Sophgo DesignWare based PCIe host controller driver
> + */
> +
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/platform_device.h>
> +
> +#include "pcie-designware.h"
> +
> +#define to_sophgo_pcie(x) dev_get_drvdata((x)->dev)
> +
> +#define PCIE_INT_SIGNAL 0xc48
> +#define PCIE_INT_EN 0xca0
> +
> +#define PCIE_INT_SIGNAL_INTX GENMASK(8, 5)
> +
> +#define PCIE_INT_EN_INTX GENMASK(4, 1)
> +#define PCIE_INT_EN_INT_MSI BIT(5)
> +
> +struct sophgo_pcie {
> + struct dw_pcie pci;
> + void __iomem *app_base;
> + struct clk_bulk_data *clks;
> + unsigned int clk_cnt;
> + struct irq_domain *irq_domain;
> +};
> +
A similar question is, can structure names and function names use
prefixes like "sg2044_"? This also makes it easier for other sophgo
products to reuse this driver file in the future.
> +static int sophgo_pcie_readl_app(struct sophgo_pcie *sophgo, u32 reg)
> +{
> + return readl_relaxed(sophgo->app_base + reg);
> +}
> +
> +static void sophgo_pcie_writel_app(struct sophgo_pcie *sophgo, u32 val, u32 reg)
> +{
> + writel_relaxed(val, sophgo->app_base + reg);
> +}
> +
> +static void sophgo_pcie_intx_handler(struct irq_desc *desc)
> +{
> + struct dw_pcie_rp *pp = irq_desc_get_handler_data(desc);
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct sophgo_pcie *sophgo = to_sophgo_pcie(pci);
> + unsigned long hwirq, reg;
> +
> + chained_irq_enter(chip, desc);
> +
> + reg = sophgo_pcie_readl_app(sophgo, PCIE_INT_SIGNAL);
> + reg = FIELD_GET(PCIE_INT_SIGNAL_INTX, reg);
> +
> + for_each_set_bit(hwirq, ®, PCI_NUM_INTX)
> + generic_handle_domain_irq(sophgo->irq_domain, hwirq);
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static void sophgo_intx_irq_mask(struct irq_data *d)
> +{
> + struct dw_pcie_rp *pp = irq_data_get_irq_chip_data(d);
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct sophgo_pcie *sophgo = to_sophgo_pcie(pci);
> + unsigned long flags;
> + u32 val;
> +
> + raw_spin_lock_irqsave(&pp->lock, flags);
> +
> + val = sophgo_pcie_readl_app(sophgo, PCIE_INT_EN);
> + val &= ~FIELD_PREP(PCIE_INT_EN_INTX, BIT(d->hwirq));
> + sophgo_pcie_writel_app(sophgo, val, PCIE_INT_EN);
> +
> + raw_spin_unlock_irqrestore(&pp->lock, flags);
> +};
> +
> +static void sophgo_intx_irq_unmask(struct irq_data *d)
> +{
> + struct dw_pcie_rp *pp = irq_data_get_irq_chip_data(d);
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct sophgo_pcie *sophgo = to_sophgo_pcie(pci);
> + unsigned long flags;
> + u32 val;
> +
> + raw_spin_lock_irqsave(&pp->lock, flags);
> +
> + val = sophgo_pcie_readl_app(sophgo, PCIE_INT_EN);
> + val |= FIELD_PREP(PCIE_INT_EN_INTX, BIT(d->hwirq));
> + sophgo_pcie_writel_app(sophgo, val, PCIE_INT_EN);
> +
> + raw_spin_unlock_irqrestore(&pp->lock, flags);
> +};
> +
> +static void sophgo_intx_irq_eoi(struct irq_data *d)
> +{
> +}
Why define this empty callback function? Please add a comment if it is
really needed.
> +
> +static struct irq_chip sophgo_intx_irq_chip = {
> + .name = "INTx",
"INTx" --> "SG2044 INTx"
> + .irq_mask = sophgo_intx_irq_mask,
> + .irq_unmask = sophgo_intx_irq_unmask,
> + .irq_eoi = sophgo_intx_irq_eoi,
> +};
> +
> +static int sophgo_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + irq_set_chip_and_handler(irq, &sophgo_intx_irq_chip, handle_fasteoi_irq);
> + irq_set_chip_data(irq, domain->host_data);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops intx_domain_ops = {
> + .map = sophgo_pcie_intx_map,
> +};
> +
> +static int sophgo_pcie_init_irq_domain(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct sophgo_pcie *sophgo = to_sophgo_pcie(pci);
"sophgo"? This variable name looks a bit strange, why not call it "pcie"
directly?
I have seen similar naming in other functions, please change it as well.
> + struct device *dev = sophgo->pci.dev;
> + struct fwnode_handle *intc;
> + int irq;
> +
> + intc = device_get_named_child_node(dev, "interrupt-controller");
> + if (!intc) {
> + dev_err(dev, "missing child interrupt-controller node\n");
> + return -ENODEV;
> + }
> +
> + irq = fwnode_irq_get(intc, 0);
> + if (irq < 0) {
> + dev_err(dev, "failed to get INTx irq number\n");
> + fwnode_handle_put(intc);
> + return irq;
> + }
> +
> + sophgo->irq_domain = irq_domain_create_linear(intc, PCI_NUM_INTX,
> + &intx_domain_ops, sophgo);
> + fwnode_handle_put(intc);
> + if (!sophgo->irq_domain) {
> + dev_err(dev, "failed to get a INTx irq domain\n");
> + return -EINVAL;
> + }
> +
> + return irq;
> +}
> +
> +static void sophgo_pcie_msi_enable(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct sophgo_pcie *sophgo = to_sophgo_pcie(pci);
> + unsigned long flags;
> + u32 val;
> +
> + raw_spin_lock_irqsave(&pp->lock, flags);
> +
> + val = sophgo_pcie_readl_app(sophgo, PCIE_INT_EN);
> + val |= PCIE_INT_EN_INT_MSI;
> + sophgo_pcie_writel_app(sophgo, val, PCIE_INT_EN);
> +
> + raw_spin_unlock_irqrestore(&pp->lock, flags);
> +}
> +
> +static int sophgo_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> + int irq;
> +
> + irq = sophgo_pcie_init_irq_domain(pp);
> + if (irq < 0)
> + return irq;
> +
> + irq_set_chained_handler_and_data(irq, sophgo_pcie_intx_handler,
> + pp);
> +
> + sophgo_pcie_msi_enable(pp);
> +
> + return 0;
> +}
> +
> +static const struct dw_pcie_host_ops sophgo_pcie_host_ops = {
> + .init = sophgo_pcie_host_init,
> +};
> +
> +static int sophgo_pcie_clk_init(struct sophgo_pcie *sophgo)
> +{
> + struct device *dev = sophgo->pci.dev;
> + int ret;
> +
> + ret = devm_clk_bulk_get_all_enabled(dev, &sophgo->clks);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "failed to get clocks\n");
> +
> + sophgo->clk_cnt = ret;
> +
> + return 0;
> +}
> +
> +static int sophgo_pcie_resource_get(struct platform_device *pdev,
> + struct sophgo_pcie *sophgo)
> +{
> + sophgo->app_base = devm_platform_ioremap_resource_byname(pdev, "app");
> + if (IS_ERR(sophgo->app_base))
> + return dev_err_probe(&pdev->dev, PTR_ERR(sophgo->app_base),
> + "failed to map app registers\n");
> +
> + return 0;
> +}
> +
> +static int sophgo_pcie_configure_rc(struct sophgo_pcie *sophgo)
> +{
> + struct dw_pcie_rp *pp;
> +
> + pp = &sophgo->pci.pp;
> + pp->ops = &sophgo_pcie_host_ops;
> +
> + return dw_pcie_host_init(pp);
> +}
> +
> +static int sophgo_pcie_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct sophgo_pcie *sophgo;
> + int ret;
> +
> + sophgo = devm_kzalloc(dev, sizeof(*sophgo), GFP_KERNEL);
> + if (!sophgo)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, sophgo);
> +
> + sophgo->pci.dev = dev;
> +
> + ret = sophgo_pcie_resource_get(pdev, sophgo);
> + if (ret)
> + return ret;
> +
> + ret = sophgo_pcie_clk_init(sophgo);
> + if (ret)
> + return ret;
> +
> + return sophgo_pcie_configure_rc(sophgo);
> +}
> +
> +static const struct of_device_id sophgo_pcie_of_match[] = {
> + { .compatible = "sophgo,sg2044-pcie" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, sophgo_pcie_acpi_match);
> +
> +static const struct acpi_device_id sophgo_pcie_acpi_match[] = {
> + { "SOPHO000", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, sophgo_pcie_acpi_match);
> +
> +static struct platform_driver sophgo_pcie_driver = {
> + .driver = {
> + .name = "sophgo-dw-pcie",
> + .of_match_table = sophgo_pcie_of_match,
> + .acpi_match_table = sophgo_pcie_acpi_match,
> + .suppress_bind_attrs = true,
> + },
> + .probe = sophgo_pcie_probe,
> +};
> +builtin_platform_driver(sophgo_pcie_driver);
Powered by blists - more mailing lists