Re: [PATCH v6 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver
From: Bjorn Helgaas
Date: Wed Nov 08 2023 - 10:31:39 EST
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;
}