On Wed, Jan 15, 2025 at 03:06:57PM +0800, Chen Wang wrote:ok.
From: Chen Wang <unicorn_wang@xxxxxxxxxxx>Please add more info about the IP. Like IP revision, PCIe Gen, lane count,
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.
etc...
Signed-off-by: Chen Wang <unicorn_wang@xxxxxxxxxxx>Spurious newline.
---
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.
+
+/*How to configure them? I can only see "sophgo,sg2042-msi" in the binding.
+ * 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.
ok+struct sg2042_pcie {Get rid of the newline between struct members, please.
+ 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 int sg2042_pcie_setup_msi(struct sg2042_pcie *pcie,The MSI controller is wired to PLIC isn't it? If so, why can't you use
+ 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);
+
hierarchial MSI domain implementation as like other controller drivers?
+/* 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.
Got it.+ bridge = devm_pci_alloc_host_bridge(dev, sizeof(*rc));Use dev_err_probe() throughout the probe function.
+ if (!bridge) {
+ dev_err(dev, "Failed to alloc host bridge!\n");
Got.+ return -ENOMEM;-ENODEV
+ }
+
+ 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;
ok, I will double check this through the file.+ }You should drop the np reference count once you are done with it.
+ syscon = syscon_node_to_regmap(np_syscon);
+ if (IS_ERR(syscon)) {PTR_ERR(syscon)
+ dev_err(dev, "Failed to get regmap for syscon\n");
+ return -ENOMEM;
ok.+ }"Unable to parse \"sophgo,link-id\"\n"
+ 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");
Got it.+ return -EINVAL;Use pm_runtime_resume_and_get().
+ }
+
+ platform_set_drvdata(pdev, pcie);
+
+ pm_runtime_enable(dev);
+
+ ret = pm_runtime_get_sync(dev);
Thanks, this should be fixed.+ if (ret < 0) {-ENODATA
+ 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;
ok, will check this through the file.
+ }Same as above. np leak here.
+
+ if (of_device_is_compatible(msi_node, "sophgo,sg2042-pcie-msi")) {
+ ret = sg2042_pcie_setup_msi(pcie, msi_node);
- Mani