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

Message ID 1528940057-183252-1-git-send-email-shawn.lin@rock-chips.com
State Changes Requested
Delegated to: Lorenzo Pieralisi
Headers show
Series
  • Add new helper to allocate IRQ domain for host drivers
Related show

Commit Message

Shawn Lin June 14, 2018, 1:34 a.m.
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(+)

Comments

Thomas Petazzoni June 14, 2018, 7:09 a.m. | #1
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
Lorenzo Pieralisi July 19, 2018, 11:37 a.m. | #2
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

Patch

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;