Message ID | ddedd8f76f83fea2c6d3887132d2fe6f2a6a02c1.1736923025.git.unicorn_wang@outlook.com (mailing list archive) |
---|---|
State | Handled Elsewhere |
Headers | show |
Series | Add PCIe support to Sophgo SG2042 SoC | expand |
On Wed, Jan 15, 2025 at 03:06:57PM +0800, Chen Wang wrote: > From: Chen Wang <unicorn_wang@outlook.com> > > Add support for PCIe controller in SG2042 SoC. The controller > uses the Cadence PCIe core programmed by pcie-cadence*.c. The > PCIe controller will work in host mode only. > Please add more info about the IP. Like IP revision, PCIe Gen, lane count, etc... > Signed-off-by: Chen Wang <unicorn_wang@outlook.com> > --- > drivers/pci/controller/cadence/Kconfig | 13 + > drivers/pci/controller/cadence/Makefile | 1 + > drivers/pci/controller/cadence/pcie-sg2042.c | 528 +++++++++++++++++++ > 3 files changed, 542 insertions(+) > create mode 100644 drivers/pci/controller/cadence/pcie-sg2042.c > > diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig > index 8a0044bb3989..292eb2b20e9c 100644 > --- a/drivers/pci/controller/cadence/Kconfig > +++ b/drivers/pci/controller/cadence/Kconfig > @@ -42,6 +42,18 @@ config PCIE_CADENCE_PLAT_EP > endpoint mode. This PCIe controller may be embedded into many > different vendors SoCs. > > +config PCIE_SG2042 > + bool "Sophgo SG2042 PCIe controller (host mode)" > + depends on ARCH_SOPHGO || COMPILE_TEST > + depends on OF > + select IRQ_MSI_LIB > + select PCI_MSI > + select PCIE_CADENCE_HOST > + help > + Say Y here if you want to support the Sophgo SG2042 PCIe platform > + controller in host mode. Sophgo SG2042 PCIe controller uses Cadence > + PCIe core. > + > config PCI_J721E > bool > > @@ -67,4 +79,5 @@ config PCI_J721E_EP > Say Y here if you want to support the TI J721E PCIe platform > controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe > core. > + Spurious newline. > endmenu > diff --git a/drivers/pci/controller/cadence/Makefile b/drivers/pci/controller/cadence/Makefile > index 9bac5fb2f13d..4df4456d9539 100644 > --- a/drivers/pci/controller/cadence/Makefile > +++ b/drivers/pci/controller/cadence/Makefile > @@ -4,3 +4,4 @@ obj-$(CONFIG_PCIE_CADENCE_HOST) += pcie-cadence-host.o > obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep.o > obj-$(CONFIG_PCIE_CADENCE_PLAT) += pcie-cadence-plat.o > obj-$(CONFIG_PCI_J721E) += pci-j721e.o > +obj-$(CONFIG_PCIE_SG2042) += pcie-sg2042.o > diff --git a/drivers/pci/controller/cadence/pcie-sg2042.c b/drivers/pci/controller/cadence/pcie-sg2042.c > new file mode 100644 > index 000000000000..56797c2af755 > --- /dev/null > +++ b/drivers/pci/controller/cadence/pcie-sg2042.c > @@ -0,0 +1,528 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * pcie-sg2042 - PCIe controller driver for Sophgo SG2042 SoC > + * > + * Copyright (C) 2024 Sophgo Technology Inc. > + * Copyright (C) 2024 Chen Wang <unicorn_wang@outlook.com> > + */ > + > +#include <linux/bits.h> > +#include <linux/irq.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/irqdomain.h> > +#include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > +#include <linux/msi.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/of_pci.h> > +#include <linux/platform_device.h> > +#include <linux/pm_runtime.h> > +#include <linux/regmap.h> > + > +#include "../../../irqchip/irq-msi-lib.h" > + > +#include "pcie-cadence.h" > + > +/* > + * SG2042 PCIe controller supports two ways to report MSI: > + * > + * - Method A, the PCIe controller implements an MSI interrupt controller > + * inside, and connect to PLIC upward through one interrupt line. > + * Provides memory-mapped MSI address, and by programming the upper 32 > + * bits of the address to zero, it can be compatible with old PCIe devices > + * that only support 32-bit MSI address. > + * > + * - Method B, the PCIe controller connects to PLIC upward through an > + * independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI > + * controller provides multiple(up to 32) interrupt sources to PLIC. > + * Compared with the first method, the advantage is that the interrupt > + * source is expanded, but because for SG2042, the MSI address provided by > + * the MSI controller is fixed and only supports 64-bit address(> 2^32), > + * it is not compatible with old PCIe devices that only support 32-bit MSI > + * address. > + * > + * Method A & B can be configured in DTS, default is Method B. How to configure them? I can only see "sophgo,sg2042-msi" in the binding. > + */ > + > +#define MAX_MSI_IRQS 8 > +#define MAX_MSI_IRQS_PER_CTRL 1 > +#define MAX_MSI_CTRLS (MAX_MSI_IRQS / MAX_MSI_IRQS_PER_CTRL) > +#define MSI_DEF_NUM_VECTORS MAX_MSI_IRQS > +#define BYTE_NUM_PER_MSI_VEC 4 > + > +#define REG_CLEAR 0x0804 > +#define REG_STATUS 0x0810 > +#define REG_LINK0_MSI_ADDR_SIZE 0x085C > +#define REG_LINK1_MSI_ADDR_SIZE 0x080C > +#define REG_LINK0_MSI_ADDR_LOW 0x0860 > +#define REG_LINK0_MSI_ADDR_HIGH 0x0864 > +#define REG_LINK1_MSI_ADDR_LOW 0x0868 > +#define REG_LINK1_MSI_ADDR_HIGH 0x086C > + > +#define REG_CLEAR_LINK0_BIT 2 > +#define REG_CLEAR_LINK1_BIT 3 > +#define REG_STATUS_LINK0_BIT 2 > +#define REG_STATUS_LINK1_BIT 3 > + > +#define REG_LINK0_MSI_ADDR_SIZE_MASK GENMASK(15, 0) > +#define REG_LINK1_MSI_ADDR_SIZE_MASK GENMASK(31, 16) > + > +struct sg2042_pcie { > + struct cdns_pcie *cdns_pcie; > + > + struct regmap *syscon; > + > + u32 link_id; > + > + struct irq_domain *msi_domain; > + > + int msi_irq; > + > + dma_addr_t msi_phys; > + void *msi_virt; > + > + u32 num_applied_vecs; /* used to speed up ISR */ > + Get rid of the newline between struct members, please. > + raw_spinlock_t msi_lock; > + DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > +}; > + [...] > +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, > + struct device_node *msi_node) > +{ > + struct device *dev = pcie->cdns_pcie->dev; > + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node); > + struct irq_domain *parent_domain; > + int ret = 0; > + > + if (!of_property_read_bool(msi_node, "msi-controller")) > + return -ENODEV; > + > + ret = of_irq_get_byname(msi_node, "msi"); > + if (ret <= 0) { > + dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node); > + return ret; > + } > + pcie->msi_irq = ret; > + > + irq_set_chained_handler_and_data(pcie->msi_irq, > + sg2042_pcie_msi_chained_isr, pcie); > + > + parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS, > + &sg2042_pcie_msi_domain_ops, pcie); > + if (!parent_domain) { > + dev_err(dev, "%pfw: Failed to create IRQ domain\n", fwnode); > + return -ENOMEM; > + } > + irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS); > + The MSI controller is wired to PLIC isn't it? If so, why can't you use hierarchial MSI domain implementation as like other controller drivers? > + parent_domain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT; > + parent_domain->msi_parent_ops = &sg2042_pcie_msi_parent_ops; > + > + pcie->msi_domain = parent_domain; > + > + ret = sg2042_pcie_init_msi_data(pcie); > + if (ret) { > + dev_err(dev, "Failed to initialize MSI data!\n"); > + return ret; > + } > + > + return 0; > +} > + > +static void sg2042_pcie_free_msi(struct sg2042_pcie *pcie) > +{ > + struct device *dev = pcie->cdns_pcie->dev; > + > + if (pcie->msi_irq) > + irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL); > + > + if (pcie->msi_virt) > + dma_free_coherent(dev, BYTE_NUM_PER_MSI_VEC * MAX_MSI_IRQS, > + pcie->msi_virt, pcie->msi_phys); > +} > + > +/* > + * SG2042 only support 4-byte aligned access, so for the rootbus (i.e. to read > + * the Root Port itself, read32 is required. For non-rootbus (i.e. to read > + * the PCIe peripheral registers, supports 1/2/4 byte aligned access, so > + * directly using read should be fine. > + * > + * The same is true for write. > + */ > +static int sg2042_pcie_config_read(struct pci_bus *bus, unsigned int devfn, > + int where, int size, u32 *value) > +{ > + if (pci_is_root_bus(bus)) > + return pci_generic_config_read32(bus, devfn, where, size, > + value); > + > + return pci_generic_config_read(bus, devfn, where, size, value); > +} > + > +static int sg2042_pcie_config_write(struct pci_bus *bus, unsigned int devfn, > + int where, int size, u32 value) > +{ > + if (pci_is_root_bus(bus)) > + return pci_generic_config_write32(bus, devfn, where, size, > + value); > + > + return pci_generic_config_write(bus, devfn, where, size, value); > +} > + > +static struct pci_ops sg2042_pcie_host_ops = { > + .map_bus = cdns_pci_map_bus, > + .read = sg2042_pcie_config_read, > + .write = sg2042_pcie_config_write, > +}; > + > +/* Dummy ops which will be assigned to cdns_pcie.ops, which must be !NULL. */ Because cadence code driver doesn't check for !ops? Please fix it instead. And the fix is trivial. > +static const struct cdns_pcie_ops sg2042_cdns_pcie_ops = {}; > + > +static int sg2042_pcie_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + struct pci_host_bridge *bridge; > + struct device_node *np_syscon; > + struct device_node *msi_node; > + struct cdns_pcie *cdns_pcie; > + struct sg2042_pcie *pcie; > + struct cdns_pcie_rc *rc; > + struct regmap *syscon; > + int ret; > + > + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); > + if (!pcie) > + return -ENOMEM; > + > + bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc)); > + if (!bridge) { > + dev_err(dev, "Failed to alloc host bridge!\n"); Use dev_err_probe() throughout the probe function. > + return -ENOMEM; > + } > + > + bridge->ops = &sg2042_pcie_host_ops; > + > + rc = pci_host_bridge_priv(bridge); > + cdns_pcie = &rc->pcie; > + cdns_pcie->dev = dev; > + cdns_pcie->ops = &sg2042_cdns_pcie_ops; > + pcie->cdns_pcie = cdns_pcie; > + > + np_syscon = of_parse_phandle(np, "sophgo,syscon-pcie-ctrl", 0); > + if (!np_syscon) { > + dev_err(dev, "Failed to get syscon node\n"); > + return -ENOMEM; -ENODEV > + } > + syscon = syscon_node_to_regmap(np_syscon); You should drop the np reference count once you are done with it. > + if (IS_ERR(syscon)) { > + dev_err(dev, "Failed to get regmap for syscon\n"); > + return -ENOMEM; PTR_ERR(syscon) > + } > + pcie->syscon = syscon; > + > + if (of_property_read_u32(np, "sophgo,link-id", &pcie->link_id)) { > + dev_err(dev, "Unable to parse sophgo,link-id\n"); "Unable to parse \"sophgo,link-id\"\n" > + return -EINVAL; > + } > + > + platform_set_drvdata(pdev, pcie); > + > + pm_runtime_enable(dev); > + > + ret = pm_runtime_get_sync(dev); Use pm_runtime_resume_and_get(). > + if (ret < 0) { > + dev_err(dev, "pm_runtime_get_sync failed\n"); > + goto err_get_sync; > + } > + > + msi_node = of_parse_phandle(dev->of_node, "msi-parent", 0); > + if (!msi_node) { > + dev_err(dev, "Failed to get msi-parent!\n"); > + return -1; -ENODATA > + } > + > + if (of_device_is_compatible(msi_node, "sophgo,sg2042-pcie-msi")) { > + ret = sg2042_pcie_setup_msi(pcie, msi_node); Same as above. np leak here. - Mani
On 2025/1/19 20:23, Manivannan Sadhasivam wrote: > On Wed, Jan 15, 2025 at 03:06:57PM +0800, Chen Wang wrote: >> From: Chen Wang <unicorn_wang@outlook.com> >> >> Add support for PCIe controller in SG2042 SoC. The controller >> uses the Cadence PCIe core programmed by pcie-cadence*.c. The >> PCIe controller will work in host mode only. >> > Please add more info about the IP. Like IP revision, PCIe Gen, lane count, > etc... ok. >> Signed-off-by: Chen Wang <unicorn_wang@outlook.com> >> --- >> drivers/pci/controller/cadence/Kconfig | 13 + >> drivers/pci/controller/cadence/Makefile | 1 + >> drivers/pci/controller/cadence/pcie-sg2042.c | 528 +++++++++++++++++++ >> 3 files changed, 542 insertions(+) >> create mode 100644 drivers/pci/controller/cadence/pcie-sg2042.c >> >> diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig >> index 8a0044bb3989..292eb2b20e9c 100644 >> --- a/drivers/pci/controller/cadence/Kconfig >> +++ b/drivers/pci/controller/cadence/Kconfig >> @@ -42,6 +42,18 @@ config PCIE_CADENCE_PLAT_EP >> endpoint mode. This PCIe controller may be embedded into many >> different vendors SoCs. >> >> +config PCIE_SG2042 >> + bool "Sophgo SG2042 PCIe controller (host mode)" >> + depends on ARCH_SOPHGO || COMPILE_TEST >> + depends on OF >> + select IRQ_MSI_LIB >> + select PCI_MSI >> + select PCIE_CADENCE_HOST >> + help >> + Say Y here if you want to support the Sophgo SG2042 PCIe platform >> + controller in host mode. Sophgo SG2042 PCIe controller uses Cadence >> + PCIe core. >> + >> config PCI_J721E >> bool >> >> @@ -67,4 +79,5 @@ config PCI_J721E_EP >> Say Y here if you want to support the TI J721E PCIe platform >> controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe >> core. >> + > Spurious newline. oops, I will fix this. [......] >> +/* >> + * SG2042 PCIe controller supports two ways to report MSI: >> + * >> + * - Method A, the PCIe controller implements an MSI interrupt controller >> + * inside, and connect to PLIC upward through one interrupt line. >> + * Provides memory-mapped MSI address, and by programming the upper 32 >> + * bits of the address to zero, it can be compatible with old PCIe devices >> + * that only support 32-bit MSI address. >> + * >> + * - Method B, the PCIe controller connects to PLIC upward through an >> + * independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI >> + * controller provides multiple(up to 32) interrupt sources to PLIC. >> + * Compared with the first method, the advantage is that the interrupt >> + * source is expanded, but because for SG2042, the MSI address provided by >> + * the MSI controller is fixed and only supports 64-bit address(> 2^32), >> + * it is not compatible with old PCIe devices that only support 32-bit MSI >> + * address. >> + * >> + * Method A & B can be configured in DTS, default is Method B. > How to configure them? I can only see "sophgo,sg2042-msi" in the binding. The value of the msi-parent attribute is used in dts to distinguish them, for example: ```dts msi: msi-controller@7030010300 { ...... }; pcie_rc0: pcie@7060000000 { msi-parent = <&msi>; }; pcie_rc1: pcie@7062000000 { ...... msi-parent = <&msi_pcie>; msi_pcie: interrupt-controller { ...... }; }; ``` Which means: pcie_rc0 uses Method B pcie_rc1 uses Method A. [......] >> +struct sg2042_pcie { >> + struct cdns_pcie *cdns_pcie; >> + >> + struct regmap *syscon; >> + >> + u32 link_id; >> + >> + struct irq_domain *msi_domain; >> + >> + int msi_irq; >> + >> + dma_addr_t msi_phys; >> + void *msi_virt; >> + >> + u32 num_applied_vecs; /* used to speed up ISR */ >> + > Get rid of the newline between struct members, please. ok >> + raw_spinlock_t msi_lock; >> + DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); >> +}; >> + > [...] > >> +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, >> + struct device_node *msi_node) >> +{ >> + struct device *dev = pcie->cdns_pcie->dev; >> + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node); >> + struct irq_domain *parent_domain; >> + int ret = 0; >> + >> + if (!of_property_read_bool(msi_node, "msi-controller")) >> + return -ENODEV; >> + >> + ret = of_irq_get_byname(msi_node, "msi"); >> + if (ret <= 0) { >> + dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node); >> + return ret; >> + } >> + pcie->msi_irq = ret; >> + >> + irq_set_chained_handler_and_data(pcie->msi_irq, >> + sg2042_pcie_msi_chained_isr, pcie); >> + >> + parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS, >> + &sg2042_pcie_msi_domain_ops, pcie); >> + if (!parent_domain) { >> + dev_err(dev, "%pfw: Failed to create IRQ domain\n", fwnode); >> + return -ENOMEM; >> + } >> + irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS); >> + > The MSI controller is wired to PLIC isn't it? If so, why can't you use > hierarchial MSI domain implementation as like other controller drivers? The method used here is somewhat similar to dw_pcie_allocate_domains() in drivers/pci/controller/dwc/pcie-designware-host.c. This MSI controller is about Method A, the PCIe controller implements an MSI interrupt controller inside, and connect to PLIC upward through only ONE interrupt line. Because MSI to PLIC is multiple to one, I use linear mode here and use chained ISR to handle the interrupts. [......] >> +/* Dummy ops which will be assigned to cdns_pcie.ops, which must be !NULL. */ > Because cadence code driver doesn't check for !ops? Please fix it instead. And > the fix is trivial. ok, I will fix this for cadence code together in next version. [......] >> + bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc)); >> + if (!bridge) { >> + dev_err(dev, "Failed to alloc host bridge!\n"); > Use dev_err_probe() throughout the probe function. Got it. >> + return -ENOMEM; >> + } >> + >> + bridge->ops = &sg2042_pcie_host_ops; >> + >> + rc = pci_host_bridge_priv(bridge); >> + cdns_pcie = &rc->pcie; >> + cdns_pcie->dev = dev; >> + cdns_pcie->ops = &sg2042_cdns_pcie_ops; >> + pcie->cdns_pcie = cdns_pcie; >> + >> + np_syscon = of_parse_phandle(np, "sophgo,syscon-pcie-ctrl", 0); >> + if (!np_syscon) { >> + dev_err(dev, "Failed to get syscon node\n"); >> + return -ENOMEM; > -ENODEV Got. >> + } >> + syscon = syscon_node_to_regmap(np_syscon); > You should drop the np reference count once you are done with it. ok, I will double check this through the file. >> + if (IS_ERR(syscon)) { >> + dev_err(dev, "Failed to get regmap for syscon\n"); >> + return -ENOMEM; > PTR_ERR(syscon) yes. >> + } >> + pcie->syscon = syscon; >> + >> + if (of_property_read_u32(np, "sophgo,link-id", &pcie->link_id)) { >> + dev_err(dev, "Unable to parse sophgo,link-id\n"); > "Unable to parse \"sophgo,link-id\"\n" ok. >> + return -EINVAL; >> + } >> + >> + platform_set_drvdata(pdev, pcie); >> + >> + pm_runtime_enable(dev); >> + >> + ret = pm_runtime_get_sync(dev); > Use pm_runtime_resume_and_get(). Got it. >> + if (ret < 0) { >> + dev_err(dev, "pm_runtime_get_sync failed\n"); >> + goto err_get_sync; >> + } >> + >> + msi_node = of_parse_phandle(dev->of_node, "msi-parent", 0); >> + if (!msi_node) { >> + dev_err(dev, "Failed to get msi-parent!\n"); >> + return -1; > -ENODATA Thanks, this should be fixed. > >> + } >> + >> + if (of_device_is_compatible(msi_node, "sophgo,sg2042-pcie-msi")) { >> + ret = sg2042_pcie_setup_msi(pcie, msi_node); > Same as above. np leak here. ok, will check this through the file. > > - Mani Thanks, Chen
+ Marc (for the IRQCHIP implementation review) On Wed, Jan 22, 2025 at 09:28:12PM +0800, Chen Wang wrote: > > On 2025/1/19 20:23, Manivannan Sadhasivam wrote: > > On Wed, Jan 15, 2025 at 03:06:57PM +0800, Chen Wang wrote: > > > From: Chen Wang <unicorn_wang@outlook.com> > > > > > > Add support for PCIe controller in SG2042 SoC. The controller > > > uses the Cadence PCIe core programmed by pcie-cadence*.c. The > > > PCIe controller will work in host mode only. > > > > > Please add more info about the IP. Like IP revision, PCIe Gen, lane count, > > etc... > ok. > > > Signed-off-by: Chen Wang <unicorn_wang@outlook.com> > > > --- > > > drivers/pci/controller/cadence/Kconfig | 13 + > > > drivers/pci/controller/cadence/Makefile | 1 + > > > drivers/pci/controller/cadence/pcie-sg2042.c | 528 +++++++++++++++++++ > > > 3 files changed, 542 insertions(+) > > > create mode 100644 drivers/pci/controller/cadence/pcie-sg2042.c > > > > > > diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig > > > index 8a0044bb3989..292eb2b20e9c 100644 > > > --- a/drivers/pci/controller/cadence/Kconfig > > > +++ b/drivers/pci/controller/cadence/Kconfig > > > @@ -42,6 +42,18 @@ config PCIE_CADENCE_PLAT_EP > > > endpoint mode. This PCIe controller may be embedded into many > > > different vendors SoCs. > > > +config PCIE_SG2042 > > > + bool "Sophgo SG2042 PCIe controller (host mode)" > > > + depends on ARCH_SOPHGO || COMPILE_TEST > > > + depends on OF > > > + select IRQ_MSI_LIB > > > + select PCI_MSI > > > + select PCIE_CADENCE_HOST > > > + help > > > + Say Y here if you want to support the Sophgo SG2042 PCIe platform > > > + controller in host mode. Sophgo SG2042 PCIe controller uses Cadence > > > + PCIe core. > > > + > > > config PCI_J721E > > > bool > > > @@ -67,4 +79,5 @@ config PCI_J721E_EP > > > Say Y here if you want to support the TI J721E PCIe platform > > > controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe > > > core. > > > + > > Spurious newline. > > oops, I will fix this. > > [......] > > > > +/* > > > + * SG2042 PCIe controller supports two ways to report MSI: > > > + * > > > + * - Method A, the PCIe controller implements an MSI interrupt controller > > > + * inside, and connect to PLIC upward through one interrupt line. > > > + * Provides memory-mapped MSI address, and by programming the upper 32 > > > + * bits of the address to zero, it can be compatible with old PCIe devices > > > + * that only support 32-bit MSI address. > > > + * > > > + * - Method B, the PCIe controller connects to PLIC upward through an > > > + * independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI > > > + * controller provides multiple(up to 32) interrupt sources to PLIC. > > > + * Compared with the first method, the advantage is that the interrupt > > > + * source is expanded, but because for SG2042, the MSI address provided by > > > + * the MSI controller is fixed and only supports 64-bit address(> 2^32), > > > + * it is not compatible with old PCIe devices that only support 32-bit MSI > > > + * address. > > > + * > > > + * Method A & B can be configured in DTS, default is Method B. > > How to configure them? I can only see "sophgo,sg2042-msi" in the binding. > > > The value of the msi-parent attribute is used in dts to distinguish them, > for example: > > ```dts > > msi: msi-controller@7030010300 { > ...... > }; > > pcie_rc0: pcie@7060000000 { > msi-parent = <&msi>; > }; > > pcie_rc1: pcie@7062000000 { > ...... > msi-parent = <&msi_pcie>; > msi_pcie: interrupt-controller { > ...... > }; > }; > > ``` > > Which means: > > pcie_rc0 uses Method B > > pcie_rc1 uses Method A. > Ok. you mentioned 'default method' which is not accurate since the choice obviously depends on DT. Maybe you should say, 'commonly used method'? But both the binding and dts patches make use of in-built MSI controller only (method A). In general, for MSI implementations inside the PCIe IP, we don't usually add a dedicated devicetree node since the IP is the same. But in your reply to the my question on the bindings patch, you said it is a separate IP. I'm confused now. > [......] > > > > +struct sg2042_pcie { > > > + struct cdns_pcie *cdns_pcie; > > > + > > > + struct regmap *syscon; > > > + > > > + u32 link_id; > > > + > > > + struct irq_domain *msi_domain; > > > + > > > + int msi_irq; > > > + > > > + dma_addr_t msi_phys; > > > + void *msi_virt; > > > + > > > + u32 num_applied_vecs; /* used to speed up ISR */ > > > + > > Get rid of the newline between struct members, please. > ok > > > + raw_spinlock_t msi_lock; > > > + DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > > > +}; > > > + > > [...] > > > > > +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, > > > + struct device_node *msi_node) > > > +{ > > > + struct device *dev = pcie->cdns_pcie->dev; > > > + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node); > > > + struct irq_domain *parent_domain; > > > + int ret = 0; > > > + > > > + if (!of_property_read_bool(msi_node, "msi-controller")) > > > + return -ENODEV; > > > + > > > + ret = of_irq_get_byname(msi_node, "msi"); > > > + if (ret <= 0) { > > > + dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node); > > > + return ret; > > > + } > > > + pcie->msi_irq = ret; > > > + > > > + irq_set_chained_handler_and_data(pcie->msi_irq, > > > + sg2042_pcie_msi_chained_isr, pcie); > > > + > > > + parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS, > > > + &sg2042_pcie_msi_domain_ops, pcie); > > > + if (!parent_domain) { > > > + dev_err(dev, "%pfw: Failed to create IRQ domain\n", fwnode); > > > + return -ENOMEM; > > > + } > > > + irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS); > > > + > > The MSI controller is wired to PLIC isn't it? If so, why can't you use > > hierarchial MSI domain implementation as like other controller drivers? > > The method used here is somewhat similar to dw_pcie_allocate_domains() in > drivers/pci/controller/dwc/pcie-designware-host.c. This MSI controller is > about Method A, the PCIe controller implements an MSI interrupt controller > inside, and connect to PLIC upward through only ONE interrupt line. Because > MSI to PLIC is multiple to one, I use linear mode here and use chained ISR > to handle the interrupts. > Hmm, ok. I'm not an IRQCHIP expert, but I'll defer to Marc to review the IRQCHIP implementation part. - Mani
On Wed, Jan 15, 2025 at 03:06:57PM +0800, Chen Wang wrote: > From: Chen Wang <unicorn_wang@outlook.com> > > Add support for PCIe controller in SG2042 SoC. The controller > uses the Cadence PCIe core programmed by pcie-cadence*.c. The > PCIe controller will work in host mode only. > + * pcie-sg2042 - PCIe controller driver for Sophgo SG2042 SoC I'm guessing this is the first of a *family* of Sophgo SoCs, so "sg2042" looks like it might be a little too specific if there will be things like "sg3042" etc added in the future. > +#include "../../../irqchip/irq-msi-lib.h" I know you're using this path because you're relying on Marc's work in progress [1]. But I don't want to carry around an #include like this in drivers/pci while we're waiting for that, so I think you should use the existing published MSI model until after Marc's update is merged. Otherwise we might end up with this ugly path here and no real path to migrate to the published, supported one. [1] https://lore.kernel.org/linux-riscv/20241204124549.607054-2-maz@kernel.org/ > + * SG2042 PCIe controller supports two ways to report MSI: > + * > + * - Method A, the PCIe controller implements an MSI interrupt controller > + * inside, and connect to PLIC upward through one interrupt line. > + * Provides memory-mapped MSI address, and by programming the upper 32 > + * bits of the address to zero, it can be compatible with old PCIe devices > + * that only support 32-bit MSI address. > + * > + * - Method B, the PCIe controller connects to PLIC upward through an > + * independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI > + * controller provides multiple(up to 32) interrupt sources to PLIC. Maybe expand "PLIC" the first time? s/SOC/SoC/ to match previous uses, e.g., in commit log s/multiple(up to 32)/up to 32/ > + * Compared with the first method, the advantage is that the interrupt > + * source is expanded, but because for SG2042, the MSI address provided by > + * the MSI controller is fixed and only supports 64-bit address(> 2^32), > + * it is not compatible with old PCIe devices that only support 32-bit MSI > + * address. "Supporting 64-bit address" means supporting any address from 0 to 2^64 - 1, but I don't think that's what you mean here. I think what you mean here is that with Method B, the MSI address is fixed and it can only be above 4GB. > +#define REG_CLEAR_LINK0_BIT 2 > +#define REG_CLEAR_LINK1_BIT 3 > +#define REG_STATUS_LINK0_BIT 2 > +#define REG_STATUS_LINK1_BIT 3 > +static void sg2042_pcie_msi_clear_status(struct sg2042_pcie *pcie) > +{ > + u32 status, clr_msi_in_bit; > + > + if (pcie->link_id == 1) > + clr_msi_in_bit = BIT(REG_CLEAR_LINK1_BIT); > + else > + clr_msi_in_bit = BIT(REG_CLEAR_LINK0_BIT); Why not put the BIT() in the #defines for REG_CLEAR_LINK0_BIT, REG_STATUS_LINK0_BIT, ...? Then this code is slightly simpler, and you can use similar style in sg2042_pcie_msi_chained_isr() instead of shifting there. > + regmap_read(pcie->syscon, REG_CLEAR, &status); > + status |= clr_msi_in_bit; > + regmap_write(pcie->syscon, REG_CLEAR, status); > +static void sg2042_pcie_msi_irq_compose_msi_msg(struct irq_data *d, > + struct msi_msg *msg) > +{ > + struct sg2042_pcie *pcie = irq_data_get_irq_chip_data(d); > + struct device *dev = pcie->cdns_pcie->dev; > + > + msg->address_lo = lower_32_bits(pcie->msi_phys) + BYTE_NUM_PER_MSI_VEC * d->hwirq; > + msg->address_hi = upper_32_bits(pcie->msi_phys); This seems a little suspect because adding "BYTE_NUM_PER_MSI_VEC * d->hwirq" could cause overflow into the upper 32 bits. I think you should add first, then take the lower/upper 32 bits of the 64-bit result. > + if (d->hwirq > pcie->num_applied_vecs) > + pcie->num_applied_vecs = d->hwirq; "num_applied_vecs" is a bit of a misnomer; it's actually the *max* hwirq. > +static const struct irq_domain_ops sg2042_pcie_msi_domain_ops = { > + .alloc = sg2042_pcie_irq_domain_alloc, > + .free = sg2042_pcie_irq_domain_free, Mention "msi" in these function names, e.g., sg2042_pcie_msi_domain_alloc(). > +static int sg2042_pcie_init_msi_data(struct sg2042_pcie *pcie) > +{ > ... > + /* Program the MSI address and size */ > + if (pcie->link_id == 1) { > + regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_LOW, > + lower_32_bits(pcie->msi_phys)); > + regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_HIGH, > + upper_32_bits(pcie->msi_phys)); > + > + regmap_read(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, &value); > + value = (value & REG_LINK1_MSI_ADDR_SIZE_MASK) | MAX_MSI_IRQS; > + regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, value); > + } else { > + regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_LOW, > + lower_32_bits(pcie->msi_phys)); > + regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_HIGH, > + upper_32_bits(pcie->msi_phys)); > + > + regmap_read(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, &value); > + value = (value & REG_LINK0_MSI_ADDR_SIZE_MASK) | (MAX_MSI_IRQS << 16); > + regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, value); > + } Would be nicer to set temporaries to the link_id-dependent values (as you did elsewhere) so it's obvious that the code is identical, e.g., if (pcie->link_id == 1) { msi_addr = REG_LINK1_MSI_ADDR_LOW; msi_addr_size = REG_LINK1_MSI_ADDR_SIZE; msi_addr_size_mask = REG_LINK1_MSI_ADDR_SIZE; } else { ... } regmap_write(pcie->syscon, msi_addr, lower_32_bits(pcie->msi_phys)); regmap_write(pcie->syscon, msi_addr + 4, upper_32_bits(pcie->msi_phys)); ... > + > + return 0; > +} > + > +static irqreturn_t sg2042_pcie_msi_handle_irq(struct sg2042_pcie *pcie) Which driver are you using as a template for function names and code structure? There are probably a dozen different names for functions that iterate like this around a call to generic_handle_domain_irq(), but you've managed to come up with a new one. If you can pick a similar name to copy, it makes it easier to compare drivers and find and fix defects across them. > +{ > + u32 i, pos; > + unsigned long val; > + u32 status, num_vectors; > + irqreturn_t ret = IRQ_NONE; > + > + num_vectors = pcie->num_applied_vecs; > + for (i = 0; i <= num_vectors; i++) { > + status = readl((void *)(pcie->msi_virt + i * BYTE_NUM_PER_MSI_VEC)); > + if (!status) > + continue; > + > + ret = IRQ_HANDLED; > + val = status; I don't think you need both val and status. > + pos = 0; > + while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL, > + pos)) != MAX_MSI_IRQS_PER_CTRL) { Most drivers use for_each_set_bit() here. > + generic_handle_domain_irq(pcie->msi_domain, > + (i * MAX_MSI_IRQS_PER_CTRL) + > + pos); > + pos++; > + } > + writel(0, ((void *)(pcie->msi_virt) + i * BYTE_NUM_PER_MSI_VEC)); > + } > + return ret; > +} > +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, > + struct device_node *msi_node) > +{ > + struct device *dev = pcie->cdns_pcie->dev; > + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node); > + struct irq_domain *parent_domain; > + int ret = 0; Pointless initialization of "ret". > + if (!of_property_read_bool(msi_node, "msi-controller")) > + return -ENODEV; > + > + ret = of_irq_get_byname(msi_node, "msi"); > + if (ret <= 0) { > + dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node); > + return ret; > + } > + pcie->msi_irq = ret; > + > + irq_set_chained_handler_and_data(pcie->msi_irq, > + sg2042_pcie_msi_chained_isr, pcie); > + > + parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS, > + &sg2042_pcie_msi_domain_ops, pcie); Wrap to fit in 80 columns like 99% of the rest of this file. Bjorn
On Wed, 22 Jan 2025 17:34:51 +0000, Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > + Marc (for the IRQCHIP implementation review) > > On Wed, Jan 22, 2025 at 09:28:12PM +0800, Chen Wang wrote: > > > > > > +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, > > > > + struct device_node *msi_node) > > > > +{ > > > > + struct device *dev = pcie->cdns_pcie->dev; > > > > + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node); > > > > + struct irq_domain *parent_domain; > > > > + int ret = 0; > > > > + > > > > + if (!of_property_read_bool(msi_node, "msi-controller")) > > > > + return -ENODEV; > > > > + > > > > + ret = of_irq_get_byname(msi_node, "msi"); > > > > + if (ret <= 0) { > > > > + dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node); > > > > + return ret; > > > > + } > > > > + pcie->msi_irq = ret; > > > > + > > > > + irq_set_chained_handler_and_data(pcie->msi_irq, > > > > + sg2042_pcie_msi_chained_isr, pcie); > > > > + > > > > + parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS, > > > > + &sg2042_pcie_msi_domain_ops, pcie); > > > > + if (!parent_domain) { > > > > + dev_err(dev, "%pfw: Failed to create IRQ domain\n", fwnode); > > > > + return -ENOMEM; > > > > + } > > > > + irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS); > > > > + > > > The MSI controller is wired to PLIC isn't it? If so, why can't you use > > > hierarchial MSI domain implementation as like other controller drivers? > > > > The method used here is somewhat similar to dw_pcie_allocate_domains() in > > drivers/pci/controller/dwc/pcie-designware-host.c. This MSI controller is > > about Method A, the PCIe controller implements an MSI interrupt controller > > inside, and connect to PLIC upward through only ONE interrupt line. Because > > MSI to PLIC is multiple to one, I use linear mode here and use chained ISR > > to handle the interrupts. > > > > Hmm, ok. I'm not an IRQCHIP expert, but I'll defer to Marc to review the IRQCHIP > implementation part. I don't offer this service anymore, I'm afraid. As for the "I create my own non-hierarchical IRQ domain", this is something that happens for all completely mis-designed interrupt controllers, MSI or not, that multiplex interrupts. These implementations are stuck in the previous century, and seeing this on modern designs, for a "server SoC", is really pathetic. maybe you now understand why I don't offer this sort of reviewing service anymore. M.
On Thu, Jan 23, 2025 at 12:12:03PM +0000, Marc Zyngier wrote: > On Wed, 22 Jan 2025 17:34:51 +0000, > Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> wrote: > > > > + Marc (for the IRQCHIP implementation review) > > > > On Wed, Jan 22, 2025 at 09:28:12PM +0800, Chen Wang wrote: > > > > > > > > +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, > > > > > + struct device_node *msi_node) > > > > > +{ > > > > > + struct device *dev = pcie->cdns_pcie->dev; > > > > > + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node); > > > > > + struct irq_domain *parent_domain; > > > > > + int ret = 0; > > > > > + > > > > > + if (!of_property_read_bool(msi_node, "msi-controller")) > > > > > + return -ENODEV; > > > > > + > > > > > + ret = of_irq_get_byname(msi_node, "msi"); > > > > > + if (ret <= 0) { > > > > > + dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node); > > > > > + return ret; > > > > > + } > > > > > + pcie->msi_irq = ret; > > > > > + > > > > > + irq_set_chained_handler_and_data(pcie->msi_irq, > > > > > + sg2042_pcie_msi_chained_isr, pcie); > > > > > + > > > > > + parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS, > > > > > + &sg2042_pcie_msi_domain_ops, pcie); > > > > > + if (!parent_domain) { > > > > > + dev_err(dev, "%pfw: Failed to create IRQ domain\n", fwnode); > > > > > + return -ENOMEM; > > > > > + } > > > > > + irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS); > > > > > + > > > > The MSI controller is wired to PLIC isn't it? If so, why can't you use > > > > hierarchial MSI domain implementation as like other controller drivers? > > > > > > The method used here is somewhat similar to dw_pcie_allocate_domains() in > > > drivers/pci/controller/dwc/pcie-designware-host.c. This MSI controller is > > > about Method A, the PCIe controller implements an MSI interrupt controller > > > inside, and connect to PLIC upward through only ONE interrupt line. Because > > > MSI to PLIC is multiple to one, I use linear mode here and use chained ISR > > > to handle the interrupts. > > > > > > > Hmm, ok. I'm not an IRQCHIP expert, but I'll defer to Marc to review the IRQCHIP > > implementation part. > > I don't offer this service anymore, I'm afraid. > > As for the "I create my own non-hierarchical IRQ domain", this is > something that happens for all completely mis-designed interrupt > controllers, MSI or not, that multiplex interrupts. > > These implementations are stuck in the previous century, and seeing > this on modern designs, for a "server SoC", is really pathetic. > > maybe you now understand why I don't offer this sort of reviewing > service anymore. > Ok, I can understand your pain as a maintainer. I'll try my best to review these implementations as I have no other choice :) - Mani
On 2025/1/23 1:34, Manivannan Sadhasivam wrote: [......] >>>> +/* >>>> + * SG2042 PCIe controller supports two ways to report MSI: >>>> + * >>>> + * - Method A, the PCIe controller implements an MSI interrupt controller >>>> + * inside, and connect to PLIC upward through one interrupt line. >>>> + * Provides memory-mapped MSI address, and by programming the upper 32 >>>> + * bits of the address to zero, it can be compatible with old PCIe devices >>>> + * that only support 32-bit MSI address. >>>> + * >>>> + * - Method B, the PCIe controller connects to PLIC upward through an >>>> + * independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI >>>> + * controller provides multiple(up to 32) interrupt sources to PLIC. >>>> + * Compared with the first method, the advantage is that the interrupt >>>> + * source is expanded, but because for SG2042, the MSI address provided by >>>> + * the MSI controller is fixed and only supports 64-bit address(> 2^32), >>>> + * it is not compatible with old PCIe devices that only support 32-bit MSI >>>> + * address. >>>> + * >>>> + * Method A & B can be configured in DTS, default is Method B. >>> How to configure them? I can only see "sophgo,sg2042-msi" in the binding. >> >> The value of the msi-parent attribute is used in dts to distinguish them, >> for example: >> >> ```dts >> >> msi: msi-controller@7030010300 { >> ...... >> }; >> >> pcie_rc0: pcie@7060000000 { >> msi-parent = <&msi>; >> }; >> >> pcie_rc1: pcie@7062000000 { >> ...... >> msi-parent = <&msi_pcie>; >> msi_pcie: interrupt-controller { >> ...... >> }; >> }; >> >> ``` >> >> Which means: >> >> pcie_rc0 uses Method B >> >> pcie_rc1 uses Method A. >> > Ok. you mentioned 'default method' which is not accurate since the choice > obviously depends on DT. Maybe you should say, 'commonly used method'? But both > the binding and dts patches make use of in-built MSI controller only (method A). "commonly used method" looks ok to me. Binding example only shows the case for Method A, due to I think the writing of case for Method A covers the writing of case for Method B. DTS patches use both Method A and B. You can see patch 4 of this patchset, pcie_rc1 uses Method A, pcie_rc0 & pcie_rc2 use Method B. > In general, for MSI implementations inside the PCIe IP, we don't usually add a > dedicated devicetree node since the IP is the same. But in your reply to the my > question on the bindings patch, you said it is a separate IP. I'm confused now. I learned the writing of DTS from "brcm,iproc-pcie", see arch/arm/boot/dts/broadcom/bcm-cygnus.dtsi for example. Wouldn't it be clearer to embed an msi controller in topo? And regarding what you said, "we don't usually add a dedicated devicetree node", do you have any example I can refer to? Thanks, Chen [......]
On 2025/1/23 5:33, Bjorn Helgaas wrote: > On Wed, Jan 15, 2025 at 03:06:57PM +0800, Chen Wang wrote: >> From: Chen Wang <unicorn_wang@outlook.com> >> >> Add support for PCIe controller in SG2042 SoC. The controller >> uses the Cadence PCIe core programmed by pcie-cadence*.c. The >> PCIe controller will work in host mode only. >> + * pcie-sg2042 - PCIe controller driver for Sophgo SG2042 SoC > I'm guessing this is the first of a *family* of Sophgo SoCs, so > "sg2042" looks like it might be a little too specific if there will be > things like "sg3042" etc added in the future. As I know, SG2042 will be the only one SoC using Cadence IP from Sophgo. They have switched to other IP(DWC) later. >> +#include "../../../irqchip/irq-msi-lib.h" > I know you're using this path because you're relying on Marc's > work in progress [1]. > > But I don't want to carry around an #include like this in drivers/pci > while we're waiting for that, so I think you should use the existing > published MSI model until after Marc's update is merged. Otherwise we > might end up with this ugly path here and no real path to migrate to > the published, supported one. > > [1] https://lore.kernel.org/linux-riscv/20241204124549.607054-2-maz@kernel.org/ OK, I will switch back to use the existing published MSI model. >> + * SG2042 PCIe controller supports two ways to report MSI: >> + * >> + * - Method A, the PCIe controller implements an MSI interrupt controller >> + * inside, and connect to PLIC upward through one interrupt line. >> + * Provides memory-mapped MSI address, and by programming the upper 32 >> + * bits of the address to zero, it can be compatible with old PCIe devices >> + * that only support 32-bit MSI address. >> + * >> + * - Method B, the PCIe controller connects to PLIC upward through an >> + * independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI >> + * controller provides multiple(up to 32) interrupt sources to PLIC. > Maybe expand "PLIC" the first time? Sure. > > s/SOC/SoC/ to match previous uses, e.g., in commit log > s/multiple(up to 32)/up to 32/ ok >> + * Compared with the first method, the advantage is that the interrupt >> + * source is expanded, but because for SG2042, the MSI address provided by >> + * the MSI controller is fixed and only supports 64-bit address(> 2^32), >> + * it is not compatible with old PCIe devices that only support 32-bit MSI >> + * address. > "Supporting 64-bit address" means supporting any address from 0 to > 2^64 - 1, but I don't think that's what you mean here. > > I think what you mean here is that with Method B, the MSI address is > fixed and it can only be above 4GB. Yes, I will fix it. >> +#define REG_CLEAR_LINK0_BIT 2 >> +#define REG_CLEAR_LINK1_BIT 3 >> +#define REG_STATUS_LINK0_BIT 2 >> +#define REG_STATUS_LINK1_BIT 3 >> +static void sg2042_pcie_msi_clear_status(struct sg2042_pcie *pcie) >> +{ >> + u32 status, clr_msi_in_bit; >> + >> + if (pcie->link_id == 1) >> + clr_msi_in_bit = BIT(REG_CLEAR_LINK1_BIT); >> + else >> + clr_msi_in_bit = BIT(REG_CLEAR_LINK0_BIT); > Why not put the BIT() in the #defines for REG_CLEAR_LINK0_BIT, > REG_STATUS_LINK0_BIT, ...? Then this code is slightly simpler, and > you can use similar style in sg2042_pcie_msi_chained_isr() instead of > shifting there. Ok, I will check this out in new version. >> + regmap_read(pcie->syscon, REG_CLEAR, &status); >> + status |= clr_msi_in_bit; >> + regmap_write(pcie->syscon, REG_CLEAR, status); >> +static void sg2042_pcie_msi_irq_compose_msi_msg(struct irq_data *d, >> + struct msi_msg *msg) >> +{ >> + struct sg2042_pcie *pcie = irq_data_get_irq_chip_data(d); >> + struct device *dev = pcie->cdns_pcie->dev; >> + >> + msg->address_lo = lower_32_bits(pcie->msi_phys) + BYTE_NUM_PER_MSI_VEC * d->hwirq; >> + msg->address_hi = upper_32_bits(pcie->msi_phys); > This seems a little suspect because adding "BYTE_NUM_PER_MSI_VEC * > d->hwirq" could cause overflow into the upper 32 bits. I think you > should add first, then take the lower/upper 32 bits of the 64-bit > result. OK, I will check this out in new version. >> + if (d->hwirq > pcie->num_applied_vecs) >> + pcie->num_applied_vecs = d->hwirq; > "num_applied_vecs" is a bit of a misnomer; it's actually the *max* > hwirq. "max_applied_vecs"? > >> +static const struct irq_domain_ops sg2042_pcie_msi_domain_ops = { >> + .alloc = sg2042_pcie_irq_domain_alloc, >> + .free = sg2042_pcie_irq_domain_free, > Mention "msi" in these function names, e.g., > sg2042_pcie_msi_domain_alloc(). ok > >> +static int sg2042_pcie_init_msi_data(struct sg2042_pcie *pcie) >> +{ >> ... >> + /* Program the MSI address and size */ >> + if (pcie->link_id == 1) { >> + regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_LOW, >> + lower_32_bits(pcie->msi_phys)); >> + regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_HIGH, >> + upper_32_bits(pcie->msi_phys)); >> + >> + regmap_read(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, &value); >> + value = (value & REG_LINK1_MSI_ADDR_SIZE_MASK) | MAX_MSI_IRQS; >> + regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, value); >> + } else { >> + regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_LOW, >> + lower_32_bits(pcie->msi_phys)); >> + regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_HIGH, >> + upper_32_bits(pcie->msi_phys)); >> + >> + regmap_read(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, &value); >> + value = (value & REG_LINK0_MSI_ADDR_SIZE_MASK) | (MAX_MSI_IRQS << 16); >> + regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, value); >> + } > Would be nicer to set temporaries to the link_id-dependent values (as > you did elsewhere) so it's obvious that the code is identical, e.g., > > if (pcie->link_id == 1) { > msi_addr = REG_LINK1_MSI_ADDR_LOW; > msi_addr_size = REG_LINK1_MSI_ADDR_SIZE; > msi_addr_size_mask = REG_LINK1_MSI_ADDR_SIZE; > } else { > ... > } > > regmap_write(pcie->syscon, msi_addr, lower_32_bits(pcie->msi_phys)); > regmap_write(pcie->syscon, msi_addr + 4, upper_32_bits(pcie->msi_phys)); > ... Ok,I will check this out. >> + >> + return 0; >> +} >> + >> +static irqreturn_t sg2042_pcie_msi_handle_irq(struct sg2042_pcie *pcie) > Which driver are you using as a template for function names and code > structure? There are probably a dozen different names for functions > that iterate like this around a call to generic_handle_domain_irq(), > but you've managed to come up with a new one. If you can pick a > similar name to copy, it makes it easier to compare drivers and find > and fix defects across them. > >> +{ >> + u32 i, pos; >> + unsigned long val; >> + u32 status, num_vectors; >> + irqreturn_t ret = IRQ_NONE; >> + >> + num_vectors = pcie->num_applied_vecs; >> + for (i = 0; i <= num_vectors; i++) { >> + status = readl((void *)(pcie->msi_virt + i * BYTE_NUM_PER_MSI_VEC)); >> + if (!status) >> + continue; >> + >> + ret = IRQ_HANDLED; >> + val = status; > I don't think you need both val and status. Yes, I will fix this. > >> + pos = 0; >> + while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL, >> + pos)) != MAX_MSI_IRQS_PER_CTRL) { > Most drivers use for_each_set_bit() here. Ok, I will check this out. >> + generic_handle_domain_irq(pcie->msi_domain, >> + (i * MAX_MSI_IRQS_PER_CTRL) + >> + pos); >> + pos++; >> + } >> + writel(0, ((void *)(pcie->msi_virt) + i * BYTE_NUM_PER_MSI_VEC)); >> + } >> + return ret; >> +} >> +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, >> + struct device_node *msi_node) >> +{ >> + struct device *dev = pcie->cdns_pcie->dev; >> + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node); >> + struct irq_domain *parent_domain; >> + int ret = 0; > Pointless initialization of "ret". Yes, I will fix this. >> + if (!of_property_read_bool(msi_node, "msi-controller")) >> + return -ENODEV; >> + >> + ret = of_irq_get_byname(msi_node, "msi"); >> + if (ret <= 0) { >> + dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node); >> + return ret; >> + } >> + pcie->msi_irq = ret; >> + >> + irq_set_chained_handler_and_data(pcie->msi_irq, >> + sg2042_pcie_msi_chained_isr, pcie); >> + >> + parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS, >> + &sg2042_pcie_msi_domain_ops, pcie); > Wrap to fit in 80 columns like 99% of the rest of this file. Ok, I will check this out. Thanks, Chen > > Bjorn
diff --git a/drivers/pci/controller/cadence/Kconfig b/drivers/pci/controller/cadence/Kconfig index 8a0044bb3989..292eb2b20e9c 100644 --- a/drivers/pci/controller/cadence/Kconfig +++ b/drivers/pci/controller/cadence/Kconfig @@ -42,6 +42,18 @@ config PCIE_CADENCE_PLAT_EP endpoint mode. This PCIe controller may be embedded into many different vendors SoCs. +config PCIE_SG2042 + bool "Sophgo SG2042 PCIe controller (host mode)" + depends on ARCH_SOPHGO || COMPILE_TEST + depends on OF + select IRQ_MSI_LIB + select PCI_MSI + select PCIE_CADENCE_HOST + help + Say Y here if you want to support the Sophgo SG2042 PCIe platform + controller in host mode. Sophgo SG2042 PCIe controller uses Cadence + PCIe core. + config PCI_J721E bool @@ -67,4 +79,5 @@ config PCI_J721E_EP Say Y here if you want to support the TI J721E PCIe platform controller in endpoint mode. TI J721E PCIe controller uses Cadence PCIe core. + endmenu diff --git a/drivers/pci/controller/cadence/Makefile b/drivers/pci/controller/cadence/Makefile index 9bac5fb2f13d..4df4456d9539 100644 --- a/drivers/pci/controller/cadence/Makefile +++ b/drivers/pci/controller/cadence/Makefile @@ -4,3 +4,4 @@ obj-$(CONFIG_PCIE_CADENCE_HOST) += pcie-cadence-host.o obj-$(CONFIG_PCIE_CADENCE_EP) += pcie-cadence-ep.o obj-$(CONFIG_PCIE_CADENCE_PLAT) += pcie-cadence-plat.o obj-$(CONFIG_PCI_J721E) += pci-j721e.o +obj-$(CONFIG_PCIE_SG2042) += pcie-sg2042.o diff --git a/drivers/pci/controller/cadence/pcie-sg2042.c b/drivers/pci/controller/cadence/pcie-sg2042.c new file mode 100644 index 000000000000..56797c2af755 --- /dev/null +++ b/drivers/pci/controller/cadence/pcie-sg2042.c @@ -0,0 +1,528 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * pcie-sg2042 - PCIe controller driver for Sophgo SG2042 SoC + * + * Copyright (C) 2024 Sophgo Technology Inc. + * Copyright (C) 2024 Chen Wang <unicorn_wang@outlook.com> + */ + +#include <linux/bits.h> +#include <linux/irq.h> +#include <linux/irqchip/chained_irq.h> +#include <linux/irqdomain.h> +#include <linux/kernel.h> +#include <linux/mfd/syscon.h> +#include <linux/msi.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/of_pci.h> +#include <linux/platform_device.h> +#include <linux/pm_runtime.h> +#include <linux/regmap.h> + +#include "../../../irqchip/irq-msi-lib.h" + +#include "pcie-cadence.h" + +/* + * SG2042 PCIe controller supports two ways to report MSI: + * + * - Method A, the PCIe controller implements an MSI interrupt controller + * inside, and connect to PLIC upward through one interrupt line. + * Provides memory-mapped MSI address, and by programming the upper 32 + * bits of the address to zero, it can be compatible with old PCIe devices + * that only support 32-bit MSI address. + * + * - Method B, the PCIe controller connects to PLIC upward through an + * independent MSI controller "sophgo,sg2042-msi" on the SOC. The MSI + * controller provides multiple(up to 32) interrupt sources to PLIC. + * Compared with the first method, the advantage is that the interrupt + * source is expanded, but because for SG2042, the MSI address provided by + * the MSI controller is fixed and only supports 64-bit address(> 2^32), + * it is not compatible with old PCIe devices that only support 32-bit MSI + * address. + * + * Method A & B can be configured in DTS, default is Method B. + */ + +#define MAX_MSI_IRQS 8 +#define MAX_MSI_IRQS_PER_CTRL 1 +#define MAX_MSI_CTRLS (MAX_MSI_IRQS / MAX_MSI_IRQS_PER_CTRL) +#define MSI_DEF_NUM_VECTORS MAX_MSI_IRQS +#define BYTE_NUM_PER_MSI_VEC 4 + +#define REG_CLEAR 0x0804 +#define REG_STATUS 0x0810 +#define REG_LINK0_MSI_ADDR_SIZE 0x085C +#define REG_LINK1_MSI_ADDR_SIZE 0x080C +#define REG_LINK0_MSI_ADDR_LOW 0x0860 +#define REG_LINK0_MSI_ADDR_HIGH 0x0864 +#define REG_LINK1_MSI_ADDR_LOW 0x0868 +#define REG_LINK1_MSI_ADDR_HIGH 0x086C + +#define REG_CLEAR_LINK0_BIT 2 +#define REG_CLEAR_LINK1_BIT 3 +#define REG_STATUS_LINK0_BIT 2 +#define REG_STATUS_LINK1_BIT 3 + +#define REG_LINK0_MSI_ADDR_SIZE_MASK GENMASK(15, 0) +#define REG_LINK1_MSI_ADDR_SIZE_MASK GENMASK(31, 16) + +struct sg2042_pcie { + struct cdns_pcie *cdns_pcie; + + struct regmap *syscon; + + u32 link_id; + + struct irq_domain *msi_domain; + + int msi_irq; + + dma_addr_t msi_phys; + void *msi_virt; + + u32 num_applied_vecs; /* used to speed up ISR */ + + raw_spinlock_t msi_lock; + DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); +}; + +static void sg2042_pcie_msi_clear_status(struct sg2042_pcie *pcie) +{ + u32 status, clr_msi_in_bit; + + if (pcie->link_id == 1) + clr_msi_in_bit = BIT(REG_CLEAR_LINK1_BIT); + else + clr_msi_in_bit = BIT(REG_CLEAR_LINK0_BIT); + + regmap_read(pcie->syscon, REG_CLEAR, &status); + status |= clr_msi_in_bit; + regmap_write(pcie->syscon, REG_CLEAR, status); + + /* need write 0 to reset, hardware can not reset automatically */ + status &= ~clr_msi_in_bit; + regmap_write(pcie->syscon, REG_CLEAR, status); +} + +static int sg2042_pcie_msi_irq_set_affinity(struct irq_data *d, + const struct cpumask *mask, + bool force) +{ + if (d->parent_data) + return irq_chip_set_affinity_parent(d, mask, force); + + return -EINVAL; +} + +static void sg2042_pcie_msi_irq_compose_msi_msg(struct irq_data *d, + struct msi_msg *msg) +{ + struct sg2042_pcie *pcie = irq_data_get_irq_chip_data(d); + struct device *dev = pcie->cdns_pcie->dev; + + msg->address_lo = lower_32_bits(pcie->msi_phys) + BYTE_NUM_PER_MSI_VEC * d->hwirq; + msg->address_hi = upper_32_bits(pcie->msi_phys); + msg->data = 1; + + if (d->hwirq > pcie->num_applied_vecs) + pcie->num_applied_vecs = d->hwirq; + + dev_dbg(dev, "compose MSI msg hwirq[%ld] address_hi[%#x] address_lo[%#x]\n", + d->hwirq, msg->address_hi, msg->address_lo); +} + +static void sg2042_pcie_msi_irq_ack(struct irq_data *d) +{ + struct sg2042_pcie *pcie = irq_data_get_irq_chip_data(d); + + sg2042_pcie_msi_clear_status(pcie); +} + +static struct irq_chip sg2042_pcie_msi_bottom_chip = { + .name = "SG2042 PCIe PLIC-MSI translator", + .irq_ack = sg2042_pcie_msi_irq_ack, + .irq_compose_msi_msg = sg2042_pcie_msi_irq_compose_msi_msg, + .irq_set_affinity = sg2042_pcie_msi_irq_set_affinity, +}; + +static int sg2042_pcie_irq_domain_alloc(struct irq_domain *domain, + unsigned int virq, unsigned int nr_irqs, + void *args) +{ + struct sg2042_pcie *pcie = domain->host_data; + unsigned long flags; + u32 i; + int bit; + + raw_spin_lock_irqsave(&pcie->msi_lock, flags); + + bit = bitmap_find_free_region(pcie->msi_irq_in_use, MSI_DEF_NUM_VECTORS, + order_base_2(nr_irqs)); + + raw_spin_unlock_irqrestore(&pcie->msi_lock, flags); + + if (bit < 0) + return -ENOSPC; + + for (i = 0; i < nr_irqs; i++) + irq_domain_set_info(domain, virq + i, bit + i, + &sg2042_pcie_msi_bottom_chip, + pcie, handle_edge_irq, + NULL, NULL); + + return 0; +} + +static void sg2042_pcie_irq_domain_free(struct irq_domain *domain, + unsigned int virq, unsigned int nr_irqs) +{ + struct irq_data *d = irq_domain_get_irq_data(domain, virq); + struct sg2042_pcie *pcie = irq_data_get_irq_chip_data(d); + unsigned long flags; + + raw_spin_lock_irqsave(&pcie->msi_lock, flags); + + bitmap_release_region(pcie->msi_irq_in_use, d->hwirq, + order_base_2(nr_irqs)); + + raw_spin_unlock_irqrestore(&pcie->msi_lock, flags); +} + +static const struct irq_domain_ops sg2042_pcie_msi_domain_ops = { + .alloc = sg2042_pcie_irq_domain_alloc, + .free = sg2042_pcie_irq_domain_free, +}; + +static int sg2042_pcie_init_msi_data(struct sg2042_pcie *pcie) +{ + struct device *dev = pcie->cdns_pcie->dev; + u32 value; + int ret; + + raw_spin_lock_init(&pcie->msi_lock); + + /* + * Though the PCIe controller can address >32-bit address space, to + * facilitate endpoints that support only 32-bit MSI target address, + * the mask is set to 32-bit to make sure that MSI target address is + * always a 32-bit address + */ + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32)); + if (ret < 0) + return ret; + + pcie->msi_virt = dma_alloc_coherent(dev, BYTE_NUM_PER_MSI_VEC * MAX_MSI_IRQS, + &pcie->msi_phys, GFP_KERNEL); + if (!pcie->msi_virt) + return -ENOMEM; + + /* Program the MSI address and size */ + if (pcie->link_id == 1) { + regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_LOW, + lower_32_bits(pcie->msi_phys)); + regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_HIGH, + upper_32_bits(pcie->msi_phys)); + + regmap_read(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, &value); + value = (value & REG_LINK1_MSI_ADDR_SIZE_MASK) | MAX_MSI_IRQS; + regmap_write(pcie->syscon, REG_LINK1_MSI_ADDR_SIZE, value); + } else { + regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_LOW, + lower_32_bits(pcie->msi_phys)); + regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_HIGH, + upper_32_bits(pcie->msi_phys)); + + regmap_read(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, &value); + value = (value & REG_LINK0_MSI_ADDR_SIZE_MASK) | (MAX_MSI_IRQS << 16); + regmap_write(pcie->syscon, REG_LINK0_MSI_ADDR_SIZE, value); + } + + return 0; +} + +static irqreturn_t sg2042_pcie_msi_handle_irq(struct sg2042_pcie *pcie) +{ + u32 i, pos; + unsigned long val; + u32 status, num_vectors; + irqreturn_t ret = IRQ_NONE; + + num_vectors = pcie->num_applied_vecs; + for (i = 0; i <= num_vectors; i++) { + status = readl((void *)(pcie->msi_virt + i * BYTE_NUM_PER_MSI_VEC)); + if (!status) + continue; + + ret = IRQ_HANDLED; + val = status; + pos = 0; + while ((pos = find_next_bit(&val, MAX_MSI_IRQS_PER_CTRL, + pos)) != MAX_MSI_IRQS_PER_CTRL) { + generic_handle_domain_irq(pcie->msi_domain, + (i * MAX_MSI_IRQS_PER_CTRL) + + pos); + pos++; + } + writel(0, ((void *)(pcie->msi_virt) + i * BYTE_NUM_PER_MSI_VEC)); + } + return ret; +} + +static void sg2042_pcie_msi_chained_isr(struct irq_desc *desc) +{ + struct irq_chip *chip = irq_desc_get_chip(desc); + u32 status, st_msi_in_bit; + struct sg2042_pcie *pcie; + + chained_irq_enter(chip, desc); + + pcie = irq_desc_get_handler_data(desc); + if (pcie->link_id == 1) + st_msi_in_bit = REG_STATUS_LINK1_BIT; + else + st_msi_in_bit = REG_STATUS_LINK0_BIT; + + regmap_read(pcie->syscon, REG_STATUS, &status); + if ((status >> st_msi_in_bit) & 0x1) { + sg2042_pcie_msi_clear_status(pcie); + + sg2042_pcie_msi_handle_irq(pcie); + } + + chained_irq_exit(chip, desc); +} + +#define SG2042_PCIE_MSI_FLAGS_REQUIRED (MSI_FLAG_USE_DEF_DOM_OPS | \ + MSI_FLAG_USE_DEF_CHIP_OPS) + +#define SG2042_PCIE_MSI_FLAGS_SUPPORTED MSI_GENERIC_FLAGS_MASK + +static struct msi_parent_ops sg2042_pcie_msi_parent_ops = { + .required_flags = SG2042_PCIE_MSI_FLAGS_REQUIRED, + .supported_flags = SG2042_PCIE_MSI_FLAGS_SUPPORTED, + .bus_select_mask = MATCH_PCI_MSI, + .bus_select_token = DOMAIN_BUS_NEXUS, + .prefix = "SG2042-", + .init_dev_msi_info = msi_lib_init_dev_msi_info, +}; + +static int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie, + struct device_node *msi_node) +{ + struct device *dev = pcie->cdns_pcie->dev; + struct fwnode_handle *fwnode = of_node_to_fwnode(dev->of_node); + struct irq_domain *parent_domain; + int ret = 0; + + if (!of_property_read_bool(msi_node, "msi-controller")) + return -ENODEV; + + ret = of_irq_get_byname(msi_node, "msi"); + if (ret <= 0) { + dev_err(dev, "%pOF: failed to get MSI irq\n", msi_node); + return ret; + } + pcie->msi_irq = ret; + + irq_set_chained_handler_and_data(pcie->msi_irq, + sg2042_pcie_msi_chained_isr, pcie); + + parent_domain = irq_domain_create_linear(fwnode, MSI_DEF_NUM_VECTORS, + &sg2042_pcie_msi_domain_ops, pcie); + if (!parent_domain) { + dev_err(dev, "%pfw: Failed to create IRQ domain\n", fwnode); + return -ENOMEM; + } + irq_domain_update_bus_token(parent_domain, DOMAIN_BUS_NEXUS); + + parent_domain->flags |= IRQ_DOMAIN_FLAG_MSI_PARENT; + parent_domain->msi_parent_ops = &sg2042_pcie_msi_parent_ops; + + pcie->msi_domain = parent_domain; + + ret = sg2042_pcie_init_msi_data(pcie); + if (ret) { + dev_err(dev, "Failed to initialize MSI data!\n"); + return ret; + } + + return 0; +} + +static void sg2042_pcie_free_msi(struct sg2042_pcie *pcie) +{ + struct device *dev = pcie->cdns_pcie->dev; + + if (pcie->msi_irq) + irq_set_chained_handler_and_data(pcie->msi_irq, NULL, NULL); + + if (pcie->msi_virt) + dma_free_coherent(dev, BYTE_NUM_PER_MSI_VEC * MAX_MSI_IRQS, + pcie->msi_virt, pcie->msi_phys); +} + +/* + * SG2042 only support 4-byte aligned access, so for the rootbus (i.e. to read + * the Root Port itself, read32 is required. For non-rootbus (i.e. to read + * the PCIe peripheral registers, supports 1/2/4 byte aligned access, so + * directly using read should be fine. + * + * The same is true for write. + */ +static int sg2042_pcie_config_read(struct pci_bus *bus, unsigned int devfn, + int where, int size, u32 *value) +{ + if (pci_is_root_bus(bus)) + return pci_generic_config_read32(bus, devfn, where, size, + value); + + return pci_generic_config_read(bus, devfn, where, size, value); +} + +static int sg2042_pcie_config_write(struct pci_bus *bus, unsigned int devfn, + int where, int size, u32 value) +{ + if (pci_is_root_bus(bus)) + return pci_generic_config_write32(bus, devfn, where, size, + value); + + return pci_generic_config_write(bus, devfn, where, size, value); +} + +static struct pci_ops sg2042_pcie_host_ops = { + .map_bus = cdns_pci_map_bus, + .read = sg2042_pcie_config_read, + .write = sg2042_pcie_config_write, +}; + +/* Dummy ops which will be assigned to cdns_pcie.ops, which must be !NULL. */ +static const struct cdns_pcie_ops sg2042_cdns_pcie_ops = {}; + +static int sg2042_pcie_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + struct pci_host_bridge *bridge; + struct device_node *np_syscon; + struct device_node *msi_node; + struct cdns_pcie *cdns_pcie; + struct sg2042_pcie *pcie; + struct cdns_pcie_rc *rc; + struct regmap *syscon; + int ret; + + pcie = devm_kzalloc(dev, sizeof(*pcie), GFP_KERNEL); + if (!pcie) + return -ENOMEM; + + bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc)); + if (!bridge) { + dev_err(dev, "Failed to alloc host bridge!\n"); + return -ENOMEM; + } + + bridge->ops = &sg2042_pcie_host_ops; + + rc = pci_host_bridge_priv(bridge); + cdns_pcie = &rc->pcie; + cdns_pcie->dev = dev; + cdns_pcie->ops = &sg2042_cdns_pcie_ops; + pcie->cdns_pcie = cdns_pcie; + + np_syscon = of_parse_phandle(np, "sophgo,syscon-pcie-ctrl", 0); + if (!np_syscon) { + dev_err(dev, "Failed to get syscon node\n"); + return -ENOMEM; + } + syscon = syscon_node_to_regmap(np_syscon); + if (IS_ERR(syscon)) { + dev_err(dev, "Failed to get regmap for syscon\n"); + return -ENOMEM; + } + pcie->syscon = syscon; + + if (of_property_read_u32(np, "sophgo,link-id", &pcie->link_id)) { + dev_err(dev, "Unable to parse sophgo,link-id\n"); + return -EINVAL; + } + + platform_set_drvdata(pdev, pcie); + + pm_runtime_enable(dev); + + ret = pm_runtime_get_sync(dev); + if (ret < 0) { + dev_err(dev, "pm_runtime_get_sync failed\n"); + goto err_get_sync; + } + + msi_node = of_parse_phandle(dev->of_node, "msi-parent", 0); + if (!msi_node) { + dev_err(dev, "Failed to get msi-parent!\n"); + return -1; + } + + if (of_device_is_compatible(msi_node, "sophgo,sg2042-pcie-msi")) { + ret = sg2042_pcie_setup_msi(pcie, msi_node); + if (ret < 0) + goto err_setup_msi; + } + + ret = cdns_pcie_init_phy(dev, cdns_pcie); + if (ret) { + dev_err(dev, "Failed to init phy!\n"); + goto err_setup_msi; + } + + ret = cdns_pcie_host_setup(rc); + if (ret < 0) { + dev_err(dev, "Failed to setup host!\n"); + goto err_host_setup; + } + + return 0; + +err_host_setup: + cdns_pcie_disable_phy(cdns_pcie); + +err_setup_msi: + sg2042_pcie_free_msi(pcie); + +err_get_sync: + pm_runtime_put(dev); + pm_runtime_disable(dev); + + return ret; +} + +static void sg2042_pcie_shutdown(struct platform_device *pdev) +{ + struct sg2042_pcie *pcie = platform_get_drvdata(pdev); + struct cdns_pcie *cdns_pcie = pcie->cdns_pcie; + struct device *dev = &pdev->dev; + + sg2042_pcie_free_msi(pcie); + + cdns_pcie_disable_phy(cdns_pcie); + + pm_runtime_put(dev); + pm_runtime_disable(dev); +} + +static const struct of_device_id sg2042_pcie_of_match[] = { + { .compatible = "sophgo,sg2042-pcie-host" }, + {}, +}; + +static struct platform_driver sg2042_pcie_driver = { + .driver = { + .name = "sg2042-pcie", + .of_match_table = sg2042_pcie_of_match, + .pm = &cdns_pcie_pm_ops, + }, + .probe = sg2042_pcie_probe, + .shutdown = sg2042_pcie_shutdown, +}; +builtin_platform_driver(sg2042_pcie_driver);