On Fri, Aug 18, 2023 at 03:05:07PM +0530, Thippeswamy Havalige wrote: > Add support for Xilinx XDMA Soft IP core as Root Port. > > The Zynq UltraScale+ MPSoCs devices support XDMA soft IP module in > programmable logic. > > The integrated XDMA soft IP block has integrated bridge function that > can act as PCIe Root Port. > > Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@xxxxxxx> > Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@xxxxxxx> > --- > changes in v6: > - Replaced chained irq's with regular interrupts. > - Modified interrupt names. > - Aligned to 80 columns > changes in v5: > - Added detailed comments for link_up check. > - Modified upper case hex values to lower case. > ... I really appreciate the chained IRQ to regular IRQ conversion here, and I'm just pulling out the diff between v5 and v6 here because it's a good example that may be useful to anybody who has the opportunity to do similar conversions for other drivers. These are the v5 and v6 xilinx-xdma patches: v5 https://lore.kernel.org/r/20230628092812.1592644-4-thippeswamy.havalige@xxxxxxx v6 https://lore.kernel.org/r/20230818093507.24435-4-thippeswamy.havalige@xxxxxxx And here's the diff between them: diff --git a/drivers/pci/controller/pcie-xilinx-dma-pl.c b/drivers/pci/controller/pcie-xilinx-dma-pl.c index 938c6934839c..c9673e905f4b 100644 --- a/drivers/pci/controller/pcie-xilinx-dma-pl.c +++ b/drivers/pci/controller/pcie-xilinx-dma-pl.c @@ -8,7 +8,6 @@ #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/irqdomain.h> -#include <linux/irqchip/chained_irq.h> #include <linux/kernel.h> #include <linux/module.h> #include <linux/msi.h> @@ -96,9 +95,8 @@ struct xilinx_msi { * @pldma_domain: PL DMA IRQ domain pointer * @resources: Bus Resources * @msi: MSI information - * @irq_misc: Legacy and error interrupt number - * @intx_irq: legacy interrupt number - * @lock: lock protecting shared register access + * @intx_irq: INTx error interrupt number + * @lock: Lock protecting shared register access */ struct pl_dma_pcie { struct device *dev; @@ -110,7 +108,6 @@ struct pl_dma_pcie { struct irq_domain *pldma_domain; struct list_head resources; struct xilinx_msi msi; - int irq_misc; int intx_irq; raw_spinlock_t lock; }; @@ -143,19 +140,23 @@ static void xilinx_pl_dma_pcie_clear_err_interrupts(struct pl_dma_pcie *port) } } -static bool xilinx_pl_dma_pcie_valid_device(struct pci_bus *bus, unsigned int devfn) +static bool xilinx_pl_dma_pcie_valid_device(struct pci_bus *bus, + unsigned int devfn) { struct pl_dma_pcie *port = bus->sysdata; /* Check if link is up when trying to access downstream ports */ if (!pci_is_root_bus(bus)) { /* - * If the link goes down after we check for link-up, we have a problem: - * if a PIO request is initiated while link-down, the whole controller - * hangs, and even after link comes up again, previous PIO requests - * won't work, and a reset of the whole PCIe controller is needed. - * Henceforth we need link-up check here to avoid sending PIO request - * when link is down. + * Checking whether link is up here is a last line of defence, + * if the link goes down after we check for link-up, we have a + * problem: if a PIO request is initiated while link-down, the + * whole controller hangs, and even after link comes up again, + * previous PIO requests won't work, and a reset of the whole + * PCIe controller is needed. Henceforth we need link-up check + * here to avoid sending PIO request when link is down. This + * check is racy by definition and does not make controller hang + * if the link goes down after this check is performed. */ if (!xilinx_pl_dma_pcie_link_up(port)) return false; @@ -178,7 +179,7 @@ static void __iomem *xilinx_pl_dma_pcie_map_bus(struct pci_bus *bus, } /* PCIe operations */ -static const struct pci_ecam_ops xilinx_pl_dma_pcie_ops = { +static struct pci_ecam_ops xilinx_pl_dma_pcie_ops = { .pci_ops = { .map_bus = xilinx_pl_dma_pcie_map_bus, .read = pci_generic_config_read, @@ -221,13 +222,13 @@ static void xilinx_unmask_intx_irq(struct irq_data *data) } static struct irq_chip xilinx_leg_irq_chip = { - .name = "INTx", + .name = "pl_dma:INTx", .irq_mask = xilinx_mask_intx_irq, .irq_unmask = xilinx_unmask_intx_irq, }; -static int xilinx_pl_dma_pcie_intx_map(struct irq_domain *domain, unsigned int irq, - irq_hw_number_t hwirq) +static int xilinx_pl_dma_pcie_intx_map(struct irq_domain *domain, + unsigned int irq, irq_hw_number_t hwirq) { irq_set_chip_and_handler(irq, &xilinx_leg_irq_chip, handle_level_irq); irq_set_chip_data(irq, domain->host_data); @@ -241,55 +242,54 @@ static const struct irq_domain_ops intx_domain_ops = { .map = xilinx_pl_dma_pcie_intx_map, }; -static void xilinx_pl_dma_pcie_handle_msi_irq(struct pl_dma_pcie *port, - u32 status_reg) +static irqreturn_t xilinx_pl_dma_pcie_msi_handler_high(int irq, void *args) { struct xilinx_msi *msi; unsigned long status; u32 bit, virq; + struct pl_dma_pcie *port = args; + + msi = &port->msi; + + while ((status = pcie_read(port, XILINX_PCIE_DMA_REG_MSI_HI)) != 0) { + for_each_set_bit(bit, &status, 32) { + pcie_write(port, 1 << bit, XILINX_PCIE_DMA_REG_MSI_HI); + bit = bit + 32; + virq = irq_find_mapping(msi->dev_domain, bit); + if (virq) + generic_handle_irq(virq); + } + } + return IRQ_HANDLED; +} + +static irqreturn_t xilinx_pl_dma_pcie_msi_handler_low(int irq, void *args) +{ + struct pl_dma_pcie *port = args; + struct xilinx_msi *msi; + unsigned long status; + u32 bit, virq; msi = &port->msi; - while ((status = pcie_read(port, status_reg)) != 0) { + while ((status = pcie_read(port, XILINX_PCIE_DMA_REG_MSI_LOW)) != 0) { for_each_set_bit(bit, &status, 32) { - pcie_write(port, 1 << bit, status_reg); - if (status_reg == XILINX_PCIE_DMA_REG_MSI_HI) - bit = bit + 32; + pcie_write(port, 1 << bit, XILINX_PCIE_DMA_REG_MSI_LOW); virq = irq_find_mapping(msi->dev_domain, bit); if (virq) generic_handle_irq(virq); } } + + return IRQ_HANDLED; } -static void xilinx_pl_dma_pcie_msi_handler_high(struct irq_desc *desc) +static irqreturn_t xilinx_pl_dma_pcie_event_flow(int irq, void *args) { - struct pl_dma_pcie *port = irq_desc_get_handler_data(desc); - struct irq_chip *chip = irq_desc_get_chip(desc); - - chained_irq_enter(chip, desc); - xilinx_pl_dma_pcie_handle_msi_irq(port, XILINX_PCIE_DMA_REG_MSI_HI); - chained_irq_exit(chip, desc); -} - -static void xilinx_pl_dma_pcie_msi_handler_low(struct irq_desc *desc) -{ - struct pl_dma_pcie *port = irq_desc_get_handler_data(desc); - struct irq_chip *chip = irq_desc_get_chip(desc); - - chained_irq_enter(chip, desc); - xilinx_pl_dma_pcie_handle_msi_irq(port, XILINX_PCIE_DMA_REG_MSI_LOW); - chained_irq_exit(chip, desc); -} - -static void xilinx_pl_dma_pcie_event_flow(struct irq_desc *desc) -{ - struct pl_dma_pcie *port = irq_desc_get_handler_data(desc); - struct irq_chip *chip = irq_desc_get_chip(desc); + struct pl_dma_pcie *port = args; unsigned long val; int i; - chained_irq_enter(chip, desc); val = pcie_read(port, XILINX_PCIE_DMA_REG_IDR); val &= pcie_read(port, XILINX_PCIE_DMA_REG_IMR); for_each_set_bit(i, &val, 32) @@ -297,7 +297,7 @@ static void xilinx_pl_dma_pcie_event_flow(struct irq_desc *desc) pcie_write(port, val, XILINX_PCIE_DMA_REG_IDR); - chained_irq_exit(chip, desc); + return IRQ_HANDLED; } #define _IC(x, s) \ @@ -313,8 +313,6 @@ static const struct { _IC(CORRECTABLE, "Correctable error message"), _IC(NONFATAL, "Non fatal error message"), _IC(FATAL, "Fatal error message"), - _IC(INTX, "INTX error message"), - _IC(MSI, "MSI message received"), _IC(SLV_UNSUPP, "Slave unsupported request"), _IC(SLV_UNEXP, "Slave unexpected completion"), _IC(SLV_COMPL, "Slave completion timeout"), @@ -350,7 +348,7 @@ static irqreturn_t xilinx_pl_dma_pcie_intr_handler(int irq, void *dev_id) } static struct irq_chip xilinx_msi_irq_chip = { - .name = "pl_dma_pciepcie:msi", + .name = "pl_dma:PCIe MSI", .irq_enable = pci_msi_unmask_irq, .irq_disable = pci_msi_mask_irq, .irq_mask = pci_msi_mask_irq, @@ -380,7 +378,7 @@ static int xilinx_msi_set_affinity(struct irq_data *irq_data, } static struct irq_chip xilinx_irq_chip = { - .name = "Xilinx MSI", + .name = "pl_dma:MSI", .irq_compose_msi_msg = xilinx_compose_msi_msg, .irq_set_affinity = xilinx_msi_set_affinity, }; @@ -427,12 +425,6 @@ static const struct irq_domain_ops dev_msi_domain_ops = { .free = xilinx_irq_domain_free, }; -static void xilinx_pl_dma_pcie_free_interrupts(struct pl_dma_pcie *port) -{ - irq_set_chained_handler_and_data(port->msi.irq_msi0, NULL, NULL); - irq_set_chained_handler_and_data(port->msi.irq_msi1, NULL, NULL); -} - static void xilinx_pl_dma_pcie_free_irq_domains(struct pl_dma_pcie *port) { struct xilinx_msi *msi = &port->msi; @@ -487,22 +479,23 @@ static int xilinx_pl_dma_pcie_init_msi_irq_domain(struct pl_dma_pcie *port) return -ENOMEM; } -static void xilinx_pl_dma_pcie_intx_flow(struct irq_desc *desc) +/* INTx error interrupts are Xilinx controller specific interrupt, used to + * notify user about error's such as cfg timeout, slave unsupported requests, + * fatal and non fatal error etc. + */ + +static irqreturn_t xilinx_pl_dma_pcie_intx_flow(int irq, void *args) { - struct pl_dma_pcie *port = irq_desc_get_handler_data(desc); - struct irq_chip *chip = irq_desc_get_chip(desc); unsigned long val; int i; - - chained_irq_enter(chip, desc); + struct pl_dma_pcie *port = args; val = FIELD_GET(XILINX_PCIE_DMA_IDRN_MASK, pcie_read(port, XILINX_PCIE_DMA_REG_IDRN)); for_each_set_bit(i, &val, PCI_NUM_INTX) generic_handle_domain_irq(port->intx_domain, i); - - chained_irq_exit(chip, desc); + return IRQ_HANDLED; } static void xilinx_pl_dma_pcie_mask_event_irq(struct irq_data *d) @@ -530,7 +523,7 @@ static void xilinx_pl_dma_pcie_unmask_event_irq(struct irq_data *d) } static struct irq_chip xilinx_pl_dma_pcie_event_irq_chip = { - .name = "RC-Event", + .name = "pl_dma:RC-Event", .irq_mask = xilinx_pl_dma_pcie_mask_event_irq, .irq_unmask = xilinx_pl_dma_pcie_unmask_event_irq, }; @@ -602,7 +595,7 @@ static int xilinx_pl_dma_pcie_setup_irq(struct pl_dma_pcie *port) { struct device *dev = port->dev; struct platform_device *pdev = to_platform_device(dev); - int i, irq; + int i, irq, err; port->irq = platform_get_irq(pdev, 0); if (port->irq < 0) @@ -621,7 +614,7 @@ static int xilinx_pl_dma_pcie_setup_irq(struct pl_dma_pcie *port) } err = devm_request_irq(dev, irq, xilinx_pl_dma_pcie_intr_handler, - 0, intr_cause[i].sym, port); + IRQF_SHARED | IRQF_NO_THREAD, intr_cause[i].sym, port); if (err) { dev_err(dev, "Failed to request IRQ %d\n", irq); return err; @@ -635,14 +628,18 @@ static int xilinx_pl_dma_pcie_setup_irq(struct pl_dma_pcie *port) return -ENXIO; } - /* Plug the INTx chained handler */ - irq_set_chained_handler_and_data(port->intx_irq, - xilinx_pl_dma_pcie_intx_flow, port); - - /* Plug the main event chained handler */ - irq_set_chained_handler_and_data(port->irq, - xilinx_pl_dma_pcie_event_flow, port); - + err = devm_request_irq(dev, port->intx_irq, xilinx_pl_dma_pcie_intx_flow, + IRQF_SHARED | IRQF_NO_THREAD, NULL, port); + if (err) { + dev_err(dev, "Failed to request INTx IRQ %d\n", irq); + return err; + } + err = devm_request_irq(dev, port->irq, xilinx_pl_dma_pcie_event_flow, + IRQF_SHARED | IRQF_NO_THREAD, NULL, port); + if (err) { + dev_err(dev, "Failed to request event IRQ %d\n", irq); + return err; + } return 0; } @@ -666,7 +663,7 @@ static void xilinx_pl_dma_pcie_init_port(struct pl_dma_pcie *port) pcie_write(port, XILINX_PCIE_DMA_IDR_ALL_MASK, XILINX_PCIE_DMA_REG_MSI_LOW_MASK); pcie_write(port, XILINX_PCIE_DMA_IDR_ALL_MASK, XILINX_PCIE_DMA_REG_MSI_HI_MASK); - /*set the Bridge enable bit */ + /* Set the Bridge enable bit */ pcie_write(port, pcie_read(port, XILINX_PCIE_DMA_REG_RPSC) | XILINX_PCIE_DMA_REG_RPSC_BEN, XILINX_PCIE_DMA_REG_RPSC); @@ -676,29 +673,32 @@ static int xilinx_request_msi_irq(struct pl_dma_pcie *port) { struct device *dev = port->dev; struct platform_device *pdev = to_platform_device(dev); + int ret; port->msi.irq_msi0 = platform_get_irq_byname(pdev, "msi0"); if (port->msi.irq_msi0 <= 0) { dev_err(dev, "Unable to find msi0 IRQ line\n"); return port->msi.irq_msi0; } - - irq_set_chained_handler_and_data(port->msi.irq_msi0, - xilinx_pl_dma_pcie_msi_handler_low, - port); - + ret = devm_request_irq(dev, port->msi.irq_msi0, xilinx_pl_dma_pcie_msi_handler_low, + IRQF_SHARED | IRQF_NO_THREAD, "xlnx-pcie-dma-pl", + port); + if (ret) { + dev_err(dev, "Failed to register interrupt\n"); + return ret; + } port->msi.irq_msi1 = platform_get_irq_byname(pdev, "msi1"); if (port->msi.irq_msi1 <= 0) { - irq_set_chained_handler_and_data(port->msi.irq_msi0, - NULL, NULL); dev_err(dev, "Unable to find msi1 IRQ line\n"); return port->msi.irq_msi1; } - - irq_set_chained_handler_and_data(port->msi.irq_msi1, - xilinx_pl_dma_pcie_msi_handler_high, - port); - + ret = devm_request_irq(dev, port->msi.irq_msi1, xilinx_pl_dma_pcie_msi_handler_high, + IRQF_SHARED | IRQF_NO_THREAD, "xlnx-pcie-dma-pl", + port); + if (ret) { + dev_err(dev, "Failed to register interrupt\n"); + return ret; + } return 0; } @@ -712,7 +712,7 @@ static int xilinx_pl_dma_pcie_parse_dt(struct pl_dma_pcie *port, res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) { - dev_err(dev, "missing \"reg\" property\n"); + dev_err(dev, "Missing \"reg\" property\n"); return -ENXIO; } port->phys_reg_base = res->start; @@ -767,7 +767,7 @@ static int xilinx_pl_dma_pcie_probe(struct platform_device *pdev) err = xilinx_pl_dma_pcie_setup_irq(port); bridge->sysdata = port; - bridge->ops = (struct pci_ops *)&xilinx_pl_dma_pcie_ops.pci_ops; + bridge->ops = &xilinx_pl_dma_pcie_ops.pci_ops; err = pci_host_probe(bridge); if (err < 0) @@ -780,7 +780,6 @@ static int xilinx_pl_dma_pcie_probe(struct platform_device *pdev) err_irq_domain: pci_ecam_free(port->cfg); - xilinx_pl_dma_pcie_free_interrupts(port); return err; }
