[PATCH v4 0/10] Add new helper to allocate IRQ domain for host drivers

[PATCH v4 0/10] Add new helper to allocate IRQ domain for host drivers

[Shawn Lin]

Hi Bjorn and Lorenzo,

PCI drivers use highly similar code block to allocate IRQ domain which
leads to code duplication than expected. This patchset adds a new helper,
pci_host_alloc_intx_irqd(), to avoid that as much as possible.


Changes in v4:
- fix another compile warning reported by Kbuild Robot

Changes in v3:
- rename to pci_host_alloc_intx_irqd and move to probe.c
- fix compile error reported by Kbuild Robot

Changes in v2:
- fix typo and move the code to C file.

Shawn Lin (10):
  PCI: Add pci_host_alloc_intx_irqd() for allocating IRQ domain
  PCI: dra7xx: Use pci_host_alloc_intx_irqd() helper to simplify the
    code
  PCI: keystone-dw: Use pci_host_alloc_intx_irqd() helper to get irq
    domain for INTx
  PCI: aardvark: Use pci_host_alloc_intx_irqd() helper to get irq domain
    for INTx
  PCI: faraday: Use pci_host_alloc_intx_irqd() helper to get irq domain
    for INTx
  PCI: altera: Use pci_host_alloc_intx_irqd() helper to get irq domain
    for INTx
  PCI: mediatek: Use pci_host_alloc_intx_irqd() helper to get irq domain
    for INTx
  PCI: xilinx-nwl: Use pci_host_alloc_intx_irqd() helper to get irq
    domain for INTx
  PCI: xilinx: Use pci_host_alloc_intx_irqd() helper to get irq domain
    for INTx
  PCI: rockchip: Use pci_host_alloc_intx_irqd() helper to get irq domain
    for INTx

 drivers/pci/controller/dwc/pci-dra7xx.c      | 42 ++--------------
 drivers/pci/controller/dwc/pci-keystone-dw.c | 13 ++---
 drivers/pci/controller/pci-aardvark.c        | 24 ++-------
 drivers/pci/controller/pci-ftpci100.c        | 15 ++----
 drivers/pci/controller/pcie-altera.c         | 38 ++------------
 drivers/pci/controller/pcie-mediatek.c       | 30 ++---------
 drivers/pci/controller/pcie-rockchip-host.c  | 38 ++------------
 drivers/pci/controller/pcie-xilinx-nwl.c     | 22 ++-------
 drivers/pci/controller/pcie-xilinx.c         | 44 ++---------------
 drivers/pci/probe.c                          | 74 ++++++++++++++++++++++++++++
 include/linux/pci.h                          |  7 +++
 11 files changed, 119 insertions(+), 228 deletions(-)

-- 
1.9.1

[PATCH v4 01/10] PCI: Add pci_host_alloc_intx_irqd() for allocating IRQ domain

[Shawn Lin]

It seems host drivers which allocate IRQ domain for INTx
and the related code block looks highly similar to each other.
Add a new helper, pci_alloc_intx_irqd(), to avoid code duplication
as much as possible.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v4:
- fix another compile warning reported by Kbuild Robot

Changes in v3:
- rename to pci_host_alloc_intx_irqd and move to probe.c
- fix compile error reported by Kbuild Robot

Changes in v2:
- fix typo and move the code to C file.

 drivers/pci/probe.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pci.h |  7 +++++
 2 files changed, 81 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ac876e3..d37b879 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -3139,3 +3139,77 @@ int pci_hp_add_bridge(struct pci_dev *dev)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(pci_hp_add_bridge);
+
+static int pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+			 irq_hw_number_t hwirq)
+{
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+	irq_set_chip_data(irq, domain->host_data);
+
+	return 0;
+}
+
+/**
+ * pci_host_alloc_intx_irqd() - Allocate INTx IRQ domain
+ * @dev: device associated with the PCI controller.
+ * @host: pointer to host specific data struct
+ * @general_xlate: flag for whether use pci_irqd_intx_xlate() helper
+ * @intx_domain_ops: pointer to driver specific struct irq_domain_ops
+ * @local_intc: pointer to driver specific interrupt controller node
+ *
+ * A simple helper for drivers to allocate IRQ domain for INTx. If
+ * intx_domain_ops is NULL, use pci_intx_domain_ops by default. And if
+ * local_intc is present, then use it firstly, otherwise, fallback to get
+ * interrupt controller node from @dev.
+ *
+ * Returns valid pointer of struct irq_domain on success, or PTR_ERR(-EINVAL)
+ * if failure occurred.
+ */
+struct irq_domain *pci_host_alloc_intx_irqd(struct device *dev, void *host,
+	bool general_xlate, const struct irq_domain_ops *intx_domain_ops,
+	struct device_node *local_intc)
+{
+	struct device_node *intc = local_intc;
+	struct irq_domain *domain = NULL;
+	struct irq_domain_ops *irqd_ops;
+	bool need_put = false;
+	int err = -EINVAL;
+
+	if (!intc) {
+		intc = of_get_next_child(dev->of_node, NULL);
+		if (!intc) {
+			dev_err(dev, "missing child interrupt-controller node\n");
+			goto err_out;
+		}
+		need_put = true;
+	}
+
+	if (!intx_domain_ops) {
+		irqd_ops = (struct irq_domain_ops *)devm_kmalloc(dev,
+				sizeof(struct irq_domain_ops), GFP_KERNEL);
+		if (!irqd_ops) {
+			err = -ENOMEM;
+			goto err_out;
+		}
+
+		irqd_ops->map = &pcie_intx_map;
+		if (general_xlate)
+			irqd_ops->xlate = &pci_irqd_intx_xlate;
+		intx_domain_ops = irqd_ops;
+	}
+#ifdef CONFIG_IRQ_DOMAIN
+	domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
+				       intx_domain_ops, host);
+#endif
+	if (!domain) {
+		dev_err(dev, "failed to get a INTx IRQ domain\n");
+		goto err_out;
+	}
+
+	return domain;
+err_out:
+	if (need_put)
+		of_node_put(intc);
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(pci_host_alloc_intx_irqd);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 340029b..8360972 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -31,6 +31,9 @@
 #include <linux/device.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_irq.h>
 #include <linux/resource_ext.h>
 #include <uapi/linux/pci.h>
 
@@ -1453,6 +1456,10 @@ static inline int pci_irqd_intx_xlate(struct irq_domain *d,
 	return 0;
 }
 
+struct irq_domain *pci_host_alloc_intx_irqd(struct device *dev,
+				void *host, bool general_xlate,
+				const struct irq_domain_ops *intx_domain_ops,
+				struct device_node *local_intc);
 #ifdef CONFIG_PCIEPORTBUS
 extern bool pcie_ports_disabled;
 extern bool pcie_ports_native;
-- 
1.9.1

[PATCH v4 02/10] PCI: dra7xx: Use pci_host_alloc_intx_irqd() helper to simplify the code

[Shawn Lin]

Just avoid code duplication, but no functional change intended.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/controller/dwc/pci-dra7xx.c | 42 +++------------------------------
 1 file changed, 3 insertions(+), 39 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-dra7xx.c b/drivers/pci/controller/dwc/pci-dra7xx.c
index cfaeef8..a281924 100644
--- a/drivers/pci/controller/dwc/pci-dra7xx.c
+++ b/drivers/pci/controller/dwc/pci-dra7xx.c
@@ -213,43 +213,6 @@ static int dra7xx_pcie_host_init(struct pcie_port *pp)
 	.host_init = dra7xx_pcie_host_init,
 };
 
-static int dra7xx_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
-				irq_hw_number_t hwirq)
-{
-	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
-	irq_set_chip_data(irq, domain->host_data);
-
-	return 0;
-}
-
-static const struct irq_domain_ops intx_domain_ops = {
-	.map = dra7xx_pcie_intx_map,
-	.xlate = pci_irqd_intx_xlate,
-};
-
-static int dra7xx_pcie_init_irq_domain(struct pcie_port *pp)
-{
-	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
-	struct device *dev = pci->dev;
-	struct dra7xx_pcie *dra7xx = to_dra7xx_pcie(pci);
-	struct device_node *node = dev->of_node;
-	struct device_node *pcie_intc_node =  of_get_next_child(node, NULL);
-
-	if (!pcie_intc_node) {
-		dev_err(dev, "No PCIe Intc node found\n");
-		return -ENODEV;
-	}
-
-	dra7xx->irq_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
-						   &intx_domain_ops, pp);
-	if (!dra7xx->irq_domain) {
-		dev_err(dev, "Failed to get a INTx IRQ domain\n");
-		return -ENODEV;
-	}
-
-	return 0;
-}
-
 static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg)
 {
 	struct dra7xx_pcie *dra7xx = arg;
@@ -455,8 +418,9 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx,
 		return ret;
 	}
 
-	ret = dra7xx_pcie_init_irq_domain(pp);
-	if (ret < 0)
+	dra7xx->irq_domain = pci_host_alloc_intx_irqd(dev, dra7xx, true, NULL,
+						      NULL);
+	if (IS_ERR(dra7xx->irq_domain))
 		return ret;
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "rc_dbics");
-- 
1.9.1

[PATCH v4 03/10] PCI: keystone-dw: Use pci_host_alloc_intx_irqd() helper to get irq domain for INTx

[Shawn Lin]

Just avoid code duplication, but no functional change intended.

Cc: Kishon Vijay Abraham I <kishon@ti.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/controller/dwc/pci-keystone-dw.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone-dw.c b/drivers/pci/controller/dwc/pci-keystone-dw.c
index 0682213..a45809d 100644
--- a/drivers/pci/controller/dwc/pci-keystone-dw.c
+++ b/drivers/pci/controller/dwc/pci-keystone-dw.c
@@ -470,15 +470,12 @@ int __init ks_dw_pcie_host_init(struct keystone_pcie *ks_pcie,
 	ks_pcie->app = *res;
 
 	/* Create legacy IRQ domain */
-	ks_pcie->legacy_irq_domain =
-			irq_domain_add_linear(ks_pcie->legacy_intc_np,
-					PCI_NUM_INTX,
+	ks_pcie->legacy_irq_domain = pci_host_alloc_intx_irqd(dev, ks_pcie,
+					false,
 					&ks_dw_pcie_legacy_irq_domain_ops,
-					NULL);
-	if (!ks_pcie->legacy_irq_domain) {
-		dev_err(dev, "Failed to add irq domain for legacy irqs\n");
-		return -EINVAL;
-	}
+					ks_pcie->legacy_intc_np);
+	if (IS_ERR(ks_pcie->legacy_irq_domain))
+		return PTR_ERR(ks_pcie->legacy_irq_domain);
 
 	return dw_pcie_host_init(pp);
 }
-- 
1.9.1

[PATCH v4 04/10] PCI: aardvark: Use pci_host_alloc_intx_irqd() helper to get irq domain for INTx

[Shawn Lin]

Just avoid code duplication, but no functional change intended.

Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/controller/pci-aardvark.c | 24 +++++-------------------
 1 file changed, 5 insertions(+), 19 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index d3172d5..4c31582 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -704,37 +704,23 @@ static void advk_pcie_remove_msi_irq_domain(struct advk_pcie *pcie)
 static int advk_pcie_init_irq_domain(struct advk_pcie *pcie)
 {
 	struct device *dev = &pcie->pdev->dev;
-	struct device_node *node = dev->of_node;
-	struct device_node *pcie_intc_node;
 	struct irq_chip *irq_chip;
 
-	pcie_intc_node =  of_get_next_child(node, NULL);
-	if (!pcie_intc_node) {
-		dev_err(dev, "No PCIe Intc node found\n");
-		return -ENODEV;
-	}
-
 	irq_chip = &pcie->irq_chip;
 
 	irq_chip->name = devm_kasprintf(dev, GFP_KERNEL, "%s-irq",
 					dev_name(dev));
-	if (!irq_chip->name) {
-		of_node_put(pcie_intc_node);
+	if (!irq_chip->name)
 		return -ENOMEM;
-	}
 
 	irq_chip->irq_mask = advk_pcie_irq_mask;
 	irq_chip->irq_mask_ack = advk_pcie_irq_mask;
 	irq_chip->irq_unmask = advk_pcie_irq_unmask;
 
-	pcie->irq_domain =
-		irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
-				      &advk_pcie_irq_domain_ops, pcie);
-	if (!pcie->irq_domain) {
-		dev_err(dev, "Failed to get a INTx IRQ domain\n");
-		of_node_put(pcie_intc_node);
-		return -ENOMEM;
-	}
+	pcie->irq_domain = pci_host_alloc_intx_irqd(dev, pcie, false,
+				&advk_pcie_irq_domain_ops, NULL);
+	if (IS_ERR(pcie->irq_domain))
+		return PTR_ERR(pcie->irq_domain);
 
 	return 0;
 }
-- 
1.9.1

[PATCH v4 05/10] PCI: faraday: Use pci_host_alloc_intx_irqd() helper to get irq domain for INTx

[Shawn Lin]

Just avoid code duplication, but no functional change intended.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/controller/pci-ftpci100.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pci-ftpci100.c b/drivers/pci/controller/pci-ftpci100.c
index a1ebe9e..8ace724 100644
--- a/drivers/pci/controller/pci-ftpci100.c
+++ b/drivers/pci/controller/pci-ftpci100.c
@@ -346,24 +346,19 @@ static int faraday_pci_setup_cascaded_irq(struct faraday_pci *p)
 	int irq;
 	int i;
 
-	if (!intc) {
-		dev_err(p->dev, "missing child interrupt-controller node\n");
-		return -EINVAL;
-	}
+	p->irqdomain = pci_host_alloc_intx_irqd(p->dev, p, false,
+					   &faraday_pci_irqdomain_ops, intc);
+	if (IS_ERR(p->irqdomain))
+		return PTR_ERR(p->irqdomain);
 
 	/* All PCI IRQs cascade off this one */
 	irq = of_irq_get(intc, 0);
 	if (irq <= 0) {
 		dev_err(p->dev, "failed to get parent IRQ\n");
+		irq_domain_remove(p->irqdomain);
 		return irq ?: -EINVAL;
 	}
 
-	p->irqdomain = irq_domain_add_linear(intc, PCI_NUM_INTX,
-					     &faraday_pci_irqdomain_ops, p);
-	if (!p->irqdomain) {
-		dev_err(p->dev, "failed to create Gemini PCI IRQ domain\n");
-		return -EINVAL;
-	}
 
 	irq_set_chained_handler_and_data(irq, faraday_pci_irq_handler, p);
 
-- 
1.9.1

[PATCH v4 06/10] PCI: altera: Use pci_host_alloc_intx_irqd() helper to get irq domain for INTx

[Shawn Lin]

Just avoid code duplication, but no functional change intended.

Cc: Ley Foon Tan <ley.foon.tan@intel.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/controller/pcie-altera.c | 38 ++++--------------------------------
 1 file changed, 4 insertions(+), 34 deletions(-)

diff --git a/drivers/pci/controller/pcie-altera.c b/drivers/pci/controller/pcie-altera.c
index 7d05e51..0915c8b 100644
--- a/drivers/pci/controller/pcie-altera.c
+++ b/drivers/pci/controller/pcie-altera.c
@@ -443,19 +443,6 @@ static void altera_pcie_retrain(struct altera_pcie *pcie)
 	}
 }
 
-static int altera_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
-				irq_hw_number_t hwirq)
-{
-	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
-	irq_set_chip_data(irq, domain->host_data);
-	return 0;
-}
-
-static const struct irq_domain_ops intx_domain_ops = {
-	.map = altera_pcie_intx_map,
-	.xlate = pci_irqd_intx_xlate,
-};
-
 static void altera_pcie_isr(struct irq_desc *desc)
 {
 	struct irq_chip *chip = irq_desc_get_chip(desc);
@@ -519,22 +506,6 @@ static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
 	return err;
 }
 
-static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
-{
-	struct device *dev = &pcie->pdev->dev;
-	struct device_node *node = dev->of_node;
-
-	/* Setup INTx */
-	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX,
-					&intx_domain_ops, pcie);
-	if (!pcie->irq_domain) {
-		dev_err(dev, "Failed to get a INTx IRQ domain\n");
-		return -ENOMEM;
-	}
-
-	return 0;
-}
-
 static int altera_pcie_parse_dt(struct altera_pcie *pcie)
 {
 	struct device *dev = &pcie->pdev->dev;
@@ -592,11 +563,10 @@ static int altera_pcie_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = altera_pcie_init_irq_domain(pcie);
-	if (ret) {
-		dev_err(dev, "Failed creating IRQ Domain\n");
-		return ret;
-	}
+	pcie->irq_domain = pci_host_alloc_intx_irqd(dev, pcie, true, NULL,
+						    dev->of_node);
+	if (IS_ERR(pcie->irq_domain))
+		return PTR_ERR(pcie->irq_domain);
 
 	/* clear all interrupts */
 	cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
-- 
1.9.1

[PATCH v4 07/10] PCI: mediatek: Use pci_host_alloc_intx_irqd() helper to get irq domain for INTx

[Shawn Lin]

Just avoid code duplication, but no functional change intended.

Cc: Ryder Lee <ryder.lee@mediatek.com>
Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/controller/pcie-mediatek.c | 30 ++++--------------------------
 1 file changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 0baabe3..b35a8ce 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -590,39 +590,17 @@ static void mtk_pcie_enable_msi(struct mtk_pcie_port *port)
 	writel(val, port->base + PCIE_INT_MASK);
 }
 
-static int mtk_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
-			     irq_hw_number_t hwirq)
-{
-	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
-	irq_set_chip_data(irq, domain->host_data);
-
-	return 0;
-}
-
-static const struct irq_domain_ops intx_domain_ops = {
-	.map = mtk_pcie_intx_map,
-};
-
 static int mtk_pcie_init_irq_domain(struct mtk_pcie_port *port,
 				    struct device_node *node)
 {
 	struct device *dev = port->pcie->dev;
-	struct device_node *pcie_intc_node;
 	int ret;
 
-	/* Setup INTx */
-	pcie_intc_node = of_get_next_child(node, NULL);
-	if (!pcie_intc_node) {
-		dev_err(dev, "no PCIe Intc node found\n");
-		return -ENODEV;
-	}
+	port->irq_domain = pci_host_alloc_intx_irqd(dev, port, false,
+						    NULL, NULL);
+	if (IS_ERR(port->irq_domain))
+		return PTR_ERR(port->irq_domain);
 
-	port->irq_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
-						 &intx_domain_ops, port);
-	if (!port->irq_domain) {
-		dev_err(dev, "failed to get INTx IRQ domain\n");
-		return -ENODEV;
-	}
 
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		ret = mtk_pcie_allocate_msi_domains(port);
-- 
1.9.1

[PATCH v4 08/10] PCI: xilinx-nwl: Use pci_host_alloc_intx_irqd() helper to get irq domain for INTx

[Shawn Lin]

Just avoid code duplication, but no functional change intended.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/controller/pcie-xilinx-nwl.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/drivers/pci/controller/pcie-xilinx-nwl.c b/drivers/pci/controller/pcie-xilinx-nwl.c
index 6a4bbb5..b0d13bb 100644
--- a/drivers/pci/controller/pcie-xilinx-nwl.c
+++ b/drivers/pci/controller/pcie-xilinx-nwl.c
@@ -546,24 +546,12 @@ static int nwl_pcie_init_msi_irq_domain(struct nwl_pcie *pcie)
 static int nwl_pcie_init_irq_domain(struct nwl_pcie *pcie)
 {
 	struct device *dev = pcie->dev;
-	struct device_node *node = dev->of_node;
-	struct device_node *legacy_intc_node;
-
-	legacy_intc_node = of_get_next_child(node, NULL);
-	if (!legacy_intc_node) {
-		dev_err(dev, "No legacy intc node found\n");
-		return -EINVAL;
-	}
 
-	pcie->legacy_irq_domain = irq_domain_add_linear(legacy_intc_node,
-							PCI_NUM_INTX,
-							&legacy_domain_ops,
-							pcie);
-
-	if (!pcie->legacy_irq_domain) {
-		dev_err(dev, "failed to create IRQ domain\n");
-		return -ENOMEM;
-	}
+	pcie->legacy_irq_domain = pci_host_alloc_intx_irqd(dev, pcie, false,
+							   &legacy_domain_ops,
+							   NULL);
+	if (IS_ERR(pcie->legacy_irq_domain))
+		return PTR_ERR(pcie->legacy_irq_domain);
 
 	raw_spin_lock_init(&pcie->leg_mask_lock);
 	nwl_pcie_init_msi_irq_domain(pcie);
-- 
1.9.1

[PATCH v4 09/10] PCI: xilinx: Use pci_host_alloc_intx_irqd() helper to get irq domain for INTx

[Shawn Lin]

Just avoid code duplication, but no functional change intended.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/controller/pcie-xilinx.c | 44 ++++--------------------------------
 1 file changed, 4 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/controller/pcie-xilinx.c b/drivers/pci/controller/pcie-xilinx.c
index b110a3a..8dbfc76 100644
--- a/drivers/pci/controller/pcie-xilinx.c
+++ b/drivers/pci/controller/pcie-xilinx.c
@@ -346,31 +346,6 @@ static void xilinx_pcie_enable_msi(struct xilinx_pcie_port *port)
 	pcie_write(port, msg_addr, XILINX_PCIE_REG_MSIBASE2);
 }
 
-/* INTx Functions */
-
-/**
- * xilinx_pcie_intx_map - Set the handler for the INTx and mark IRQ as valid
- * @domain: IRQ domain
- * @irq: Virtual IRQ number
- * @hwirq: HW interrupt number
- *
- * Return: Always returns 0.
- */
-static int xilinx_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
-				irq_hw_number_t hwirq)
-{
-	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
-	irq_set_chip_data(irq, domain->host_data);
-
-	return 0;
-}
-
-/* INTx IRQ Domain operations */
-static const struct irq_domain_ops intx_domain_ops = {
-	.map = xilinx_pcie_intx_map,
-	.xlate = pci_irqd_intx_xlate,
-};
-
 /* PCIe HW Functions */
 
 /**
@@ -497,22 +472,11 @@ static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port)
 {
 	struct device *dev = port->dev;
 	struct device_node *node = dev->of_node;
-	struct device_node *pcie_intc_node;
 
-	/* Setup INTx */
-	pcie_intc_node = of_get_next_child(node, NULL);
-	if (!pcie_intc_node) {
-		dev_err(dev, "No PCIe Intc node found\n");
-		return -ENODEV;
-	}
-
-	port->leg_domain = irq_domain_add_linear(pcie_intc_node, PCI_NUM_INTX,
-						 &intx_domain_ops,
-						 port);
-	if (!port->leg_domain) {
-		dev_err(dev, "Failed to get a INTx IRQ domain\n");
-		return -ENODEV;
-	}
+	port->leg_domain = pci_host_alloc_intx_irqd(dev, port, true, NULL,
+						    NULL);
+	if (IS_ERR(port->leg_domain))
+		return PTR_ERR(port->leg_domain);
 
 	/* Setup MSI */
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
-- 
1.9.1

[PATCH v4 10/10] PCI: rockchip: Use pci_host_alloc_intx_irqd() helper to get irq domain for INTx

[Shawn Lin]

Just avoid code duplication, but no functional change intended.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>
---

Changes in v4: None
Changes in v3: None
Changes in v2: None

 drivers/pci/controller/pcie-rockchip-host.c | 38 +++--------------------------
 1 file changed, 3 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 1372d27..eb62ba4 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -699,39 +699,6 @@ static void rockchip_pcie_enable_interrupts(struct rockchip_pcie *rockchip)
 	rockchip_pcie_enable_bw_int(rockchip);
 }
 
-static int rockchip_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
-				  irq_hw_number_t hwirq)
-{
-	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
-	irq_set_chip_data(irq, domain->host_data);
-
-	return 0;
-}
-
-static const struct irq_domain_ops intx_domain_ops = {
-	.map = rockchip_pcie_intx_map,
-};
-
-static int rockchip_pcie_init_irq_domain(struct rockchip_pcie *rockchip)
-{
-	struct device *dev = rockchip->dev;
-	struct device_node *intc = of_get_next_child(dev->of_node, NULL);
-
-	if (!intc) {
-		dev_err(dev, "missing child interrupt-controller node\n");
-		return -EINVAL;
-	}
-
-	rockchip->irq_domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
-						    &intx_domain_ops, rockchip);
-	if (!rockchip->irq_domain) {
-		dev_err(dev, "failed to get a INTx IRQ domain\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int rockchip_pcie_prog_ob_atu(struct rockchip_pcie *rockchip,
 				     int region_no, int type, u8 num_pass_bits,
 				     u32 lower_addr, u32 upper_addr)
@@ -990,8 +957,9 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 
 	rockchip_pcie_enable_interrupts(rockchip);
 
-	err = rockchip_pcie_init_irq_domain(rockchip);
-	if (err < 0)
+	rockchip->irq_domain = pci_host_alloc_intx_irqd(dev, rockchip, false,
+							NULL, NULL);
+	if (IS_ERR(rockchip->irq_domain))
 		goto err_deinit_port;
 
 	err = devm_of_pci_get_host_bridge_resources(dev, 0, 0xff,
-- 
1.9.1

Re: [PATCH v4 01/10] PCI: Add pci_host_alloc_intx_irqd() for allocating IRQ domain

[Thomas Petazzoni]

Hello,

One nit below.

On Thu, 14 Jun 2018 09:34:17 +0800, Shawn Lin wrote:

> +	if (!intc) {
> +		intc = of_get_next_child(dev->of_node, NULL);
> +		if (!intc) {
> +			dev_err(dev, "missing child interrupt-controller node\n");
> +			goto err_out;
> +		}
> +		need_put = true;
> +	}
> +
> +	if (!intx_domain_ops) {
> +		irqd_ops = (struct irq_domain_ops *)devm_kmalloc(dev,

The cast doesn't seem to be useful.

> +				sizeof(struct irq_domain_ops), GFP_KERNEL);
> +		if (!irqd_ops) {
> +			err = -ENOMEM;
> +			goto err_out;
> +		}
> +
> +		irqd_ops->map = &pcie_intx_map;
> +		if (general_xlate)
> +			irqd_ops->xlate = &pci_irqd_intx_xlate;
> +		intx_domain_ops = irqd_ops;
> +	}
> +#ifdef CONFIG_IRQ_DOMAIN
> +	domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
> +				       intx_domain_ops, host);
> +#endif

Isn't it a bit weird to have this in the middle of this function ?
Should you instead declare a stub pci_host_alloc_intx_irqd() function
in the header file if CONFIG_IRQ_DOMAIN is not enabled ?

I.e in pci.h:

#ifdef CONFIG_IRQ_DOMAIN
struct irq_domain *pci_host_alloc_intx_irqd(struct device *dev,
				void *host, bool general_xlate,
				const struct irq_domain_ops *intx_domain_ops,
				struct device_node *local_intc);
#else
static inline struct irq_domain *pci_host_alloc_intx_irqd(struct device *dev,
	void *host, bool general_xlate, const struct irq_domain_ops *intx_domain_ops,
	struct device_node *local_intc)
{
	return PTR_ERR(-EINVAL);
}
#endif

or something like that.

Finally a purely subjective and personal opinion: I'm not a big fan of
this boolean that tells whether the ->xlate hook should be populated or
not. I'm not sure if it makes sense for this function to create the
irqdomain_ops altogether, perhaps it should be mandatory for the
irq_domain_ops to be provided by the caller. The problem with the
boolean general_xlate is that it is an endless story of additional
booleans to tweak more stuff in the irq_domain_ops structure. But of
course that's just a personal view, and my view doesn't really count
here :-)

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v4 06/10] PCI: altera: Use pci_host_alloc_intx_irqd() helper to get irq domain for INTx

[Ley Foon Tan]

On Thu, 2018-06-14 at 09:35 +0800, Shawn Lin wrote:
> Just avoid code duplication, but no functional change intended.
> 
> Cc: Ley Foon Tan <ley.foon.tan@intel.com>
> Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

Reviewed-by: Ley Foon Tan <ley.foon.tan@intel.com>
> ---
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/pci/controller/pcie-altera.c | 38 ++++--------------------
> ------------
>  1 file changed, 4 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-altera.c
> b/drivers/pci/controller/pcie-altera.c
> index 7d05e51..0915c8b 100644
> --- a/drivers/pci/controller/pcie-altera.c
> +++ b/drivers/pci/controller/pcie-altera.c
> @@ -443,19 +443,6 @@ static void altera_pcie_retrain(struct
> altera_pcie *pcie)
>  	}
>  }
>  
> -static int altera_pcie_intx_map(struct irq_domain *domain, unsigned
> int irq,
> -				irq_hw_number_t hwirq)
> -{
> -	irq_set_chip_and_handler(irq, &dummy_irq_chip,
> handle_simple_irq);
> -	irq_set_chip_data(irq, domain->host_data);
> -	return 0;
> -}
> -
> -static const struct irq_domain_ops intx_domain_ops = {
> -	.map = altera_pcie_intx_map,
> -	.xlate = pci_irqd_intx_xlate,
> -};
> -
>  static void altera_pcie_isr(struct irq_desc *desc)
>  {
>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> @@ -519,22 +506,6 @@ static int
> altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
>  	return err;
>  }
>  
> -static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
> -{
> -	struct device *dev = &pcie->pdev->dev;
> -	struct device_node *node = dev->of_node;
> -
> -	/* Setup INTx */
> -	pcie->irq_domain = irq_domain_add_linear(node, PCI_NUM_INTX,
> -					&intx_domain_ops, pcie);
> -	if (!pcie->irq_domain) {
> -		dev_err(dev, "Failed to get a INTx IRQ domain\n");
> -		return -ENOMEM;
> -	}
> -
> -	return 0;
> -}
> -
>  static int altera_pcie_parse_dt(struct altera_pcie *pcie)
>  {
>  	struct device *dev = &pcie->pdev->dev;
> @@ -592,11 +563,10 @@ static int altera_pcie_probe(struct
> platform_device *pdev)
>  		return ret;
>  	}
>  
> -	ret = altera_pcie_init_irq_domain(pcie);
> -	if (ret) {
> -		dev_err(dev, "Failed creating IRQ Domain\n");
> -		return ret;
> -	}
> +	pcie->irq_domain = pci_host_alloc_intx_irqd(dev, pcie, true,
> NULL,
> +						    dev->of_node);
> +	if (IS_ERR(pcie->irq_domain))
> +		return PTR_ERR(pcie->irq_domain);
>  
>  	/* clear all interrupts */
>  	cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);

Re: [PATCH v4 01/10] PCI: Add pci_host_alloc_intx_irqd() for allocating IRQ domain

[Lorenzo Pieralisi]

On Thu, Jun 14, 2018 at 09:09:02AM +0200, Thomas Petazzoni wrote:

[...]

> > +				sizeof(struct irq_domain_ops), GFP_KERNEL);
> > +		if (!irqd_ops) {
> > +			err = -ENOMEM;
> > +			goto err_out;
> > +		}
> > +
> > +		irqd_ops->map = &pcie_intx_map;
> > +		if (general_xlate)
> > +			irqd_ops->xlate = &pci_irqd_intx_xlate;
> > +		intx_domain_ops = irqd_ops;
> > +	}
> > +#ifdef CONFIG_IRQ_DOMAIN
> > +	domain = irq_domain_add_linear(intc, PCI_NUM_INTX,
> > +				       intx_domain_ops, host);
> > +#endif
> 
> Isn't it a bit weird to have this in the middle of this function ?
> Should you instead declare a stub pci_host_alloc_intx_irqd() function
> in the header file if CONFIG_IRQ_DOMAIN is not enabled ?
> 
> I.e in pci.h:
> 
> #ifdef CONFIG_IRQ_DOMAIN
> struct irq_domain *pci_host_alloc_intx_irqd(struct device *dev,
> 				void *host, bool general_xlate,
> 				const struct irq_domain_ops *intx_domain_ops,
> 				struct device_node *local_intc);
> #else
> static inline struct irq_domain *pci_host_alloc_intx_irqd(struct device *dev,
> 	void *host, bool general_xlate, const struct irq_domain_ops *intx_domain_ops,
> 	struct device_node *local_intc)
> {
> 	return PTR_ERR(-EINVAL);
> }
> #endif
> 
> or something like that.

Yes, he should.

> Finally a purely subjective and personal opinion: I'm not a big fan of
> this boolean that tells whether the ->xlate hook should be populated or
> not. I'm not sure if it makes sense for this function to create the
> irqdomain_ops altogether, perhaps it should be mandatory for the
> irq_domain_ops to be provided by the caller. The problem with the
> boolean general_xlate is that it is an endless story of additional
> booleans to tweak more stuff in the irq_domain_ops structure. But of
> course that's just a personal view, and my view doesn't really count
> here :-)

Your view does count here and I 100% agree with you. I do not like the
->xlate hack (that is just there to make things work on systems with
broken DTS files and bindings for legacy IRQ mappings) and this patch
would just institutionalize it at PCI API level. So, if we *do* want to
consolidate code, the domain ops should be passed in, as you mentioned.

Thanks,
Lorenzo