[PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

Hierarchical IRQ domains can be used to stack different IRQ
controllers on top of each other.

Bring hierarchical IRQ domains into the GPIOLIB core with the
following basic idea:

Drivers that need their interrupts handled hierarchically
specify a callback to translate the child hardware IRQ and
IRQ type for each GPIO offset to a parent hardware IRQ and
parent hardware IRQ type.

Users have to pass the callback, fwnode, and parent irqdomain
before calling gpiochip_irqchip_add().

We use the new method of just filling in the struct
gpio_irq_chip before adding the gpiochip for all hierarchical
irqchips of this type.

The code path for device tree is pretty straight-forward,
while the code path for old boardfiles or anything else will
be more convoluted requireing upfront allocation of the
interrupts when adding the chip.

One specific use-case where this can be useful is if a power
management controller has top-level controls for wakeup
interrupts. In such cases, the power management controller can
be a parent to other interrupt controllers and program
additional registers when an IRQ has its wake capability
enabled or disabled.

The hierarchical irqchip helper code will only be available
when IRQ_DOMAIN_HIERARCHY is selected to GPIO chips using
this should select or depend on that symbol. When using
hierarchical IRQs, the parent interrupt controller must
also be hierarchical all the way up to the top interrupt
controller wireing directly into the CPU, so on systems
that do not have this we can get rid of all the extra
code for supporting hierarchical irqs.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Lina Iyer <ilina@codeaurora.org>
Cc: Jon Hunter <jonathanh@nvidia.com>
Cc: Sowjanya Komatineni <skomatineni@nvidia.com>
Cc: Bitan Biswas <bbiswas@nvidia.com>
Cc: linux-tegra@vger.kernel.org
Cc: David Daney <david.daney@cavium.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Brian Masney <masneyb@onstation.org>
Signed-off-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog RFC->v1:
- Tested on real hardware
- Incorporate Thierry's idea to have a translation callback.
  He was right about this approach, I was wrong in insisting
  on IRQ maps.
---
 Documentation/driver-api/gpio/driver.rst | 120 ++++++++--
 drivers/gpio/gpiolib.c                   | 285 ++++++++++++++++++++++-
 include/linux/gpio/driver.h              |  46 ++++
 3 files changed, 426 insertions(+), 25 deletions(-)

diff --git a/Documentation/driver-api/gpio/driver.rst b/Documentation/driver-api/gpio/driver.rst
index 1ce7fcd0f989..3099c7fbefdb 100644
--- a/Documentation/driver-api/gpio/driver.rst
+++ b/Documentation/driver-api/gpio/driver.rst
@@ -259,7 +259,7 @@ most often cascaded off a parent interrupt controller, and in some special
 cases the GPIO logic is melded with a SoC's primary interrupt controller.
 
 The IRQ portions of the GPIO block are implemented using an irq_chip, using
-the header <linux/irq.h>. So basically such a driver is utilizing two sub-
+the header <linux/irq.h>. So this combined driver is utilizing two sub-
 systems simultaneously: gpio and irq.
 
 It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
@@ -391,14 +391,108 @@ Infrastructure helpers for GPIO irqchips
 ----------------------------------------
 
 To help out in handling the set-up and management of GPIO irqchips and the
-associated irqdomain and resource allocation callbacks, the gpiolib has
-some helpers that can be enabled by selecting the GPIOLIB_IRQCHIP Kconfig
-symbol:
-
-- gpiochip_irqchip_add(): adds a chained cascaded irqchip to a gpiochip. It
-  will pass the struct gpio_chip* for the chip to all IRQ callbacks, so the
-  callbacks need to embed the gpio_chip in its state container and obtain a
-  pointer to the container using container_of().
+associated irqdomain and resource allocation callbacks. These are activated
+by selecting the Kconfig symbol GPIOLIB_IRQCHIP. If the symbol
+IRQ_DOMAIN_HIERARCHY is also selected, hierarchical helpers will also be
+provided. A big portion of overhead code will be managed by gpiolib,
+under the assumption that your interrupts are 1-to-1-mapped to the
+GPIO line index:
+
+  GPIO line offset   Hardware IRQ
+  0                  0
+  1                  1
+  2                  2
+  ...                ...
+  ngpio-1            ngpio-1
+
+If some GPIO lines do not have corresponding IRQs, the bitmask valid_mask
+and the flag need_valid_mask in gpio_irq_chip can be used to mask off some
+lines as invalid for associating with IRQs.
+
+The preferred way to set up the helpers is to fill in the
+struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip.
+If you do this, the additional irq_chip will be set up by gpiolib at the
+same time as setting up the rest of the GPIO functionality. The following
+is a typical example of a cascaded interrupt handler using gpio_irq_chip:
+
+  /* Typical state container with dynamic irqchip */
+  struct my_gpio {
+      struct gpio_chip gc;
+      struct irq_chip irq;
+  };
+
+  int irq; /* from platform etc */
+  struct my_gpio *g;
+  struct gpio_irq_chip *girq
+
+  /* Set up the irqchip dynamically */
+  g->irq.name = "my_gpio_irq";
+  g->irq.irq_ack = my_gpio_ack_irq;
+  g->irq.irq_mask = my_gpio_mask_irq;
+  g->irq.irq_unmask = my_gpio_unmask_irq;
+  g->irq.irq_set_type = my_gpio_set_irq_type;
+
+  /* Get a pointer to the gpio_irq_chip */
+  girq = &g->gc.irq;
+  girq->chip = &g->irq;
+  girq->parent_handler = ftgpio_gpio_irq_handler;
+  girq->num_parents = 1;
+  girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
+                               GFP_KERNEL);
+  if (!girq->parents)
+      return -ENOMEM;
+  girq->default_type = IRQ_TYPE_NONE;
+  girq->handler = handle_bad_irq;
+  girq->parents[0] = irq;
+
+  return devm_gpiochip_add_data(dev, &g->gc, g);
+
+The helper support using hierarchical interrupt controllers as well.
+In this case the typical set-up will look like this:
+
+  /* Typical state container with dynamic irqchip */
+  struct my_gpio {
+      struct gpio_chip gc;
+      struct irq_chip irq;
+      struct fwnode_handle *fwnode;
+  };
+
+  int irq; /* from platform etc */
+  struct my_gpio *g;
+  struct gpio_irq_chip *girq
+
+  /* Set up the irqchip dynamically */
+  g->irq.name = "my_gpio_irq";
+  g->irq.irq_ack = my_gpio_ack_irq;
+  g->irq.irq_mask = my_gpio_mask_irq;
+  g->irq.irq_unmask = my_gpio_unmask_irq;
+  g->irq.irq_set_type = my_gpio_set_irq_type;
+
+  /* Get a pointer to the gpio_irq_chip */
+  girq = &g->gc.irq;
+  girq->chip = &g->irq;
+  girq->default_type = IRQ_TYPE_NONE;
+  girq->handler = handle_bad_irq;
+  girq->fwnode = g->fwnode;
+  girq->parent_domain = parent;
+  girq->child_to_parent_hwirq = my_gpio_child_to_parent_hwirq;
+
+  return devm_gpiochip_add_data(dev, &g->gc, g);
+
+As you can see pretty similar, but you do not supply a parent handler for
+the IRQ, instead a parent irqdomain, an fwnode for the hardware and
+a funcion .child_to_parent_hwirq() that has the purpose of looking up
+the parent hardware irq from a child (i.e. this gpio chip) hardware irq.
+As always it is good to look at examples in the kernel tree for advice
+on how to find the required pieces.
+
+The old way of adding irqchips to gpiochips after registration is also still
+available but we try to move away from this:
+
+- DEPRECATED: gpiochip_irqchip_add(): adds a chained cascaded irqchip to a
+  gpiochip. It will pass the struct gpio_chip* for the chip to all IRQ
+  callbacks, so the callbacks need to embed the gpio_chip in its state
+  container and obtain a pointer to the container using container_of().
   (See Documentation/driver-model/design-patterns.txt)
 
 - gpiochip_irqchip_add_nested(): adds a nested cascaded irqchip to a gpiochip,
@@ -406,10 +500,10 @@ symbol:
   cascaded irq has to be handled by a threaded interrupt handler.
   Apart from that it works exactly like the chained irqchip.
 
-- gpiochip_set_chained_irqchip(): sets up a chained cascaded irq handler for a
-  gpio_chip from a parent IRQ and passes the struct gpio_chip* as handler
-  data. Notice that we pass is as the handler data, since the irqchip data is
-  likely used by the parent irqchip.
+- DEPRECATED: gpiochip_set_chained_irqchip(): sets up a chained cascaded irq
+  handler for a gpio_chip from a parent IRQ and passes the struct gpio_chip*
+  as handler data. Notice that we pass is as the handler data, since the
+  irqchip data is likely used by the parent irqchip.
 
 - gpiochip_set_nested_irqchip(): sets up a nested cascaded irq handler for a
   gpio_chip from a parent IRQ. As the parent IRQ has usually been
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e013d417a936..af72ffa02963 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1718,6 +1718,240 @@ void gpiochip_set_nested_irqchip(struct gpio_chip *gpiochip,
 }
 EXPORT_SYMBOL_GPL(gpiochip_set_nested_irqchip);
 
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+
+/**
+ * gpiochip_set_hierarchical_irqchip() - connects a hierarchical irqchip
+ * to a gpiochip
+ * @gc: the gpiochip to set the irqchip hierarchical handler to
+ * @irqchip: the irqchip to handle this level of the hierarchy, the interrupt
+ * will then percolate up to the parent
+ */
+static void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gc,
+					      struct irq_chip *irqchip)
+{
+	/* DT will deal with mapping each IRQ as we go along */
+	if (is_of_node(gc->irq.fwnode))
+		return;
+
+	/*
+	 * This is for legacy and boardfile "irqchip" fwnodes: allocate
+	 * irqs upfront instead of dynamically since we don't have the
+	 * dynamic type of allocation that hardware description languages
+	 * provide. Once all GPIO drivers using board files are gone from
+	 * the kernel we can delete this code, but for a transitional period
+	 * it is necessary to keep this around.
+	 */
+	if (is_fwnode_irqchip(gc->irq.fwnode)) {
+		int i;
+		int ret;
+
+		for (i = 0; i < gc->ngpio; i++) {
+			struct irq_fwspec fwspec;
+			unsigned int parent_hwirq;
+			unsigned int parent_type;
+			struct gpio_irq_chip *girq = &gc->irq;
+
+			/*
+			 * We call the child to parent translation function
+			 * only to check if the child IRQ is valid or not.
+			 * Just pick the rising edge type here as that is what
+			 * we likely need to support.
+			 */
+			ret = girq->child_to_parent_hwirq(gc, i,
+							  IRQ_TYPE_EDGE_RISING,
+							  &parent_hwirq,
+							  &parent_type);
+			if (ret) {
+				chip_err(gc, "skip set-up on hwirq %d\n",
+					 i);
+				continue;
+			}
+
+			fwspec.fwnode = gc->irq.fwnode;
+			/* This is the hwirq for the GPIO line side of things */
+			fwspec.param[0] = i;
+			/* Just pick something */
+			fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+			fwspec.param_count = 2;
+			ret = __irq_domain_alloc_irqs(gc->irq.domain,
+						      /* just pick something */
+						      -1,
+						      1,
+						      NUMA_NO_NODE,
+						      &fwspec,
+						      false,
+						      NULL);
+			if (ret < 0) {
+				chip_err(gc,
+					 "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n",
+					 i, parent_hwirq,
+					 ret);
+			}
+		}
+	}
+
+	chip_err(gc, "%s unknown fwnode type proceed anyway\n", __func__);
+
+	return;
+}
+
+static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d,
+						   struct irq_fwspec *fwspec,
+						   unsigned long *hwirq,
+						   unsigned int *type)
+{
+	/* We support standard DT translation */
+	if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
+		return irq_domain_translate_twocell(d, fwspec, hwirq, type);
+	}
+
+	/* This is for board files and others not using DT */
+	if (is_fwnode_irqchip(fwspec->fwnode)) {
+		int ret;
+
+		ret = irq_domain_translate_twocell(d, fwspec, hwirq, type);
+		if (ret)
+			return ret;
+		WARN_ON(*type == IRQ_TYPE_NONE);
+		return 0;
+	}
+	return -EINVAL;
+}
+
+static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
+					       unsigned int irq,
+					       unsigned int nr_irqs,
+					       void *data)
+{
+	struct gpio_chip *gc = d->host_data;
+	irq_hw_number_t hwirq;
+	unsigned int type = IRQ_TYPE_NONE;
+	struct irq_fwspec *fwspec = data;
+	int ret;
+	int i;
+
+	chip_info(gc, "called %s\n", __func__);
+
+	ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	chip_info(gc, "allocate IRQ %d..%d, hwirq %lu..%lu\n",
+		  irq, irq + nr_irqs - 1,
+		  hwirq, hwirq + nr_irqs - 1);
+
+	for (i = 0; i < nr_irqs; i++) {
+		struct irq_fwspec parent_fwspec;
+		unsigned int parent_hwirq;
+		unsigned int parent_type;
+		struct gpio_irq_chip *girq = &gc->irq;
+
+		ret = girq->child_to_parent_hwirq(gc, hwirq, type,
+						  &parent_hwirq, &parent_type);
+		if (ret) {
+			chip_err(gc, "can't look up hwirq %lu\n", hwirq);
+			return ret;
+		}
+		chip_info(gc, "found parent hwirq %u\n", parent_hwirq);
+
+		/*
+		 * We set handle_bad_irq because the .set_type() should
+		 * always be invoked and set the right type of handler.
+		 */
+		irq_domain_set_info(d,
+				    irq + i,
+				    hwirq + i,
+				    gc->irq.chip,
+				    gc,
+				    handle_bad_irq,
+				    NULL, NULL);
+		irq_set_probe(irq + i);
+
+		/*
+		 * Create a IRQ fwspec to send up to the parent irqdomain:
+		 * specify the hwirq we address on the parent and tie it
+		 * all together up the chain.
+		 */
+		parent_fwspec.fwnode = d->parent->fwnode;
+		parent_fwspec.param_count = 2;
+		parent_fwspec.param[0] = parent_hwirq;
+		/* This parent only handles asserted level IRQs */
+		parent_fwspec.param[1] = parent_type;
+		chip_info(gc, "alloc_irqs_parent for %d parent hwirq %d\n",
+			  irq + i, parent_hwirq);
+		ret = irq_domain_alloc_irqs_parent(d, irq + i, 1,
+						   &parent_fwspec);
+		if (ret)
+			chip_err(gc,
+				 "failed to allocate parent hwirq %d for hwirq %lu\n",
+				 parent_hwirq, hwirq);
+	}
+
+	return 0;
+}
+
+static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = {
+	.activate = gpiochip_irq_domain_activate,
+	.deactivate = gpiochip_irq_domain_deactivate,
+	.translate = gpiochip_hierarchy_irq_domain_translate,
+	.alloc = gpiochip_hierarchy_irq_domain_alloc,
+	.free = irq_domain_free_irqs_common,
+};
+
+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
+{
+	if (!gc->irq.parent_domain) {
+		chip_err(gc, "missing parent irqdomain\n");
+		return -EINVAL;
+	}
+
+	if (!gc->irq.parent_domain ||
+	    !gc->irq.child_to_parent_hwirq ||
+	    !gc->irq.fwnode) {
+		chip_err(gc, "missing irqdomain vital data\n");
+		return -EINVAL;
+	}
+
+	gc->irq.domain = irq_domain_create_hierarchy(
+		gc->irq.parent_domain,
+		IRQ_DOMAIN_FLAG_HIERARCHY,
+		gc->ngpio,
+		gc->irq.fwnode,
+		&gpiochip_hierarchy_domain_ops,
+		gc);
+
+	if (!gc->irq.domain) {
+		chip_err(gc, "failed to add hierarchical domain\n");
+		return -EINVAL;
+	}
+
+	gpiochip_set_hierarchical_irqchip(gc, gc->irq.chip);
+
+	chip_info(gc, "set up hierarchical irqdomain\n");
+
+	return 0;
+}
+
+static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)
+{
+	return !!gc->irq.parent_domain;
+}
+
+#else
+
+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
+{
+	return -EINVAL;
+}
+
+static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)
+{
+	return false;
+}
+
+#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
+
 /**
  * gpiochip_irq_map() - maps an IRQ into a GPIO irqchip
  * @d: the irqdomain used by this irqchip
@@ -1786,6 +2020,11 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
 	.xlate	= irq_domain_xlate_twocell,
 };
 
+/*
+ * TODO: move these activate/deactivate in under the hierarchicial
+ * irqchip implementation as static once SPMI and SSBI (all external
+ * users) are phased over.
+ */
 /**
  * gpiochip_irq_domain_activate() - Lock a GPIO to be used as an IRQ
  * @domain: The IRQ domain used by this IRQ chip
@@ -1825,10 +2064,23 @@ EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate);
 
 static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 {
+	struct irq_domain *domain = chip->irq.domain;
+
 	if (!gpiochip_irqchip_irq_valid(chip, offset))
 		return -ENXIO;
 
-	return irq_create_mapping(chip->irq.domain, offset);
+	if (irq_domain_is_hierarchy(domain)) {
+		struct irq_fwspec spec;
+
+		spec.fwnode = domain->fwnode;
+		spec.param_count = 2;
+		spec.param[0] = offset;
+		spec.param[1] = IRQ_TYPE_NONE;
+
+		return irq_create_fwspec_mapping(&spec);
+	}
+
+	return irq_create_mapping(domain, offset);
 }
 
 static int gpiochip_irq_reqres(struct irq_data *d)
@@ -1905,7 +2157,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 				struct lock_class_key *request_key)
 {
 	struct irq_chip *irqchip = gpiochip->irq.chip;
-	const struct irq_domain_ops *ops;
+	const struct irq_domain_ops *ops = NULL;
 	struct device_node *np;
 	unsigned int type;
 	unsigned int i;
@@ -1941,16 +2193,25 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
 	gpiochip->irq.lock_key = lock_key;
 	gpiochip->irq.request_key = request_key;
 
-	if (gpiochip->irq.domain_ops)
-		ops = gpiochip->irq.domain_ops;
-	else
-		ops = &gpiochip_domain_ops;
-
-	gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
-						     gpiochip->irq.first,
-						     ops, gpiochip);
-	if (!gpiochip->irq.domain)
-		return -EINVAL;
+	/* If a parent irqdomain is provided, let's build a hierarchy */
+	if (gpiochip_hierarchy_is_hierarchical(gpiochip)) {
+		int ret = gpiochip_hierarchy_add_domain(gpiochip);
+		if (ret)
+			return ret;
+	} else {
+		/* Some drivers provide custom irqdomain ops */
+		if (gpiochip->irq.domain_ops)
+			ops = gpiochip->irq.domain_ops;
+
+		if (!ops)
+			ops = &gpiochip_domain_ops;
+		gpiochip->irq.domain = irq_domain_add_simple(np,
+			gpiochip->ngpio,
+			gpiochip->irq.first,
+			ops, gpiochip);
+		if (!gpiochip->irq.domain)
+			return -EINVAL;
+	}
 
 	if (gpiochip->irq.parent_handler) {
 		void *data = gpiochip->irq.parent_handler_data ?: gpiochip;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index a1d273c96016..e32d02cb2d08 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -22,6 +22,9 @@ enum gpiod_flags;
 #ifdef CONFIG_GPIOLIB
 
 #ifdef CONFIG_GPIOLIB_IRQCHIP
+
+struct gpio_chip;
+
 /**
  * struct gpio_irq_chip - GPIO interrupt controller
  */
@@ -48,6 +51,49 @@ struct gpio_irq_chip {
 	 */
 	const struct irq_domain_ops *domain_ops;
 
+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
+	/**
+	 * @fwnode:
+	 *
+	 * Firmware node corresponding to this gpiochip/irqchip, necessary
+	 * for hierarchical irqdomain support.
+	 */
+	struct fwnode_handle *fwnode;
+
+	/**
+	 * @parent_domain:
+	 *
+	 * If non-NULL, will be set as the parent of this GPIO interrupt
+	 * controller's IRQ domain to establish a hierarchical interrupt
+	 * domain. The presence of this will activate the hierarchical
+	 * interrupt support.
+	 */
+	struct irq_domain *parent_domain;
+
+	/**
+	 * @child_to_parent_hwirq:
+	 *
+	 * This callback translates a child hardware IRQ offset to a parent
+	 * hardware IRQ offset on a hierarchical interrupt chip. The child
+	 * hardware IRQs correspond to the GPIO index 0..ngpio-1 (see the
+	 * ngpio field of struct gpio_chip) and the corresponding parent
+	 * hardware IRQ and type (such as IRQ_TYPE_*) shall be returned by
+	 * the driver. The driver can calculate this from an offset or using
+	 * a lookup table or whatever method is best for this chip. Return
+	 * 0 on successful translation in the driver.
+	 *
+	 * If some ranges of hardware IRQs do not have a corresponding parent
+	 * HWIRQ, return -EINVAL, but also make sure to fill in @valid_mask and
+	 * @need_valid_mask to make these GPIO lines unavailable for
+	 * translation.
+	 */
+	int (*child_to_parent_hwirq)(struct gpio_chip *chip,
+				     unsigned int child_hwirq,
+				     unsigned int child_type,
+				     unsigned int *parent_hwirq,
+				     unsigned int *parent_type);
+#endif
+
 	/**
 	 * @handler:
 	 *
-- 
2.21.0

[PATCH 2/4 v1] gpio: ixp4xx: Convert to hieararchical GPIOLIB_IRQCHIP

[Linus Walleij]

This modifies the IXP4xx driver to use the new helpers
to handle the remapping of parent to child hardware irqs
in the gpiolib core.

This pulls the majority of the code out of the driver
and use the generic code in gpiolib.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Cc: Lina Iyer <ilina@codeaurora.org>
Cc: Jon Hunter <jonathanh@nvidia.com>
Cc: Sowjanya Komatineni <skomatineni@nvidia.com>
Cc: Bitan Biswas <bbiswas@nvidia.com>
Cc: linux-tegra@vger.kernel.org
Cc: Thierry Reding <treding@nvidia.com>
Cc: Brian Masney <masneyb@onstation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog RFC->v1:
- Fixed some bugs in dereferencing the gpio_chip first
  from the irqchip data, then dereference the
  local state container from the gpio_chip
- Adapted to changes in the core patch like provide
  as translation callback rather than a table.
- Tested on the Linksys NSLU2 and works like a charm
---
 drivers/gpio/Kconfig       |   2 +-
 drivers/gpio/gpio-ixp4xx.c | 277 +++++++++----------------------------
 2 files changed, 63 insertions(+), 216 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 8023d03ec362..72027b0c5bf4 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -291,7 +291,7 @@ config GPIO_IXP4XX
 	depends on ARM # For <asm/mach-types.h>
 	depends on ARCH_IXP4XX
 	select GPIO_GENERIC
-	select IRQ_DOMAIN
+	select GPIOLIB_IRQCHIP
 	select IRQ_DOMAIN_HIERARCHY
 	help
 	  Say yes here to support the GPIO functionality of a number of Intel
diff --git a/drivers/gpio/gpio-ixp4xx.c b/drivers/gpio/gpio-ixp4xx.c
index 670c2a85a35b..8bd23e80c61f 100644
--- a/drivers/gpio/gpio-ixp4xx.c
+++ b/drivers/gpio/gpio-ixp4xx.c
@@ -47,7 +47,6 @@
  * @dev: containing device for this instance
  * @fwnode: the fwnode for this GPIO chip
  * @gc: gpiochip for this instance
- * @domain: irqdomain for this chip instance
  * @base: remapped I/O-memory base
  * @irq_edge: Each bit represents an IRQ: 1: edge-triggered,
  * 0: level triggered
@@ -56,48 +55,22 @@ struct ixp4xx_gpio {
 	struct device *dev;
 	struct fwnode_handle *fwnode;
 	struct gpio_chip gc;
-	struct irq_domain *domain;
 	void __iomem *base;
 	unsigned long long irq_edge;
 };
 
-/**
- * struct ixp4xx_gpio_map - IXP4 GPIO to parent IRQ map
- * @gpio_offset: offset of the IXP4 GPIO line
- * @parent_hwirq: hwirq on the parent IRQ controller
- */
-struct ixp4xx_gpio_map {
-	int gpio_offset;
-	int parent_hwirq;
-};
-
-/* GPIO lines 0..12 have corresponding IRQs, GPIOs 13..15 have no IRQs */
-const struct ixp4xx_gpio_map ixp4xx_gpiomap[] = {
-	{ .gpio_offset = 0, .parent_hwirq = 6 },
-	{ .gpio_offset = 1, .parent_hwirq = 7 },
-	{ .gpio_offset = 2, .parent_hwirq = 19 },
-	{ .gpio_offset = 3, .parent_hwirq = 20 },
-	{ .gpio_offset = 4, .parent_hwirq = 21 },
-	{ .gpio_offset = 5, .parent_hwirq = 22 },
-	{ .gpio_offset = 6, .parent_hwirq = 23 },
-	{ .gpio_offset = 7, .parent_hwirq = 24 },
-	{ .gpio_offset = 8, .parent_hwirq = 25 },
-	{ .gpio_offset = 9, .parent_hwirq = 26 },
-	{ .gpio_offset = 10, .parent_hwirq = 27 },
-	{ .gpio_offset = 11, .parent_hwirq = 28 },
-	{ .gpio_offset = 12, .parent_hwirq = 29 },
-};
-
 static void ixp4xx_gpio_irq_ack(struct irq_data *d)
 {
-	struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct ixp4xx_gpio *g = gpiochip_get_data(gc);
 
 	__raw_writel(BIT(d->hwirq), g->base + IXP4XX_REG_GPIS);
 }
 
 static void ixp4xx_gpio_irq_unmask(struct irq_data *d)
 {
-	struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct ixp4xx_gpio *g = gpiochip_get_data(gc);
 
 	/* ACK when unmasking if not edge-triggered */
 	if (!(g->irq_edge & BIT(d->hwirq)))
@@ -108,7 +81,8 @@ static void ixp4xx_gpio_irq_unmask(struct irq_data *d)
 
 static int ixp4xx_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
-	struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct ixp4xx_gpio *g = gpiochip_get_data(gc);
 	int line = d->hwirq;
 	unsigned long flags;
 	u32 int_style;
@@ -187,122 +161,31 @@ static struct irq_chip ixp4xx_gpio_irqchip = {
 	.irq_set_type = ixp4xx_gpio_irq_set_type,
 };
 
-static int ixp4xx_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
-{
-	struct ixp4xx_gpio *g = gpiochip_get_data(gc);
-	struct irq_fwspec fwspec;
-
-	fwspec.fwnode = g->fwnode;
-	fwspec.param_count = 2;
-	fwspec.param[0] = offset;
-	fwspec.param[1] = IRQ_TYPE_NONE;
-
-	return irq_create_fwspec_mapping(&fwspec);
-}
-
-static int ixp4xx_gpio_irq_domain_translate(struct irq_domain *domain,
-					    struct irq_fwspec *fwspec,
-					    unsigned long *hwirq,
-					    unsigned int *type)
+static int ixp4xx_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
+					     unsigned int child,
+					     unsigned int child_type,
+					     unsigned int *parent,
+					     unsigned int *parent_type)
 {
-	int ret;
+	/* All these interrupts are level high in the CPU */
+	*parent_type = IRQ_TYPE_LEVEL_HIGH;
 
-	/* We support standard DT translation */
-	if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
-		return irq_domain_translate_twocell(domain, fwspec,
-						    hwirq, type);
+	/* GPIO lines 0..12 have dedicated IRQs */
+	if (child == 0) {
+		*parent = 6;
+		return 0;
 	}
-
-	/* This goes away when we transition to DT */
-	if (is_fwnode_irqchip(fwspec->fwnode)) {
-		ret = irq_domain_translate_twocell(domain, fwspec,
-						   hwirq, type);
-		if (ret)
-			return ret;
-		WARN_ON(*type == IRQ_TYPE_NONE);
+	if (child == 1) {
+		*parent = 7;
 		return 0;
 	}
-	return -EINVAL;
-}
-
-static int ixp4xx_gpio_irq_domain_alloc(struct irq_domain *d,
-					unsigned int irq, unsigned int nr_irqs,
-					void *data)
-{
-	struct ixp4xx_gpio *g = d->host_data;
-	irq_hw_number_t hwirq;
-	unsigned int type = IRQ_TYPE_NONE;
-	struct irq_fwspec *fwspec = data;
-	int ret;
-	int i;
-
-	ret = ixp4xx_gpio_irq_domain_translate(d, fwspec, &hwirq, &type);
-	if (ret)
-		return ret;
-
-	dev_dbg(g->dev, "allocate IRQ %d..%d, hwirq %lu..%lu\n",
-		irq, irq + nr_irqs - 1,
-		hwirq, hwirq + nr_irqs - 1);
-
-	for (i = 0; i < nr_irqs; i++) {
-		struct irq_fwspec parent_fwspec;
-		const struct ixp4xx_gpio_map *map;
-		int j;
-
-		/* Not all lines support IRQs */
-		for (j = 0; j < ARRAY_SIZE(ixp4xx_gpiomap); j++) {
-			map = &ixp4xx_gpiomap[j];
-			if (map->gpio_offset == hwirq)
-				break;
-		}
-		if (j == ARRAY_SIZE(ixp4xx_gpiomap)) {
-			dev_err(g->dev, "can't look up hwirq %lu\n", hwirq);
-			return -EINVAL;
-		}
-		dev_dbg(g->dev, "found parent hwirq %u\n", map->parent_hwirq);
-
-		/*
-		 * We set handle_bad_irq because the .set_type() should
-		 * always be invoked and set the right type of handler.
-		 */
-		irq_domain_set_info(d,
-				    irq + i,
-				    hwirq + i,
-				    &ixp4xx_gpio_irqchip,
-				    g,
-				    handle_bad_irq,
-				    NULL, NULL);
-		irq_set_probe(irq + i);
-
-		/*
-		 * Create a IRQ fwspec to send up to the parent irqdomain:
-		 * specify the hwirq we address on the parent and tie it
-		 * all together up the chain.
-		 */
-		parent_fwspec.fwnode = d->parent->fwnode;
-		parent_fwspec.param_count = 2;
-		parent_fwspec.param[0] = map->parent_hwirq;
-		/* This parent only handles asserted level IRQs */
-		parent_fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
-		dev_dbg(g->dev, "alloc_irqs_parent for %d parent hwirq %d\n",
-			irq + i, map->parent_hwirq);
-		ret = irq_domain_alloc_irqs_parent(d, irq + i, 1,
-						   &parent_fwspec);
-		if (ret)
-			dev_err(g->dev,
-				"failed to allocate parent hwirq %d for hwirq %lu\n",
-				map->parent_hwirq, hwirq);
+	if (child >= 2 && child <= 12) {
+		*parent = child + 17;
+		return 0;
 	}
-
-	return 0;
+	return -EINVAL;
 }
 
-static const struct irq_domain_ops ixp4xx_gpio_irqdomain_ops = {
-	.translate = ixp4xx_gpio_irq_domain_translate,
-	.alloc = ixp4xx_gpio_irq_domain_alloc,
-	.free = irq_domain_free_irqs_common,
-};
-
 static int ixp4xx_gpio_probe(struct platform_device *pdev)
 {
 	unsigned long flags;
@@ -311,8 +194,8 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
 	struct irq_domain *parent;
 	struct resource *res;
 	struct ixp4xx_gpio *g;
+	struct gpio_irq_chip *girq;
 	int ret;
-	int i;
 
 	g = devm_kzalloc(dev, sizeof(*g), GFP_KERNEL);
 	if (!g)
@@ -326,6 +209,35 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(g->base);
 	}
 
+	/*
+	 * When we convert to device tree we will simply look up the
+	 * parent irqdomain using irq_find_host(parent) as parent comes
+	 * from IRQCHIP_DECLARE(), then use of_node_to_fwnode() to get
+	 * the fwnode. For now we need this boardfile style code.
+	 */
+	if (np) {
+		struct device_node *irq_parent;
+
+		irq_parent = of_irq_find_parent(np);
+		if (!irq_parent) {
+			dev_err(dev, "no IRQ parent node\n");
+			return -ENODEV;
+		}
+		parent = irq_find_host(irq_parent);
+		if (!parent) {
+			dev_err(dev, "no IRQ parent domain\n");
+			return -ENODEV;
+		}
+		g->fwnode = of_node_to_fwnode(np);
+	} else {
+		parent = ixp4xx_get_irq_domain();
+		g->fwnode = irq_domain_alloc_fwnode(g->base);
+		if (!g->fwnode) {
+			dev_err(dev, "no domain base\n");
+			return -ENODEV;
+		}
+	}
+
 	/*
 	 * Make sure GPIO 14 and 15 are NOT used as clocks but GPIO on
 	 * specific machines.
@@ -360,7 +272,6 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
 		dev_err(dev, "unable to init generic GPIO\n");
 		return ret;
 	}
-	g->gc.to_irq = ixp4xx_gpio_to_irq;
 	g->gc.ngpio = 16;
 	g->gc.label = "IXP4XX_GPIO_CHIP";
 	/*
@@ -372,86 +283,22 @@ static int ixp4xx_gpio_probe(struct platform_device *pdev)
 	g->gc.parent = &pdev->dev;
 	g->gc.owner = THIS_MODULE;
 
+	girq = &g->gc.irq;
+	girq->chip = &ixp4xx_gpio_irqchip;
+	girq->fwnode = g->fwnode;
+	girq->parent_domain = parent;
+	girq->child_to_parent_hwirq = ixp4xx_gpio_child_to_parent_hwirq;
+	girq->handler = handle_bad_irq;
+	girq->default_type = IRQ_TYPE_NONE;
+
 	ret = devm_gpiochip_add_data(dev, &g->gc, g);
 	if (ret) {
 		dev_err(dev, "failed to add SoC gpiochip\n");
 		return ret;
 	}
 
-	/*
-	 * When we convert to device tree we will simply look up the
-	 * parent irqdomain using irq_find_host(parent) as parent comes
-	 * from IRQCHIP_DECLARE(), then use of_node_to_fwnode() to get
-	 * the fwnode. For now we need this boardfile style code.
-	 */
-	if (np) {
-		struct device_node *irq_parent;
-
-		irq_parent = of_irq_find_parent(np);
-		if (!irq_parent) {
-			dev_err(dev, "no IRQ parent node\n");
-			return -ENODEV;
-		}
-		parent = irq_find_host(irq_parent);
-		if (!parent) {
-			dev_err(dev, "no IRQ parent domain\n");
-			return -ENODEV;
-		}
-		g->fwnode = of_node_to_fwnode(np);
-	} else {
-		parent = ixp4xx_get_irq_domain();
-		g->fwnode = irq_domain_alloc_fwnode(g->base);
-		if (!g->fwnode) {
-			dev_err(dev, "no domain base\n");
-			return -ENODEV;
-		}
-	}
-	g->domain = irq_domain_create_hierarchy(parent,
-						IRQ_DOMAIN_FLAG_HIERARCHY,
-						ARRAY_SIZE(ixp4xx_gpiomap),
-						g->fwnode,
-						&ixp4xx_gpio_irqdomain_ops,
-						g);
-	if (!g->domain) {
-		irq_domain_free_fwnode(g->fwnode);
-		dev_err(dev, "no hierarchical irq domain\n");
-		return ret;
-	}
-
-	/*
-	 * After adding OF support, this is no longer needed: irqs
-	 * will be allocated for the respective fwnodes.
-	 */
-	if (!np) {
-		for (i = 0; i < ARRAY_SIZE(ixp4xx_gpiomap); i++) {
-			const struct ixp4xx_gpio_map *map = &ixp4xx_gpiomap[i];
-			struct irq_fwspec fwspec;
-
-			fwspec.fwnode = g->fwnode;
-			/* This is the hwirq for the GPIO line side of things */
-			fwspec.param[0] = map->gpio_offset;
-			fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
-			fwspec.param_count = 2;
-			ret = __irq_domain_alloc_irqs(g->domain,
-						      -1, /* just pick something */
-						      1,
-						      NUMA_NO_NODE,
-						      &fwspec,
-						      false,
-						      NULL);
-			if (ret < 0) {
-				irq_domain_free_fwnode(g->fwnode);
-				dev_err(dev,
-					"can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n",
-					map->gpio_offset, map->parent_hwirq,
-					ret);
-				return ret;
-			}
-		}
-	}
-
 	platform_set_drvdata(pdev, g);
-	dev_info(dev, "IXP4 GPIO @%p registered\n", g->base);
+	dev_info(dev, "IXP4 GPIO registered\n");
 
 	return 0;
 }
-- 
2.21.0

[PATCH 3/4 v1] RFT: gpio: thunderx: Switch to GPIOLIB_IRQCHIP

[Linus Walleij]

Use the new infrastructure for hierarchical irqchips in
gpiolib.

The major part of the rewrite was dues to the fact that
the driver was passing around a per-irq pointer to
struct thunderx_line * data container, and the central
handlers will assume struct gpio_chip * to be passed
to we need to use the hwirq as index to look up the
struct thunderx_line * for each IRQ.

The pushing and pop:ing of the irqdomain was confusing
because I've never seen this before, but I tried to
replicate it as best I could.

I have no chance to test or debug this so I need
help.

Cc: David Daney <david.daney@cavium.com>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Brian Masney <masneyb@onstation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/Kconfig         |   1 +
 drivers/gpio/gpio-thunderx.c | 163 ++++++++++++-----------------------
 2 files changed, 57 insertions(+), 107 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 72027b0c5bf4..ee34716a39aa 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -541,6 +541,7 @@ config GPIO_THUNDERX
 	tristate "Cavium ThunderX/OCTEON-TX GPIO"
 	depends on ARCH_THUNDER || (64BIT && COMPILE_TEST)
 	depends on PCI_MSI
+	select GPIOLIB_IRQCHIP
 	select IRQ_DOMAIN_HIERARCHY
 	select IRQ_FASTEOI_HIERARCHY_HANDLERS
 	help
diff --git a/drivers/gpio/gpio-thunderx.c b/drivers/gpio/gpio-thunderx.c
index 715371b5102a..ddad5c7ea617 100644
--- a/drivers/gpio/gpio-thunderx.c
+++ b/drivers/gpio/gpio-thunderx.c
@@ -53,7 +53,6 @@ struct thunderx_line {
 struct thunderx_gpio {
 	struct gpio_chip	chip;
 	u8 __iomem		*register_base;
-	struct irq_domain	*irqd;
 	struct msix_entry	*msix_entries;	/* per line MSI-X */
 	struct thunderx_line	*line_entries;	/* per line irq info */
 	raw_spinlock_t		lock;
@@ -283,54 +282,60 @@ static void thunderx_gpio_set_multiple(struct gpio_chip *chip,
 	}
 }
 
-static void thunderx_gpio_irq_ack(struct irq_data *data)
+static void thunderx_gpio_irq_ack(struct irq_data *d)
 {
-	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct thunderx_gpio *txgpio = gpiochip_get_data(gc);
 
 	writeq(GPIO_INTR_INTR,
-	       txline->txgpio->register_base + intr_reg(txline->line));
+	       txgpio->register_base + intr_reg(irqd_to_hwirq(d)));
 }
 
-static void thunderx_gpio_irq_mask(struct irq_data *data)
+static void thunderx_gpio_irq_mask(struct irq_data *d)
 {
-	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct thunderx_gpio *txgpio = gpiochip_get_data(gc);
 
 	writeq(GPIO_INTR_ENA_W1C,
-	       txline->txgpio->register_base + intr_reg(txline->line));
+	       txgpio->register_base + intr_reg(irqd_to_hwirq(d)));
 }
 
-static void thunderx_gpio_irq_mask_ack(struct irq_data *data)
+static void thunderx_gpio_irq_mask_ack(struct irq_data *d)
 {
-	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct thunderx_gpio *txgpio = gpiochip_get_data(gc);
 
 	writeq(GPIO_INTR_ENA_W1C | GPIO_INTR_INTR,
-	       txline->txgpio->register_base + intr_reg(txline->line));
+	       txgpio->register_base + intr_reg(irqd_to_hwirq(d)));
 }
 
-static void thunderx_gpio_irq_unmask(struct irq_data *data)
+static void thunderx_gpio_irq_unmask(struct irq_data *d)
 {
-	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct thunderx_gpio *txgpio = gpiochip_get_data(gc);
 
 	writeq(GPIO_INTR_ENA_W1S,
-	       txline->txgpio->register_base + intr_reg(txline->line));
+	       txgpio->register_base + intr_reg(irqd_to_hwirq(d)));
 }
 
-static int thunderx_gpio_irq_set_type(struct irq_data *data,
+static int thunderx_gpio_irq_set_type(struct irq_data *d,
 				      unsigned int flow_type)
 {
-	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
-	struct thunderx_gpio *txgpio = txline->txgpio;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct thunderx_gpio *txgpio = gpiochip_get_data(gc);
+	struct thunderx_line *txline =
+		&txgpio->line_entries[irqd_to_hwirq(d)];
 	u64 bit_cfg;
 
-	irqd_set_trigger_type(data, flow_type);
+	irqd_set_trigger_type(d, flow_type);
 
 	bit_cfg = txline->fil_bits | GPIO_BIT_CFG_INT_EN;
 
 	if (flow_type & IRQ_TYPE_EDGE_BOTH) {
-		irq_set_handler_locked(data, handle_fasteoi_ack_irq);
+		irq_set_handler_locked(d, handle_fasteoi_ack_irq);
 		bit_cfg |= GPIO_BIT_CFG_INT_TYPE;
 	} else {
-		irq_set_handler_locked(data, handle_fasteoi_mask_irq);
+		irq_set_handler_locked(d, handle_fasteoi_mask_irq);
 	}
 
 	raw_spin_lock(&txgpio->lock);
@@ -359,33 +364,6 @@ static void thunderx_gpio_irq_disable(struct irq_data *data)
 	irq_chip_disable_parent(data);
 }
 
-static int thunderx_gpio_irq_request_resources(struct irq_data *data)
-{
-	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
-	struct thunderx_gpio *txgpio = txline->txgpio;
-	int r;
-
-	r = gpiochip_lock_as_irq(&txgpio->chip, txline->line);
-	if (r)
-		return r;
-
-	r = irq_chip_request_resources_parent(data);
-	if (r)
-		gpiochip_unlock_as_irq(&txgpio->chip, txline->line);
-
-	return r;
-}
-
-static void thunderx_gpio_irq_release_resources(struct irq_data *data)
-{
-	struct thunderx_line *txline = irq_data_get_irq_chip_data(data);
-	struct thunderx_gpio *txgpio = txline->txgpio;
-
-	irq_chip_release_resources_parent(data);
-
-	gpiochip_unlock_as_irq(&txgpio->chip, txline->line);
-}
-
 /*
  * Interrupts are chained from underlying MSI-X vectors.  We have
  * these irq_chip functions to be able to handle level triggering
@@ -402,48 +380,22 @@ static struct irq_chip thunderx_gpio_irq_chip = {
 	.irq_unmask		= thunderx_gpio_irq_unmask,
 	.irq_eoi		= irq_chip_eoi_parent,
 	.irq_set_affinity	= irq_chip_set_affinity_parent,
-	.irq_request_resources	= thunderx_gpio_irq_request_resources,
-	.irq_release_resources	= thunderx_gpio_irq_release_resources,
 	.irq_set_type		= thunderx_gpio_irq_set_type,
 
 	.flags			= IRQCHIP_SET_TYPE_MASKED
 };
 
-static int thunderx_gpio_irq_translate(struct irq_domain *d,
-				       struct irq_fwspec *fwspec,
-				       irq_hw_number_t *hwirq,
-				       unsigned int *type)
+static int thunderx_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
+					       unsigned int child,
+					       unsigned int child_type,
+					       unsigned int *parent,
+					       unsigned int *parent_type)
 {
-	struct thunderx_gpio *txgpio = d->host_data;
-
-	if (WARN_ON(fwspec->param_count < 2))
-		return -EINVAL;
-	if (fwspec->param[0] >= txgpio->chip.ngpio)
-		return -EINVAL;
-	*hwirq = fwspec->param[0];
-	*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
-	return 0;
-}
-
-static int thunderx_gpio_irq_alloc(struct irq_domain *d, unsigned int virq,
-				   unsigned int nr_irqs, void *arg)
-{
-	struct thunderx_line *txline = arg;
+	struct thunderx_gpio *txgpio = gpiochip_get_data(gc);
 
-	return irq_domain_set_hwirq_and_chip(d, virq, txline->line,
-					     &thunderx_gpio_irq_chip, txline);
-}
-
-static const struct irq_domain_ops thunderx_gpio_irqd_ops = {
-	.alloc		= thunderx_gpio_irq_alloc,
-	.translate	= thunderx_gpio_irq_translate
-};
-
-static int thunderx_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
-{
-	struct thunderx_gpio *txgpio = gpiochip_get_data(chip);
-
-	return irq_find_mapping(txgpio->irqd, offset);
+	*parent = txgpio->base_msi + (2 * child);
+	*parent_type = IRQ_TYPE_LEVEL_HIGH;
+	return 0;
 }
 
 static int thunderx_gpio_probe(struct pci_dev *pdev,
@@ -453,6 +405,7 @@ static int thunderx_gpio_probe(struct pci_dev *pdev,
 	struct device *dev = &pdev->dev;
 	struct thunderx_gpio *txgpio;
 	struct gpio_chip *chip;
+	struct gpio_irq_chip *girq;
 	int ngpio, i;
 	int err = 0;
 
@@ -497,8 +450,8 @@ static int thunderx_gpio_probe(struct pci_dev *pdev,
 	}
 
 	txgpio->msix_entries = devm_kcalloc(dev,
-					  ngpio, sizeof(struct msix_entry),
-					  GFP_KERNEL);
+					    ngpio, sizeof(struct msix_entry),
+					    GFP_KERNEL);
 	if (!txgpio->msix_entries) {
 		err = -ENOMEM;
 		goto out;
@@ -539,27 +492,6 @@ static int thunderx_gpio_probe(struct pci_dev *pdev,
 	if (err < 0)
 		goto out;
 
-	/*
-	 * Push GPIO specific irqdomain on hierarchy created as a side
-	 * effect of the pci_enable_msix()
-	 */
-	txgpio->irqd = irq_domain_create_hierarchy(irq_get_irq_data(txgpio->msix_entries[0].vector)->domain,
-						   0, 0, of_node_to_fwnode(dev->of_node),
-						   &thunderx_gpio_irqd_ops, txgpio);
-	if (!txgpio->irqd) {
-		err = -ENOMEM;
-		goto out;
-	}
-
-	/* Push on irq_data and the domain for each line. */
-	for (i = 0; i < ngpio; i++) {
-		err = irq_domain_push_irq(txgpio->irqd,
-					  txgpio->msix_entries[i].vector,
-					  &txgpio->line_entries[i]);
-		if (err < 0)
-			dev_err(dev, "irq_domain_push_irq: %d\n", err);
-	}
-
 	chip->label = KBUILD_MODNAME;
 	chip->parent = dev;
 	chip->owner = THIS_MODULE;
@@ -574,11 +506,28 @@ static int thunderx_gpio_probe(struct pci_dev *pdev,
 	chip->set = thunderx_gpio_set;
 	chip->set_multiple = thunderx_gpio_set_multiple;
 	chip->set_config = thunderx_gpio_set_config;
-	chip->to_irq = thunderx_gpio_to_irq;
+	girq = &chip->irq;
+	girq->chip = &thunderx_gpio_irq_chip;
+	girq->fwnode = of_node_to_fwnode(dev->of_node);
+	girq->parent_domain =
+		irq_get_irq_data(txgpio->msix_entries[0].vector)->domain;
+	girq->child_to_parent_hwirq = thunderx_gpio_child_to_parent_hwirq;
+	girq->handler = handle_bad_irq;
+	girq->default_type = IRQ_TYPE_NONE;
+
 	err = devm_gpiochip_add_data(dev, chip, txgpio);
 	if (err)
 		goto out;
 
+	/* Push on irq_data and the domain for each line. */
+	for (i = 0; i < ngpio; i++) {
+		err = irq_domain_push_irq(chip->irq.domain,
+					  txgpio->msix_entries[i].vector,
+					  chip);
+		if (err < 0)
+			dev_err(dev, "irq_domain_push_irq: %d\n", err);
+	}
+
 	dev_info(dev, "ThunderX GPIO: %d lines with base %d.\n",
 		 ngpio, chip->base);
 	return 0;
@@ -593,10 +542,10 @@ static void thunderx_gpio_remove(struct pci_dev *pdev)
 	struct thunderx_gpio *txgpio = pci_get_drvdata(pdev);
 
 	for (i = 0; i < txgpio->chip.ngpio; i++)
-		irq_domain_pop_irq(txgpio->irqd,
+		irq_domain_pop_irq(txgpio->chip.irq.domain,
 				   txgpio->msix_entries[i].vector);
 
-	irq_domain_remove(txgpio->irqd);
+	irq_domain_remove(txgpio->chip.irq.domain);
 
 	pci_set_drvdata(pdev, NULL);
 }
-- 
2.21.0

[PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP

[Linus Walleij]

Use the new infrastructure for hierarchical irqchips in
gpiolib.

I have no chance to test or debug this so I need
help.

Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Thierry Reding <treding@nvidia.com>
Cc: Brian Masney <masneyb@onstation.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/Kconfig         |   1 +
 drivers/gpio/gpio-uniphier.c | 172 +++++++++--------------------------
 2 files changed, 45 insertions(+), 128 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ee34716a39aa..806d642498a6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -552,6 +552,7 @@ config GPIO_UNIPHIER
 	tristate "UniPhier GPIO support"
 	depends on ARCH_UNIPHIER || COMPILE_TEST
 	depends on OF_GPIO
+	select GPIOLIB_IRQCHIP
 	select IRQ_DOMAIN_HIERARCHY
 	help
 	  Say yes here to support UniPhier GPIOs.
diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c
index 93cdcc41e9fb..e960836b9c01 100644
--- a/drivers/gpio/gpio-uniphier.c
+++ b/drivers/gpio/gpio-uniphier.c
@@ -6,7 +6,6 @@
 #include <linux/bits.h>
 #include <linux/gpio/driver.h>
 #include <linux/irq.h>
-#include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -30,7 +29,6 @@
 struct uniphier_gpio_priv {
 	struct gpio_chip chip;
 	struct irq_chip irq_chip;
-	struct irq_domain *domain;
 	void __iomem *regs;
 	spinlock_t lock;
 	u32 saved_vals[0];
@@ -162,49 +160,33 @@ static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
 	}
 }
 
-static int uniphier_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+static void uniphier_gpio_irq_mask(struct irq_data *d)
 {
-	struct irq_fwspec fwspec;
-
-	if (offset < UNIPHIER_GPIO_IRQ_OFFSET)
-		return -ENXIO;
-
-	fwspec.fwnode = of_node_to_fwnode(chip->parent->of_node);
-	fwspec.param_count = 2;
-	fwspec.param[0] = offset - UNIPHIER_GPIO_IRQ_OFFSET;
-	/*
-	 * IRQ_TYPE_NONE is rejected by the parent irq domain. Set LEVEL_HIGH
-	 * temporarily. Anyway, ->irq_set_type() will override it later.
-	 */
-	fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
-
-	return irq_create_fwspec_mapping(&fwspec);
-}
-
-static void uniphier_gpio_irq_mask(struct irq_data *data)
-{
-	struct uniphier_gpio_priv *priv = data->chip_data;
-	u32 mask = BIT(data->hwirq);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct uniphier_gpio_priv *priv = gpiochip_get_data(gc);
+	u32 mask = BIT(d->hwirq);
 
 	uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, 0);
 
-	return irq_chip_mask_parent(data);
+	return irq_chip_mask_parent(d);
 }
 
-static void uniphier_gpio_irq_unmask(struct irq_data *data)
+static void uniphier_gpio_irq_unmask(struct irq_data *d)
 {
-	struct uniphier_gpio_priv *priv = data->chip_data;
-	u32 mask = BIT(data->hwirq);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct uniphier_gpio_priv *priv = gpiochip_get_data(gc);
+	u32 mask = BIT(d->hwirq);
 
 	uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, mask);
 
-	return irq_chip_unmask_parent(data);
+	return irq_chip_unmask_parent(d);
 }
 
-static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+static int uniphier_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
-	struct uniphier_gpio_priv *priv = data->chip_data;
-	u32 mask = BIT(data->hwirq);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct uniphier_gpio_priv *priv = gpiochip_get_data(gc);
+	u32 mask = BIT(d->hwirq);
 	u32 val = 0;
 
 	if (type == IRQ_TYPE_EDGE_BOTH) {
@@ -216,7 +198,7 @@ static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 	/* To enable both edge detection, the noise filter must be enabled. */
 	uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_FLT_EN, mask, val);
 
-	return irq_chip_set_type_parent(data, type);
+	return irq_chip_set_type_parent(d, type);
 }
 
 static int uniphier_gpio_irq_get_parent_hwirq(struct uniphier_gpio_priv *priv,
@@ -245,82 +227,6 @@ static int uniphier_gpio_irq_get_parent_hwirq(struct uniphier_gpio_priv *priv,
 	return -ENOENT;
 }
 
-static int uniphier_gpio_irq_domain_translate(struct irq_domain *domain,
-					      struct irq_fwspec *fwspec,
-					      unsigned long *out_hwirq,
-					      unsigned int *out_type)
-{
-	if (WARN_ON(fwspec->param_count < 2))
-		return -EINVAL;
-
-	*out_hwirq = fwspec->param[0];
-	*out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
-
-	return 0;
-}
-
-static int uniphier_gpio_irq_domain_alloc(struct irq_domain *domain,
-					  unsigned int virq,
-					  unsigned int nr_irqs, void *arg)
-{
-	struct uniphier_gpio_priv *priv = domain->host_data;
-	struct irq_fwspec parent_fwspec;
-	irq_hw_number_t hwirq;
-	unsigned int type;
-	int ret;
-
-	if (WARN_ON(nr_irqs != 1))
-		return -EINVAL;
-
-	ret = uniphier_gpio_irq_domain_translate(domain, arg, &hwirq, &type);
-	if (ret)
-		return ret;
-
-	ret = uniphier_gpio_irq_get_parent_hwirq(priv, hwirq);
-	if (ret < 0)
-		return ret;
-
-	/* parent is UniPhier AIDET */
-	parent_fwspec.fwnode = domain->parent->fwnode;
-	parent_fwspec.param_count = 2;
-	parent_fwspec.param[0] = ret;
-	parent_fwspec.param[1] = (type == IRQ_TYPE_EDGE_BOTH) ?
-						IRQ_TYPE_EDGE_FALLING : type;
-
-	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
-					    &priv->irq_chip, priv);
-	if (ret)
-		return ret;
-
-	return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
-}
-
-static int uniphier_gpio_irq_domain_activate(struct irq_domain *domain,
-					     struct irq_data *data, bool early)
-{
-	struct uniphier_gpio_priv *priv = domain->host_data;
-	struct gpio_chip *chip = &priv->chip;
-
-	return gpiochip_lock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET);
-}
-
-static void uniphier_gpio_irq_domain_deactivate(struct irq_domain *domain,
-						struct irq_data *data)
-{
-	struct uniphier_gpio_priv *priv = domain->host_data;
-	struct gpio_chip *chip = &priv->chip;
-
-	gpiochip_unlock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET);
-}
-
-static const struct irq_domain_ops uniphier_gpio_irq_domain_ops = {
-	.alloc = uniphier_gpio_irq_domain_alloc,
-	.free = irq_domain_free_irqs_common,
-	.activate = uniphier_gpio_irq_domain_activate,
-	.deactivate = uniphier_gpio_irq_domain_deactivate,
-	.translate = uniphier_gpio_irq_domain_translate,
-};
-
 static void uniphier_gpio_hw_init(struct uniphier_gpio_priv *priv)
 {
 	/*
@@ -338,6 +244,26 @@ static unsigned int uniphier_gpio_get_nbanks(unsigned int ngpio)
 	return DIV_ROUND_UP(ngpio, UNIPHIER_GPIO_LINES_PER_BANK);
 }
 
+static int uniphier_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
+					       unsigned int child,
+					       unsigned int child_type,
+					       unsigned int *parent,
+					       unsigned int *parent_type)
+{
+	struct uniphier_gpio_priv *priv = gpiochip_get_data(gc);
+	int ret;
+
+	ret = uniphier_gpio_irq_get_parent_hwirq(priv, child);
+	if (ret < 0)
+		return ret;
+	*parent = ret;
+	if (child_type == IRQ_TYPE_EDGE_BOTH)
+		*parent_type = IRQ_TYPE_EDGE_FALLING;
+	else
+		*parent_type = child_type;
+	return 0;
+}
+
 static int uniphier_gpio_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -345,6 +271,7 @@ static int uniphier_gpio_probe(struct platform_device *pdev)
 	struct irq_domain *parent_domain;
 	struct uniphier_gpio_priv *priv;
 	struct gpio_chip *chip;
+	struct gpio_irq_chip *girq;
 	struct irq_chip *irq_chip;
 	unsigned int nregs;
 	u32 ngpios;
@@ -386,7 +313,6 @@ static int uniphier_gpio_probe(struct platform_device *pdev)
 	chip->get = uniphier_gpio_get;
 	chip->set = uniphier_gpio_set;
 	chip->set_multiple = uniphier_gpio_set_multiple;
-	chip->to_irq = uniphier_gpio_to_irq;
 	chip->base = -1;
 	chip->ngpio = ngpios;
 
@@ -398,34 +324,25 @@ static int uniphier_gpio_probe(struct platform_device *pdev)
 	irq_chip->irq_set_affinity = irq_chip_set_affinity_parent;
 	irq_chip->irq_set_type = uniphier_gpio_irq_set_type;
 
+	girq = &chip->irq;
+	girq->chip = irq_chip;
+	girq->fwnode = of_node_to_fwnode(dev->of_node);
+	girq->parent_domain = parent_domain;
+	girq->child_to_parent_hwirq = uniphier_gpio_child_to_parent_hwirq;
+	girq->handler = handle_bad_irq;
+	girq->default_type = IRQ_TYPE_NONE;
+
 	uniphier_gpio_hw_init(priv);
 
 	ret = devm_gpiochip_add_data(dev, chip, priv);
 	if (ret)
 		return ret;
 
-	priv->domain = irq_domain_create_hierarchy(
-					parent_domain, 0,
-					UNIPHIER_GPIO_IRQ_MAX_NUM,
-					of_node_to_fwnode(dev->of_node),
-					&uniphier_gpio_irq_domain_ops, priv);
-	if (!priv->domain)
-		return -ENOMEM;
-
 	platform_set_drvdata(pdev, priv);
 
 	return 0;
 }
 
-static int uniphier_gpio_remove(struct platform_device *pdev)
-{
-	struct uniphier_gpio_priv *priv = platform_get_drvdata(pdev);
-
-	irq_domain_remove(priv->domain);
-
-	return 0;
-}
-
 static int __maybe_unused uniphier_gpio_suspend(struct device *dev)
 {
 	struct uniphier_gpio_priv *priv = dev_get_drvdata(dev);
@@ -485,7 +402,6 @@ MODULE_DEVICE_TABLE(of, uniphier_gpio_match);
 
 static struct platform_driver uniphier_gpio_driver = {
 	.probe = uniphier_gpio_probe,
-	.remove = uniphier_gpio_remove,
 	.driver = {
 		.name = "uniphier-gpio",
 		.of_match_table = uniphier_gpio_match,
-- 
2.21.0

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Lina Iyer]

Thanks for the patch Linus. I was running into the warning in
gpiochip_set_irq_hooks(), because it was called from two places.
Hopefully, this will fix that as well. I will give it a try.

On Mon, Jun 24 2019 at 07:29 -0600, Linus Walleij wrote:
>Hierarchical IRQ domains can be used to stack different IRQ
>controllers on top of each other.
>
>Bring hierarchical IRQ domains into the GPIOLIB core with the
>following basic idea:
>
>Drivers that need their interrupts handled hierarchically
>specify a callback to translate the child hardware IRQ and
>IRQ type for each GPIO offset to a parent hardware IRQ and
>parent hardware IRQ type.
>
>Users have to pass the callback, fwnode, and parent irqdomain
>before calling gpiochip_irqchip_add().
>
>We use the new method of just filling in the struct
>gpio_irq_chip before adding the gpiochip for all hierarchical
>irqchips of this type.
>
>The code path for device tree is pretty straight-forward,
>while the code path for old boardfiles or anything else will
>be more convoluted requireing upfront allocation of the
>interrupts when adding the chip.
>
>One specific use-case where this can be useful is if a power
>management controller has top-level controls for wakeup
>interrupts. In such cases, the power management controller can
>be a parent to other interrupt controllers and program
>additional registers when an IRQ has its wake capability
>enabled or disabled.
>
>The hierarchical irqchip helper code will only be available
>when IRQ_DOMAIN_HIERARCHY is selected to GPIO chips using
>this should select or depend on that symbol. When using
>hierarchical IRQs, the parent interrupt controller must
>also be hierarchical all the way up to the top interrupt
>controller wireing directly into the CPU, so on systems
>that do not have this we can get rid of all the extra
>code for supporting hierarchical irqs.
>
>Cc: Thomas Gleixner <tglx@linutronix.de>
>Cc: Marc Zyngier <marc.zyngier@arm.com>
>Cc: Lina Iyer <ilina@codeaurora.org>
>Cc: Jon Hunter <jonathanh@nvidia.com>
>Cc: Sowjanya Komatineni <skomatineni@nvidia.com>
>Cc: Bitan Biswas <bbiswas@nvidia.com>
>Cc: linux-tegra@vger.kernel.org
>Cc: David Daney <david.daney@cavium.com>
>Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
>Cc: Brian Masney <masneyb@onstation.org>
>Signed-off-by: Thierry Reding <treding@nvidia.com>
>Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>---
>ChangeLog RFC->v1:
>- Tested on real hardware
>- Incorporate Thierry's idea to have a translation callback.
>  He was right about this approach, I was wrong in insisting
>  on IRQ maps.
>---
> Documentation/driver-api/gpio/driver.rst | 120 ++++++++--
> drivers/gpio/gpiolib.c                   | 285 ++++++++++++++++++++++-
> include/linux/gpio/driver.h              |  46 ++++
> 3 files changed, 426 insertions(+), 25 deletions(-)
>
>diff --git a/Documentation/driver-api/gpio/driver.rst b/Documentation/driver-api/gpio/driver.rst
>index 1ce7fcd0f989..3099c7fbefdb 100644
>--- a/Documentation/driver-api/gpio/driver.rst
>+++ b/Documentation/driver-api/gpio/driver.rst
>@@ -259,7 +259,7 @@ most often cascaded off a parent interrupt controller, and in some special
> cases the GPIO logic is melded with a SoC's primary interrupt controller.
>
> The IRQ portions of the GPIO block are implemented using an irq_chip, using
>-the header <linux/irq.h>. So basically such a driver is utilizing two sub-
>+the header <linux/irq.h>. So this combined driver is utilizing two sub-
> systems simultaneously: gpio and irq.
>
> It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
>@@ -391,14 +391,108 @@ Infrastructure helpers for GPIO irqchips
> ----------------------------------------
>
> To help out in handling the set-up and management of GPIO irqchips and the
>-associated irqdomain and resource allocation callbacks, the gpiolib has
>-some helpers that can be enabled by selecting the GPIOLIB_IRQCHIP Kconfig
>-symbol:
>-
>-- gpiochip_irqchip_add(): adds a chained cascaded irqchip to a gpiochip. It
>-  will pass the struct gpio_chip* for the chip to all IRQ callbacks, so the
>-  callbacks need to embed the gpio_chip in its state container and obtain a
>-  pointer to the container using container_of().
>+associated irqdomain and resource allocation callbacks. These are activated
>+by selecting the Kconfig symbol GPIOLIB_IRQCHIP. If the symbol
>+IRQ_DOMAIN_HIERARCHY is also selected, hierarchical helpers will also be
>+provided. A big portion of overhead code will be managed by gpiolib,
>+under the assumption that your interrupts are 1-to-1-mapped to the
>+GPIO line index:
>+
>+  GPIO line offset   Hardware IRQ
>+  0                  0
>+  1                  1
>+  2                  2
>+  ...                ...
>+  ngpio-1            ngpio-1
>+
>+If some GPIO lines do not have corresponding IRQs, the bitmask valid_mask
>+and the flag need_valid_mask in gpio_irq_chip can be used to mask off some
>+lines as invalid for associating with IRQs.
>+
>+The preferred way to set up the helpers is to fill in the
>+struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip.
>+If you do this, the additional irq_chip will be set up by gpiolib at the
>+same time as setting up the rest of the GPIO functionality. The following
>+is a typical example of a cascaded interrupt handler using gpio_irq_chip:
>+
>+  /* Typical state container with dynamic irqchip */
>+  struct my_gpio {
>+      struct gpio_chip gc;
>+      struct irq_chip irq;
>+  };
>+
>+  int irq; /* from platform etc */
>+  struct my_gpio *g;
>+  struct gpio_irq_chip *girq
>+
>+  /* Set up the irqchip dynamically */
>+  g->irq.name = "my_gpio_irq";
>+  g->irq.irq_ack = my_gpio_ack_irq;
>+  g->irq.irq_mask = my_gpio_mask_irq;
>+  g->irq.irq_unmask = my_gpio_unmask_irq;
>+  g->irq.irq_set_type = my_gpio_set_irq_type;
>+
>+  /* Get a pointer to the gpio_irq_chip */
>+  girq = &g->gc.irq;
>+  girq->chip = &g->irq;
>+  girq->parent_handler = ftgpio_gpio_irq_handler;
>+  girq->num_parents = 1;
>+  girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
>+                               GFP_KERNEL);
Could this be folded into the gpiolib?

>+  if (!girq->parents)
>+      return -ENOMEM;
>+  girq->default_type = IRQ_TYPE_NONE;
>+  girq->handler = handle_bad_irq;
>+  girq->parents[0] = irq;
>+
>+  return devm_gpiochip_add_data(dev, &g->gc, g);
>+
>+The helper support using hierarchical interrupt controllers as well.
>+In this case the typical set-up will look like this:
>+
>+  /* Typical state container with dynamic irqchip */
>+  struct my_gpio {
>+      struct gpio_chip gc;
>+      struct irq_chip irq;
>+      struct fwnode_handle *fwnode;
>+  };
>+
>+  int irq; /* from platform etc */
>+  struct my_gpio *g;
>+  struct gpio_irq_chip *girq
>+
>+  /* Set up the irqchip dynamically */
>+  g->irq.name = "my_gpio_irq";
>+  g->irq.irq_ack = my_gpio_ack_irq;
>+  g->irq.irq_mask = my_gpio_mask_irq;
>+  g->irq.irq_unmask = my_gpio_unmask_irq;
>+  g->irq.irq_set_type = my_gpio_set_irq_type;
>+
>+  /* Get a pointer to the gpio_irq_chip */
>+  girq = &g->gc.irq;
>+  girq->chip = &g->irq;
>+  girq->default_type = IRQ_TYPE_NONE;
>+  girq->handler = handle_bad_irq;
>+  girq->fwnode = g->fwnode;
>+  girq->parent_domain = parent;
>+  girq->child_to_parent_hwirq = my_gpio_child_to_parent_hwirq;
>+
Should be the necessary, if the driver implements it's own .alloc?

>+  return devm_gpiochip_add_data(dev, &g->gc, g);
>+
>+As you can see pretty similar, but you do not supply a parent handler for
>+the IRQ, instead a parent irqdomain, an fwnode for the hardware and
>+a funcion .child_to_parent_hwirq() that has the purpose of looking up
>+the parent hardware irq from a child (i.e. this gpio chip) hardware irq.
>+As always it is good to look at examples in the kernel tree for advice
>+on how to find the required pieces.
>+
>+The old way of adding irqchips to gpiochips after registration is also still
>+available but we try to move away from this:
>+
>+- DEPRECATED: gpiochip_irqchip_add(): adds a chained cascaded irqchip to a
>+  gpiochip. It will pass the struct gpio_chip* for the chip to all IRQ
>+  callbacks, so the callbacks need to embed the gpio_chip in its state
>+  container and obtain a pointer to the container using container_of().
>   (See Documentation/driver-model/design-patterns.txt)
>
> - gpiochip_irqchip_add_nested(): adds a nested cascaded irqchip to a gpiochip,
>@@ -406,10 +500,10 @@ symbol:
>   cascaded irq has to be handled by a threaded interrupt handler.
>   Apart from that it works exactly like the chained irqchip.
>
>-- gpiochip_set_chained_irqchip(): sets up a chained cascaded irq handler for a
>-  gpio_chip from a parent IRQ and passes the struct gpio_chip* as handler
>-  data. Notice that we pass is as the handler data, since the irqchip data is
>-  likely used by the parent irqchip.
>+- DEPRECATED: gpiochip_set_chained_irqchip(): sets up a chained cascaded irq
>+  handler for a gpio_chip from a parent IRQ and passes the struct gpio_chip*
>+  as handler data. Notice that we pass is as the handler data, since the
>+  irqchip data is likely used by the parent irqchip.
>
> - gpiochip_set_nested_irqchip(): sets up a nested cascaded irq handler for a
>   gpio_chip from a parent IRQ. As the parent IRQ has usually been
>diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>index e013d417a936..af72ffa02963 100644
>--- a/drivers/gpio/gpiolib.c
>+++ b/drivers/gpio/gpiolib.c
>@@ -1718,6 +1718,240 @@ void gpiochip_set_nested_irqchip(struct gpio_chip *gpiochip,
> }
> EXPORT_SYMBOL_GPL(gpiochip_set_nested_irqchip);
>
>+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>+
>+/**
>+ * gpiochip_set_hierarchical_irqchip() - connects a hierarchical irqchip
>+ * to a gpiochip
>+ * @gc: the gpiochip to set the irqchip hierarchical handler to
>+ * @irqchip: the irqchip to handle this level of the hierarchy, the interrupt
>+ * will then percolate up to the parent
>+ */
>+static void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gc,
>+					      struct irq_chip *irqchip)
>+{
>+	/* DT will deal with mapping each IRQ as we go along */
>+	if (is_of_node(gc->irq.fwnode))
>+		return;
>+
>+	/*
>+	 * This is for legacy and boardfile "irqchip" fwnodes: allocate
>+	 * irqs upfront instead of dynamically since we don't have the
>+	 * dynamic type of allocation that hardware description languages
>+	 * provide. Once all GPIO drivers using board files are gone from
>+	 * the kernel we can delete this code, but for a transitional period
>+	 * it is necessary to keep this around.
>+	 */
>+	if (is_fwnode_irqchip(gc->irq.fwnode)) {
>+		int i;
>+		int ret;
>+
>+		for (i = 0; i < gc->ngpio; i++) {
>+			struct irq_fwspec fwspec;
>+			unsigned int parent_hwirq;
>+			unsigned int parent_type;
>+			struct gpio_irq_chip *girq = &gc->irq;
>+
>+			/*
>+			 * We call the child to parent translation function
>+			 * only to check if the child IRQ is valid or not.
>+			 * Just pick the rising edge type here as that is what
>+			 * we likely need to support.
>+			 */
>+			ret = girq->child_to_parent_hwirq(gc, i,
>+							  IRQ_TYPE_EDGE_RISING,
>+							  &parent_hwirq,
>+							  &parent_type);
>+			if (ret) {
>+				chip_err(gc, "skip set-up on hwirq %d\n",
>+					 i);
>+				continue;
>+			}
>+
>+			fwspec.fwnode = gc->irq.fwnode;
>+			/* This is the hwirq for the GPIO line side of things */
>+			fwspec.param[0] = i;
>+			/* Just pick something */
>+			fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
>+			fwspec.param_count = 2;
>+			ret = __irq_domain_alloc_irqs(gc->irq.domain,
>+						      /* just pick something */
>+						      -1,
>+						      1,
>+						      NUMA_NO_NODE,
>+						      &fwspec,
>+						      false,
>+						      NULL);
>+			if (ret < 0) {
>+				chip_err(gc,
>+					 "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n",
>+					 i, parent_hwirq,
>+					 ret);
>+			}
>+		}
>+	}
>+
>+	chip_err(gc, "%s unknown fwnode type proceed anyway\n", __func__);
>+
>+	return;
>+}
>+
>+static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d,
>+						   struct irq_fwspec *fwspec,
>+						   unsigned long *hwirq,
>+						   unsigned int *type)
>+{
>+	/* We support standard DT translation */
>+	if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
>+		return irq_domain_translate_twocell(d, fwspec, hwirq, type);
>+	}
>+
>+	/* This is for board files and others not using DT */
>+	if (is_fwnode_irqchip(fwspec->fwnode)) {
>+		int ret;
>+
>+		ret = irq_domain_translate_twocell(d, fwspec, hwirq, type);
>+		if (ret)
>+			return ret;
>+		WARN_ON(*type == IRQ_TYPE_NONE);
>+		return 0;
>+	}
>+	return -EINVAL;
>+}
>+
>+static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
>+					       unsigned int irq,
>+					       unsigned int nr_irqs,
>+					       void *data)
>+{
>+	struct gpio_chip *gc = d->host_data;
>+	irq_hw_number_t hwirq;
>+	unsigned int type = IRQ_TYPE_NONE;
>+	struct irq_fwspec *fwspec = data;
>+	int ret;
>+	int i;
>+
>+	chip_info(gc, "called %s\n", __func__);
>+
>+	ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type);
>+	if (ret)
>+		return ret;
>+
>+	chip_info(gc, "allocate IRQ %d..%d, hwirq %lu..%lu\n",
>+		  irq, irq + nr_irqs - 1,
>+		  hwirq, hwirq + nr_irqs - 1);
>+
>+	for (i = 0; i < nr_irqs; i++) {
>+		struct irq_fwspec parent_fwspec;
>+		unsigned int parent_hwirq;
>+		unsigned int parent_type;
>+		struct gpio_irq_chip *girq = &gc->irq;
>+
>+		ret = girq->child_to_parent_hwirq(gc, hwirq, type,
>+						  &parent_hwirq, &parent_type);
>+		if (ret) {
>+			chip_err(gc, "can't look up hwirq %lu\n", hwirq);
>+			return ret;
>+		}
>+		chip_info(gc, "found parent hwirq %u\n", parent_hwirq);
>+
>+		/*
>+		 * We set handle_bad_irq because the .set_type() should
>+		 * always be invoked and set the right type of handler.
>+		 */
>+		irq_domain_set_info(d,
>+				    irq + i,
>+				    hwirq + i,
>+				    gc->irq.chip,
>+				    gc,
>+				    handle_bad_irq,
>+				    NULL, NULL);
>+		irq_set_probe(irq + i);
>+
>+		/*
>+		 * Create a IRQ fwspec to send up to the parent irqdomain:
>+		 * specify the hwirq we address on the parent and tie it
>+		 * all together up the chain.
>+		 */
>+		parent_fwspec.fwnode = d->parent->fwnode;
>+		parent_fwspec.param_count = 2;
>+		parent_fwspec.param[0] = parent_hwirq;
>+		/* This parent only handles asserted level IRQs */
>+		parent_fwspec.param[1] = parent_type;
>+		chip_info(gc, "alloc_irqs_parent for %d parent hwirq %d\n",
>+			  irq + i, parent_hwirq);
>+		ret = irq_domain_alloc_irqs_parent(d, irq + i, 1,
>+						   &parent_fwspec);
>+		if (ret)
>+			chip_err(gc,
>+				 "failed to allocate parent hwirq %d for hwirq %lu\n",
>+				 parent_hwirq, hwirq);
>+	}
>+
>+	return 0;
>+}
>+
>+static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = {
>+	.activate = gpiochip_irq_domain_activate,
>+	.deactivate = gpiochip_irq_domain_deactivate,
>+	.translate = gpiochip_hierarchy_irq_domain_translate,
>+	.alloc = gpiochip_hierarchy_irq_domain_alloc,
>+	.free = irq_domain_free_irqs_common,
>+};
>+
>+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
>+{
>+	if (!gc->irq.parent_domain) {
>+		chip_err(gc, "missing parent irqdomain\n");
>+		return -EINVAL;
>+	}
>+
>+	if (!gc->irq.parent_domain ||
>+	    !gc->irq.child_to_parent_hwirq ||
This should probably be validated if the .ops have not been set.

>+	    !gc->irq.fwnode) {
>+		chip_err(gc, "missing irqdomain vital data\n");
>+		return -EINVAL;
>+	}
>+
>+	gc->irq.domain = irq_domain_create_hierarchy(
>+		gc->irq.parent_domain,
>+		IRQ_DOMAIN_FLAG_HIERARCHY,
>+		gc->ngpio,
>+		gc->irq.fwnode,
>+		&gpiochip_hierarchy_domain_ops,
>+		gc);
>+
>+	if (!gc->irq.domain) {
>+		chip_err(gc, "failed to add hierarchical domain\n");
>+		return -EINVAL;
>+	}
>+
>+	gpiochip_set_hierarchical_irqchip(gc, gc->irq.chip);
>+
>+	chip_info(gc, "set up hierarchical irqdomain\n");
>+
>+	return 0;
>+}
>+
>+static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)
>+{
>+	return !!gc->irq.parent_domain;
>+}
>+
>+#else
>+
>+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
>+{
>+	return -EINVAL;
>+}
>+
>+static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)
>+{
>+	return false;
>+}
>+
>+#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
>+
> /**
>  * gpiochip_irq_map() - maps an IRQ into a GPIO irqchip
>  * @d: the irqdomain used by this irqchip
>@@ -1786,6 +2020,11 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
> 	.xlate	= irq_domain_xlate_twocell,
> };
>
>+/*
>+ * TODO: move these activate/deactivate in under the hierarchicial
>+ * irqchip implementation as static once SPMI and SSBI (all external
>+ * users) are phased over.
>+ */
> /**
>  * gpiochip_irq_domain_activate() - Lock a GPIO to be used as an IRQ
>  * @domain: The IRQ domain used by this IRQ chip
>@@ -1825,10 +2064,23 @@ EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate);
>
> static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
> {
>+	struct irq_domain *domain = chip->irq.domain;
>+
> 	if (!gpiochip_irqchip_irq_valid(chip, offset))
> 		return -ENXIO;
>
>-	return irq_create_mapping(chip->irq.domain, offset);
>+	if (irq_domain_is_hierarchy(domain)) {
>+		struct irq_fwspec spec;
>+
>+		spec.fwnode = domain->fwnode;
>+		spec.param_count = 2;
>+		spec.param[0] = offset;
>+		spec.param[1] = IRQ_TYPE_NONE;
>+
>+		return irq_create_fwspec_mapping(&spec);
>+	}
>+
>+	return irq_create_mapping(domain, offset);
> }
>
> static int gpiochip_irq_reqres(struct irq_data *d)
>@@ -1905,7 +2157,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
> 				struct lock_class_key *request_key)
> {
> 	struct irq_chip *irqchip = gpiochip->irq.chip;
>-	const struct irq_domain_ops *ops;
>+	const struct irq_domain_ops *ops = NULL;
> 	struct device_node *np;
> 	unsigned int type;
> 	unsigned int i;
>@@ -1941,16 +2193,25 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
> 	gpiochip->irq.lock_key = lock_key;
> 	gpiochip->irq.request_key = request_key;
>
>-	if (gpiochip->irq.domain_ops)
>-		ops = gpiochip->irq.domain_ops;
>-	else
>-		ops = &gpiochip_domain_ops;
>-
>-	gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
>-						     gpiochip->irq.first,
>-						     ops, gpiochip);
>-	if (!gpiochip->irq.domain)
>-		return -EINVAL;
>+	/* If a parent irqdomain is provided, let's build a hierarchy */
>+	if (gpiochip_hierarchy_is_hierarchical(gpiochip)) {
>+		int ret = gpiochip_hierarchy_add_domain(gpiochip);
>+		if (ret)
>+			return ret;
>+	} else {
>+		/* Some drivers provide custom irqdomain ops */
>+		if (gpiochip->irq.domain_ops)
>+			ops = gpiochip->irq.domain_ops;
>+
>+		if (!ops)
>+			ops = &gpiochip_domain_ops;
>+		gpiochip->irq.domain = irq_domain_add_simple(np,
>+			gpiochip->ngpio,
>+			gpiochip->irq.first,
>+			ops, gpiochip);
>+		if (!gpiochip->irq.domain)
>+			return -EINVAL;
>+	}
>
> 	if (gpiochip->irq.parent_handler) {
> 		void *data = gpiochip->irq.parent_handler_data ?: gpiochip;
>diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
>index a1d273c96016..e32d02cb2d08 100644
>--- a/include/linux/gpio/driver.h
>+++ b/include/linux/gpio/driver.h
>@@ -22,6 +22,9 @@ enum gpiod_flags;
> #ifdef CONFIG_GPIOLIB
>
> #ifdef CONFIG_GPIOLIB_IRQCHIP
>+
>+struct gpio_chip;
>+
> /**
>  * struct gpio_irq_chip - GPIO interrupt controller
>  */
>@@ -48,6 +51,49 @@ struct gpio_irq_chip {
> 	 */
> 	const struct irq_domain_ops *domain_ops;
>
>+#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
>+	/**
>+	 * @fwnode:
>+	 *
>+	 * Firmware node corresponding to this gpiochip/irqchip, necessary
>+	 * for hierarchical irqdomain support.
>+	 */
>+	struct fwnode_handle *fwnode;
>+
>+	/**
>+	 * @parent_domain:
>+	 *
>+	 * If non-NULL, will be set as the parent of this GPIO interrupt
>+	 * controller's IRQ domain to establish a hierarchical interrupt
>+	 * domain. The presence of this will activate the hierarchical
>+	 * interrupt support.
>+	 */
>+	struct irq_domain *parent_domain;
>+
>+	/**
>+	 * @child_to_parent_hwirq:
>+	 *
>+	 * This callback translates a child hardware IRQ offset to a parent
>+	 * hardware IRQ offset on a hierarchical interrupt chip. The child
>+	 * hardware IRQs correspond to the GPIO index 0..ngpio-1 (see the
>+	 * ngpio field of struct gpio_chip) and the corresponding parent
>+	 * hardware IRQ and type (such as IRQ_TYPE_*) shall be returned by
>+	 * the driver. The driver can calculate this from an offset or using
>+	 * a lookup table or whatever method is best for this chip. Return
>+	 * 0 on successful translation in the driver.
>+	 *
>+	 * If some ranges of hardware IRQs do not have a corresponding parent
>+	 * HWIRQ, return -EINVAL, but also make sure to fill in @valid_mask and
>+	 * @need_valid_mask to make these GPIO lines unavailable for
>+	 * translation.
>+	 */
>+	int (*child_to_parent_hwirq)(struct gpio_chip *chip,
>+				     unsigned int child_hwirq,
>+				     unsigned int child_type,
>+				     unsigned int *parent_hwirq,
>+				     unsigned int *parent_type);
Would irq_fwspec(s) be better than passing all these arguments around?

Thanks,
Lina

>+#endif
>+
> 	/**
> 	 * @handler:
> 	 *
>--
>2.21.0
>

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Lina Iyer]

On Mon, Jun 24 2019 at 07:29 -0600, Linus Walleij wrote:
>+static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = {
>+	.activate = gpiochip_irq_domain_activate,
>+	.deactivate = gpiochip_irq_domain_deactivate,
>+	.translate = gpiochip_hierarchy_irq_domain_translate,
>+	.alloc = gpiochip_hierarchy_irq_domain_alloc,
>+	.free = irq_domain_free_irqs_common,
>+};
>+
>+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
>+{
>+	if (!gc->irq.parent_domain) {
>+		chip_err(gc, "missing parent irqdomain\n");
>+		return -EINVAL;
>+	}
>+
>+	if (!gc->irq.parent_domain ||
>+	    !gc->irq.child_to_parent_hwirq ||
>+	    !gc->irq.fwnode) {
>+		chip_err(gc, "missing irqdomain vital data\n");
>+		return -EINVAL;
>+	}
>+
>+	gc->irq.domain = irq_domain_create_hierarchy(
>+		gc->irq.parent_domain,
>+		IRQ_DOMAIN_FLAG_HIERARCHY,
>+		gc->ngpio,
>+		gc->irq.fwnode,
>+		&gpiochip_hierarchy_domain_ops,
This should probably be used only if gc->irq.domain_ops is not set.
>+		gc);
>+
>+	if (!gc->irq.domain) {
>+		chip_err(gc, "failed to add hierarchical domain\n");
>+		return -EINVAL;
>+	}
>+
>+	gpiochip_set_hierarchical_irqchip(gc, gc->irq.chip);
>+
>+	chip_info(gc, "set up hierarchical irqdomain\n");
>+
>+	return 0;
>+}
>+

Thanks,
Lina

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

Hi Lina,

thanks for your comments!

On Wed, Jun 26, 2019 at 10:09 PM Lina Iyer <ilina@codeaurora.org> wrote:

> Thanks for the patch Linus. I was running into the warning in
> gpiochip_set_irq_hooks(), because it was called from two places.
> Hopefully, this will fix that as well. I will give it a try.

That's usually when creating two irqchips from a static config.
The most common solution is to put struct irq_chip into the
driver state container and assign all functions dynamically so
the irqchip is a "live" struct if you see how I mean. This is
needed because the gpiolib irqchip core will augment some
of the pointers in the irqchip, so if that is done twice, it feels
a bit shaky.

> On Mon, Jun 24 2019 at 07:29 -0600, Linus Walleij wrote:

> >+  girq->num_parents = 1;
> >+  girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
> >+                               GFP_KERNEL);
>
> Could this be folded into the gpiolib?

It is part of the existing API for providing an irq_chip along
with the gpio_chip but you are right, it's kludgy. I do have
a patch like this, making the parents a static sized field
simply:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=devel-gpio-irqchip

So I might go on this approach. (In a separate patch.)

> >+  /* Get a pointer to the gpio_irq_chip */
> >+  girq = &g->gc.irq;
> >+  girq->chip = &g->irq;
> >+  girq->default_type = IRQ_TYPE_NONE;
> >+  girq->handler = handle_bad_irq;
> >+  girq->fwnode = g->fwnode;
> >+  girq->parent_domain = parent;
> >+  girq->child_to_parent_hwirq = my_gpio_child_to_parent_hwirq;
> >+
> Should be the necessary, if the driver implements it's own .alloc?

The idea is that when using GPIOLIB_IRQCHIP and
IRQ_DOMAIN_HIERARCHY together, the drivers utilizing
GPIOLIB_IRQCHIP will rely on the .alloc() and .translate()
implementations in gpiolib so the ambition is that these should
cover all hierarchical use cases.

> >+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
> >+{
> >+      if (!gc->irq.parent_domain) {
> >+              chip_err(gc, "missing parent irqdomain\n");
> >+              return -EINVAL;
> >+      }
> >+
> >+      if (!gc->irq.parent_domain ||
> >+          !gc->irq.child_to_parent_hwirq ||
>
> This should probably be validated if the .ops have not been set.

Yeah the idea here is a pretty imperialistic one: the gpiolib
always provide the ops for hierarchical IRQ. The library
implementation should cover all needs of all consumers,
for the hierarchical case.

I might be wrong, but then I need to see some example
of hierarchies that need something more than what the
gpiolib core is providing and idiomatic enough that it can't
be rewritten and absolutely must have its own ops.

> >+      int (*child_to_parent_hwirq)(struct gpio_chip *chip,
> >+                                   unsigned int child_hwirq,
> >+                                   unsigned int child_type,
> >+                                   unsigned int *parent_hwirq,
> >+                                   unsigned int *parent_type);
>
> Would irq_fwspec(s) be better than passing all these arguments around?

I looked over these three drivers that I patched in the series
and they all seemed to need pretty much these arguments,
so wrapping it into fwspec would probably just make all
drivers have to unwrap them to get child (I guess not parent)
back out.

But we can patch it later if it proves this is too much arguments
for some drivers. Its the right amount for those I changed,
AFAICT.

Yours,
Linus Walleij

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Brian Masney]

On Mon, Jun 24, 2019 at 03:25:28PM +0200, Linus Walleij wrote:
> +static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
> +					       unsigned int irq,
> +					       unsigned int nr_irqs,
> +					       void *data)
> +{
> +	struct gpio_chip *gc = d->host_data;
> +	irq_hw_number_t hwirq;
> +	unsigned int type = IRQ_TYPE_NONE;
> +	struct irq_fwspec *fwspec = data;
> +	int ret;
> +	int i;
> +
> +	chip_info(gc, "called %s\n", __func__);
> +
> +	ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	chip_info(gc, "allocate IRQ %d..%d, hwirq %lu..%lu\n",
> +		  irq, irq + nr_irqs - 1,
> +		  hwirq, hwirq + nr_irqs - 1);
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		struct irq_fwspec parent_fwspec;
> +		unsigned int parent_hwirq;
> +		unsigned int parent_type;
> +		struct gpio_irq_chip *girq = &gc->irq;
> +
> +		ret = girq->child_to_parent_hwirq(gc, hwirq, type,
> +						  &parent_hwirq, &parent_type);
> +		if (ret) {
> +			chip_err(gc, "can't look up hwirq %lu\n", hwirq);
> +			return ret;
> +		}
> +		chip_info(gc, "found parent hwirq %u\n", parent_hwirq);
> +
> +		/*
> +		 * We set handle_bad_irq because the .set_type() should
> +		 * always be invoked and set the right type of handler.
> +		 */
> +		irq_domain_set_info(d,
> +				    irq + i,
> +				    hwirq + i,
> +				    gc->irq.chip,
> +				    gc,
> +				    handle_bad_irq,
> +				    NULL, NULL);
> +		irq_set_probe(irq + i);
> +
> +		/*
> +		 * Create a IRQ fwspec to send up to the parent irqdomain:
> +		 * specify the hwirq we address on the parent and tie it
> +		 * all together up the chain.
> +		 */
> +		parent_fwspec.fwnode = d->parent->fwnode;
> +		parent_fwspec.param_count = 2;
> +		parent_fwspec.param[0] = parent_hwirq;
> +		/* This parent only handles asserted level IRQs */
> +		parent_fwspec.param[1] = parent_type;
> +		chip_info(gc, "alloc_irqs_parent for %d parent hwirq %d\n",
> +			  irq + i, parent_hwirq);
> +		ret = irq_domain_alloc_irqs_parent(d, irq + i, 1,
> +						   &parent_fwspec);

I started to convert qcom's spmi-gpio over to this new API. I'm not done
yet but I noticed that this new API assumes two cells for the parent,
however spmi-gpio's parent (drivers/spmi/spmi-pmic-arb.c) expects four
cells. See pmic_gpio_domain_alloc() in
drivers/pinctrl/qcom/pinctrl-spmi-gpio.c for more details.

What do you think about adding a function pointer to struct
gpio_irq_chip that is used to populate the parent_fwspec?

Brian

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

On Fri, Jun 28, 2019 at 11:43 AM Brian Masney <masneyb@onstation.org> wrote:

> I started to convert qcom's spmi-gpio over to this new API. I'm not done
> yet but I noticed that this new API assumes two cells for the parent,
> however spmi-gpio's parent (drivers/spmi/spmi-pmic-arb.c) expects four
> cells. See pmic_gpio_domain_alloc() in
> drivers/pinctrl/qcom/pinctrl-spmi-gpio.c for more details.
>
> What do you think about adding a function pointer to struct
> gpio_irq_chip that is used to populate the parent_fwspec?

I discussed this before with Lina and I naïvely thought we
would have only twocell irqchips... OK I was wrong and
Lina is right.

And I agree, let's put a function pointer for this in gpio_irq_chip
and add code so we default to twocell if it is not specified.

You can hack something up if you like and I will just put that
on top of my patch.

Yours,
Linus Walleij

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Lina Iyer]

Hi Linus,

On Fri, Jun 28 2019 at 03:15 -0600, Linus Walleij wrote:
>Hi Lina,
>
>thanks for your comments!
>
>On Wed, Jun 26, 2019 at 10:09 PM Lina Iyer <ilina@codeaurora.org> wrote:
>
>> Thanks for the patch Linus. I was running into the warning in
>> gpiochip_set_irq_hooks(), because it was called from two places.
>> Hopefully, this will fix that as well. I will give it a try.
>
>That's usually when creating two irqchips from a static config.
>The most common solution is to put struct irq_chip into the
>driver state container and assign all functions dynamically so
>the irqchip is a "live" struct if you see how I mean. This is
>needed because the gpiolib irqchip core will augment some
>of the pointers in the irqchip, so if that is done twice, it feels
>a bit shaky.
>
Yeah, I realized what was happening. I have fixed it in my drivers.

>> On Mon, Jun 24 2019 at 07:29 -0600, Linus Walleij wrote:
>
>> >+  girq->num_parents = 1;
>> >+  girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
>> >+                               GFP_KERNEL);
>>
>> Could this be folded into the gpiolib?
>
>It is part of the existing API for providing an irq_chip along
>with the gpio_chip but you are right, it's kludgy. I do have
>a patch like this, making the parents a static sized field
>simply:
>https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/commit/?h=devel-gpio-irqchip
>
>So I might go on this approach. (In a separate patch.)
>
>> >+  /* Get a pointer to the gpio_irq_chip */
>> >+  girq = &g->gc.irq;
>> >+  girq->chip = &g->irq;
>> >+  girq->default_type = IRQ_TYPE_NONE;
>> >+  girq->handler = handle_bad_irq;
>> >+  girq->fwnode = g->fwnode;
>> >+  girq->parent_domain = parent;
>> >+  girq->child_to_parent_hwirq = my_gpio_child_to_parent_hwirq;
>> >+
>> Should be the necessary, if the driver implements it's own .alloc?
>
>The idea is that when using GPIOLIB_IRQCHIP and
>IRQ_DOMAIN_HIERARCHY together, the drivers utilizing
>GPIOLIB_IRQCHIP will rely on the .alloc() and .translate()
>implementations in gpiolib so the ambition is that these should
>cover all hierarchical use cases.
>
>> >+static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
>> >+{
>> >+      if (!gc->irq.parent_domain) {
>> >+              chip_err(gc, "missing parent irqdomain\n");
>> >+              return -EINVAL;
>> >+      }
>> >+
>> >+      if (!gc->irq.parent_domain ||
>> >+          !gc->irq.child_to_parent_hwirq ||
>>
>> This should probably be validated if the .ops have not been set.
>
>Yeah the idea here is a pretty imperialistic one: the gpiolib
>always provide the ops for hierarchical IRQ. The library
>implementation should cover all needs of all consumers,
>for the hierarchical case.
>
>I might be wrong, but then I need to see some example
>of hierarchies that need something more than what the
>gpiolib core is providing and idiomatic enough that it can't
>be rewritten and absolutely must have its own ops.
>
Here is an example of what I am working on [1]. The series is based on
this patch. What I want to point out is the .alloc function. The TLMM
irqchip's parent could be a PDC or a MPM depending on the QCOM SoC
architecture. They behave differently. The PDC takes over for the GPIO
and handles the monitoring etc, while the MPM comes into play only after
the SoC is in low power therefore TLMM needs to do its job. The way to
cleanly support both of themis to have our own .alloc functions to help
understand the the wakeup-parent irqchip's behavior.

Since I need my own .ops, it makes the function below irrelevant to
gpiolib. While I would still need a function to translate to parent
hwirq, I don't see it any beneficial to gpiolib.

thanks,
Lina

>> >+      int (*child_to_parent_hwirq)(struct gpio_chip *chip,
>> >+                                   unsigned int child_hwirq,
>> >+                                   unsigned int child_type,
>> >+                                   unsigned int *parent_hwirq,
>> >+                                   unsigned int *parent_type);
>>
>> Would irq_fwspec(s) be better than passing all these arguments around?
>
>I looked over these three drivers that I patched in the series
>and they all seemed to need pretty much these arguments,
>so wrapping it into fwspec would probably just make all
>drivers have to unwrap them to get child (I guess not parent)
>back out.
>
>But we can patch it later if it proves this is too much arguments
>for some drivers. Its the right amount for those I changed,
>AFAICT.
>
>Yours,
>Linus Walleij

[1]. https://github.com/i-lina/linux/commits/gpio2-2

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

On Fri, Jun 28, 2019 at 5:58 PM Lina Iyer <ilina@codeaurora.org> wrote:

> >I might be wrong, but then I need to see some example
> >of hierarchies that need something more than what the
> >gpiolib core is providing and idiomatic enough that it can't
> >be rewritten and absolutely must have its own ops.
>
> Here is an example of what I am working on [1]. The series is based on
> this patch. What I want to point out is the .alloc function. The TLMM
> irqchip's parent could be a PDC or a MPM depending on the QCOM SoC
> architecture. They behave differently. The PDC takes over for the GPIO
> and handles the monitoring etc, while the MPM comes into play only after
> the SoC is in low power therefore TLMM needs to do its job. The way to
> cleanly support both of themis to have our own .alloc functions to help
> understand the the wakeup-parent irqchip's behavior.
>
> Since I need my own .ops, it makes the function below irrelevant to
> gpiolib. While I would still need a function to translate to parent
> hwirq, I don't see it any beneficial to gpiolib.

OK I see, I am holding the patch set back for v5.4 so we can make
sure that yours and also Brian Masney's drivers will be able to
work with this. Let's try to get this in shape for the next kernel.

Yours,
Linus Walleij

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Brian Masney]

Hi Linus,

On Mon, Jun 24, 2019 at 03:25:28PM +0200, Linus Walleij wrote:
>  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
> +	struct irq_domain *domain = chip->irq.domain;
> +
>  	if (!gpiochip_irqchip_irq_valid(chip, offset))
>  		return -ENXIO;
>  
> -	return irq_create_mapping(chip->irq.domain, offset);
> +	if (irq_domain_is_hierarchy(domain)) {
> +		struct irq_fwspec spec;
> +
> +		spec.fwnode = domain->fwnode;
> +		spec.param_count = 2;
> +		spec.param[0] = offset;
> +		spec.param[1] = IRQ_TYPE_NONE;
> +
> +		return irq_create_fwspec_mapping(&spec);
> +	}

spmi-gpio's to_irq() needs to add one to the offset:

	static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
	{
		struct pmic_gpio_state *state = gpiochip_get_data(chip);
		struct irq_fwspec fwspec;
	
		fwspec.fwnode = state->fwnode;
		fwspec.param_count = 2;
		fwspec.param[0] = pin + PMIC_GPIO_PHYSICAL_OFFSET;
		/*
		 * Set the type to a safe value temporarily. This will be overwritten
		 * later with the proper value by irq_set_type.
		 */
		fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
	
		return irq_create_fwspec_mapping(&fwspec);
	}

ssbi-gpio will have the same problem as well.

What do you think about adding a new field to the struct gpio_irq_chip
inside the CONFIG_IRQ_DOMAIN_HIERARCHY ifdef called something like
to_irq_offset? (I'm bad at naming things.)

Also, instead of hardcoding IRQ_TYPE_NONE, what do you think about using
the default_type field that's available?

Brian

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

On Wed, Jul 3, 2019 at 11:22 AM Brian Masney <masneyb@onstation.org> wrote:
> On Mon, Jun 24, 2019 at 03:25:28PM +0200, Linus Walleij wrote:
> >  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
> >  {
> > +     struct irq_domain *domain = chip->irq.domain;
> > +
> >       if (!gpiochip_irqchip_irq_valid(chip, offset))
> >               return -ENXIO;
> >
> > -     return irq_create_mapping(chip->irq.domain, offset);
> > +     if (irq_domain_is_hierarchy(domain)) {
> > +             struct irq_fwspec spec;
> > +
> > +             spec.fwnode = domain->fwnode;
> > +             spec.param_count = 2;
> > +             spec.param[0] = offset;
> > +             spec.param[1] = IRQ_TYPE_NONE;
> > +
> > +             return irq_create_fwspec_mapping(&spec);
> > +     }
>
> spmi-gpio's to_irq() needs to add one to the offset:
>
>         static int pmic_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
>         {
>                 struct pmic_gpio_state *state = gpiochip_get_data(chip);
>                 struct irq_fwspec fwspec;
>
>                 fwspec.fwnode = state->fwnode;
>                 fwspec.param_count = 2;
>                 fwspec.param[0] = pin + PMIC_GPIO_PHYSICAL_OFFSET;
>                 /*
>                  * Set the type to a safe value temporarily. This will be overwritten
>                  * later with the proper value by irq_set_type.
>                  */
>                 fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
>
>                 return irq_create_fwspec_mapping(&fwspec);
>         }
>
> ssbi-gpio will have the same problem as well.
>
> What do you think about adding a new field to the struct gpio_irq_chip
> inside the CONFIG_IRQ_DOMAIN_HIERARCHY ifdef called something like
> to_irq_offset? (I'm bad at naming things.)

I think to cover Lina's need and following the direction set out
by Thierry's desire to have callback so we can control the
parent-to-child translation with code, it might be best to have
an optional callback for translating fwspec.param[0].

Thierry seems to need exactly this for the Tegra driver to,
I think that is why it has custom ops today.

> Also, instead of hardcoding IRQ_TYPE_NONE, what do you think about using
> the default_type field that's available?

I want to get rid of the .default_type in the long run, it is
nominally only for board files where the .set_type() isn't
ever getting called. For anything modern, the .set_type()
callback is always called before any irq is used.

Yours,
Linus Walleij

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Brian Masney]

Hi Linus,

On Mon, Jun 24, 2019 at 03:25:28PM +0200, Linus Walleij wrote:
> Hierarchical IRQ domains can be used to stack different IRQ
> controllers on top of each other.
> 
> Bring hierarchical IRQ domains into the GPIOLIB core with the
> following basic idea:

I got this working with spmi-gpio with two additional changes. See below
for details. Hopefully I'll have time tomorrow evening (GMT-4) to finish
cleaning up what I have so I can send out my series.

> +static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d,
> +						   struct irq_fwspec *fwspec,
> +						   unsigned long *hwirq,
> +						   unsigned int *type)
> +{
> +	/* We support standard DT translation */
> +	if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
> +		return irq_domain_translate_twocell(d, fwspec, hwirq, type);
> +	}
> +
> +	/* This is for board files and others not using DT */
> +	if (is_fwnode_irqchip(fwspec->fwnode)) {
> +		int ret;
> +
> +		ret = irq_domain_translate_twocell(d, fwspec, hwirq, type);
> +		if (ret)
> +			return ret;
> +		WARN_ON(*type == IRQ_TYPE_NONE);
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
>
> [ snip ]
>
> +static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = {
> +	.activate = gpiochip_irq_domain_activate,
> +	.deactivate = gpiochip_irq_domain_deactivate,
> +	.translate = gpiochip_hierarchy_irq_domain_translate,
> +	.alloc = gpiochip_hierarchy_irq_domain_alloc,
> +	.free = irq_domain_free_irqs_common,
> +};

spmi and ssbi gpio both need to subtract one from the hwirq in the
translate function.

https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c#L956

I'm going to optionally allow overriding the translate() function
pointer as well.

> +static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
> +					       unsigned int irq,
> +					       unsigned int nr_irqs,
> +					       void *data)
> +{
> +	struct gpio_chip *gc = d->host_data;
> +	irq_hw_number_t hwirq;
> +	unsigned int type = IRQ_TYPE_NONE;
> +	struct irq_fwspec *fwspec = data;
> +	int ret;
> +	int i;
> +
> +	chip_info(gc, "called %s\n", __func__);
> +
> +	ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	chip_info(gc, "allocate IRQ %d..%d, hwirq %lu..%lu\n",
> +		  irq, irq + nr_irqs - 1,
> +		  hwirq, hwirq + nr_irqs - 1);
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		struct irq_fwspec parent_fwspec;
> +		unsigned int parent_hwirq;
> +		unsigned int parent_type;
> +		struct gpio_irq_chip *girq = &gc->irq;
> +
> +		ret = girq->child_to_parent_hwirq(gc, hwirq, type,
> +						  &parent_hwirq, &parent_type);
> +		if (ret) {
> +			chip_err(gc, "can't look up hwirq %lu\n", hwirq);
> +			return ret;
> +		}
> +		chip_info(gc, "found parent hwirq %u\n", parent_hwirq);
> +
> +		/*
> +		 * We set handle_bad_irq because the .set_type() should
> +		 * always be invoked and set the right type of handler.
> +		 */
> +		irq_domain_set_info(d,
> +				    irq + i,
> +				    hwirq + i,
> +				    gc->irq.chip,
> +				    gc,
> +				    handle_bad_irq,
                                    ^^^^^^^^^^
In order to get this working, I had to change handle_bad_irq to
handle_level_irq otherwise I get this attempted NULL pointer
dereference:

[    2.260256] Unable to handle kernel NULL pointer dereference at virtual address 00000018
[    2.263388] pgd = (ptrval)
[    2.271624] [00000018] *pgd=00000000
[    2.274149] Internal error: Oops: 5 [#1] PREEMPT SMP ARM
[    2.277877] Modules linked in:
[    2.283174] CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.2.0-rc7-next-20190701-00017-g58c27736c3cb-dirty #34
[    2.286043] Hardware name: Generic DT based system
[    2.295687] PC is at irq_chip_ack_parent+0x8/0x10
[    2.300540] LR is at __irq_do_set_handler+0x1b4/0x1bc
[    2.305309] pc : [<c0372af4>]    lr : [<c0373f6c>]    psr: a0000093
[    2.310343] sp : de899be0  ip : c100f06c  fp : 00000001
[    2.316420] r10: 0000004e  r9 : 00000000  r8 : 00000000
[    2.321626] r7 : 00000000  r6 : 00000000  r5 : c036eea4  r4 : c4e7a600
[    2.326839] r3 : 00000000  r2 : 00000000  r1 : c0372aec  r0 : c4e7d5c0
[    2.333438] Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
[    2.339947] Control: 10c5787d  Table: 0020406a  DAC: 00000051
[    2.347152] Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
[    2.352967] Stack: (0xde899be0 to 0xde89a000)
[    2.359063] 9be0: 00000000 00000000 c1004c48 c4e7a600 c036eea4 c0373fc0 60000013 921471da
[    2.363322] 9c00: 0000004e 00000000 c036eea4 00000000 00000000 c0375d44 c4df124c 921471da
[    2.371480] 9c20: c4df124c c4df124c 0000004e 00000000 c4de8900 c06cf28c c4df124c c036eea4
[    2.379640] 9c40: 00000000 00000000 c1004c48 c0da5e28 c036eea4 c0da5e48 de899c7c 00000001
[    2.387800] 9c60: 00000000 000000c1 00000000 00000001 c4e7a600 c036e394 c0d74960 00000000
[    2.395959] 9c80: 0000004f c0af60b0 00000000 00000000 00000002 0000004e c4df0600 00000001
[    2.404120] 9ca0: 00000000 ffffffff 00000000 c4de8900 c4e7d5c0 921471da c4e27940 c06cf120
[    2.412278] 9cc0: 00000001 0000004e 0000004e c4de8900 c0e172b4 00000001 c4e7d5c0 c0376bac
[    2.420438] 9ce0: 00000000 c0834274 00000000 c1004c48 c4de8900 de899d4c 00000000 de9d9c10
[    2.428598] 9d00: 00000001 00000000 c4e27940 c03771d4 de899d4c 00000000 00000000 c06ca5b0
[    2.436759] 9d20: de9d9db8 00000001 00000000 921471da c1004c48 c4df124c c4de8900 00000001
[    2.444919] 9d40: de9d9c10 c06c90b8 c27065bc def6e270 00000002 00000002 00000000 c10a2374
[    2.453077] 9d60: c4d6f810 de9d9c10 de899d80 6f697067 de9d0073 a0000013 c1004c48 00000000
[    2.461237] 9d80: a0000013 c098977c c4e27940 921471da c4e27940 921471da c4d6f810 c4e6ec8c
[    2.469399] 9da0: c4e2795c 00000000 c27065bc c06c8624 c4e6ec8c c092b1b8 00000001 c27065bc
[    2.477558] 9dc0: c10c59b8 c4e6ec60 00000000 c4e81000 00000000 def757d4 c4e2795c c4e6ec40
[    2.485717] 9de0: de9d9c00 c092ab44 c092aac8 c092ab60 00000000 de9d9c10 00000000 c10a2608
[    2.493876] 9e00: 00000000 c10c59b8 00000000 c10a2608 0000000e c0817c00 c110dcf4 de9d9c10
[    2.502037] 9e20: c110dcf8 c0815738 c10a2608 de9d9c10 c0f004a4 de9d9c10 c10a2608 c10a2608
[    2.510197] 9e40: c0815fa4 00000000 c0f56838 c0f56858 c0f004a4 c0815bf4 c0f56858 c0989428
[    2.518355] 9e60: c0bbf5c8 de9d9c10 00000000 c10a2608 c0815fa4 00000000 c0f56838 c0f56858
[    2.526515] 9e80: c0f004a4 c0815f9c 00000000 c10a2608 de9d9c10 c0816058 de9dab34 c1004c48
[    2.534677] 9ea0: c10a2608 c0813908 c1095780 de825858 de9dab34 921471da de82586c c10a2608
[    2.542835] 9ec0: c4e96780 c1095780 00000000 c0814aa0 c0df2cac c1004c48 ffffe000 c10a2608
[    2.550994] 9ee0: c1004c48 ffffe000 c0f39074 c0816be4 c10b97c0 c1004c48 ffffe000 c0302f7c
[    2.559153] 9f00: 00000000 c0f004a4 c10be278 00000000 00000007 c0d59294 c0dd2fe4 00000000
[    2.567314] 9f20: 00000000 c1004c48 c0d69934 c0d5934c 00002cc0 dffffbf8 dffffc0d 921471da
[    2.575475] 9f40: 00000000 c10b97c0 dffffa80 921471da c10b97c0 c0f6414c 00000008 c10d2cc0
[    2.583633] 9f60: c10d2cc0 c0f01204 00000007 00000007 00000000 c0f004a4 000000db 00000000
[    2.591793] 9f80: c0af5ba8 00000000 c0af5ba8 00000000 00000000 00000000 00000000 00000000
[    2.599951] 9fa0: 00000000 c0af5bb0 00000000 c03010e8 00000000 00000000 00000000 00000000
[    2.608111] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    2.616271] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[    2.624430] [<c0372af4>] (irq_chip_ack_parent) from [<c0373f6c>] (__irq_do_set_handler+0x1b4/0x1bc)
[    2.632584] [<c0373f6c>] (__irq_do_set_handler) from [<c0373fc0>] (__irq_set_handler+0x4c/0x78)
[    2.641441] [<c0373fc0>] (__irq_set_handler) from [<c0375d44>] (irq_domain_set_info+0x38/0x4c)
[    2.650126] [<c0375d44>] (irq_domain_set_info) from [<c06cf28c>] (gpiochip_hierarchy_irq_domain_alloc+0x16c/0x22c)
[    2.658808] [<c06cf28c>] (gpiochip_hierarchy_irq_domain_alloc) from [<c0376bac>] (__irq_domain_alloc_irqs+0x12c/0x320)
[    2.669134] [<c0376bac>] (__irq_domain_alloc_irqs) from [<c03771d4>] (irq_create_fwspec_mapping+0x27c/0x344)
[    2.679808] [<c03771d4>] (irq_create_fwspec_mapping) from [<c06c90b8>] (gpiochip_to_irq+0x6c/0xa0)
[    2.689793] [<c06c90b8>] (gpiochip_to_irq) from [<c06c8624>] (gpiod_to_irq+0x48/0x64)
[    2.698558] [<c06c8624>] (gpiod_to_irq) from [<c092b1b8>] (gpio_keys_probe+0x4b4/0x8e8)
[    2.706461] [<c092b1b8>] (gpio_keys_probe) from [<c0817c00>] (platform_drv_probe+0x48/0x98)
[    2.714272] [<c0817c00>] (platform_drv_probe) from [<c0815738>] (really_probe+0x108/0x40c)
[    2.722599] [<c0815738>] (really_probe) from [<c0815bf4>] (driver_probe_device+0x78/0x1c4)
[    2.730934] [<c0815bf4>] (driver_probe_device) from [<c0815f9c>] (device_driver_attach+0x58/0x60)
[    2.739182] [<c0815f9c>] (device_driver_attach) from [<c0816058>] (__driver_attach+0xb4/0x154)
[    2.748121] [<c0816058>] (__driver_attach) from [<c0813908>] (bus_for_each_dev+0x74/0xb4)
[    2.756628] [<c0813908>] (bus_for_each_dev) from [<c0814aa0>] (bus_add_driver+0x1c0/0x200)
[    2.764875] [<c0814aa0>] (bus_add_driver) from [<c0816be4>] (driver_register+0x7c/0x114)
[    2.773035] [<c0816be4>] (driver_register) from [<c0302f7c>] (do_one_initcall+0x54/0x2a0)
[    2.781284] [<c0302f7c>] (do_one_initcall) from [<c0f01204>] (kernel_init_freeable+0x2d4/0x36c)
[    2.789357] [<c0f01204>] (kernel_init_freeable) from [<c0af5bb0>] (kernel_init+0x8/0x110)
[    2.797860] [<c0af5bb0>] (kernel_init) from [<c03010e8>] (ret_from_fork+0x14/0x2c)
[    2.806181] Exception stack(0xde899fb0 to 0xde899ff8)
[    2.813653] 9fa0:                                     00000000 00000000 00000000 00000000
[    2.818788] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[    2.826943] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000
[    2.835102] Code: 0592301c e12fff13 e5900018 e5903010 (e5933018) 
[    2.841513] ---[ end trace e31675e4bfb4e93f ]---

The parent's irq_chip struct isn't populated yet and the error occurs
here:

    void irq_chip_ack_parent(struct irq_data *data)
    {
            data = data->parent_data;
            data->chip->irq_ack(data);
                  ^^^^

We haven't called irq_domain_alloc_irqs_parent() yet, which is fine.

__irq_do_set_handler() has a special check for handle_bad_irq():

https://elixir.bootlin.com/linux/latest/source/kernel/irq/chip.c#L974

I'm not sure what the proper fix is here and not going to dig into this
anymore this evening.

> diff --git a/Documentation/driver-api/gpio/driver.rst b/Documentation/driver-api/gpio/driver.rst
> index 1ce7fcd0f989..3099c7fbefdb 100644
> --- a/Documentation/driver-api/gpio/driver.rst
> +++ b/Documentation/driver-api/gpio/driver.rst

I'm still on linux next-20190701. Does this patch series of yours
require any other patches? I get a merge conflict against driver.rst.
Everything else applies cleanly. I honestly haven't looked in detail
about the conflicts.

Brian

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

On Sun, Jul 7, 2019 at 3:46 AM Brian Masney <masneyb@onstation.org> wrote:

> I got this working with spmi-gpio with two additional changes. See below
> for details. Hopefully I'll have time tomorrow evening (GMT-4) to finish
> cleaning up what I have so I can send out my series.

Awesome! No hurry because it is v5.4 material at this point but I'm
hoping to get to something you, Lina and Thierry can all use for early
merge and smoke test.

> > +static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = {
> > +     .activate = gpiochip_irq_domain_activate,
> > +     .deactivate = gpiochip_irq_domain_deactivate,
> > +     .translate = gpiochip_hierarchy_irq_domain_translate,
> > +     .alloc = gpiochip_hierarchy_irq_domain_alloc,
> > +     .free = irq_domain_free_irqs_common,
> > +};
>
> spmi and ssbi gpio both need to subtract one from the hwirq in the
> translate function.
>
> https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/qcom/pinctrl-spmi-gpio.c#L956
>
> I'm going to optionally allow overriding the translate() function
> pointer as well.

Hm was more thinking to let gpiolib call out to an optional translate
function (on top of the template) but this is maybe cleaner.

> > +             /*
> > +              * We set handle_bad_irq because the .set_type() should
> > +              * always be invoked and set the right type of handler.
> > +              */
> > +             irq_domain_set_info(d,
> > +                                 irq + i,
> > +                                 hwirq + i,
> > +                                 gc->irq.chip,
> > +                                 gc,
> > +                                 handle_bad_irq,
>                                     ^^^^^^^^^^
> In order to get this working, I had to change handle_bad_irq to
> handle_level_irq otherwise I get this attempted NULL pointer
> dereference:

Hmmmmmmmm that should not happen, we need to get to the
bottom of this.

> [    2.624430] [<c0372af4>] (irq_chip_ack_parent) from [<c0373f6c>] (__irq_do_set_handler+0x1b4/0x1bc)
> [    2.632584] [<c0373f6c>] (__irq_do_set_handler) from [<c0373fc0>] (__irq_set_handler+0x4c/0x78)
> [    2.641441] [<c0373fc0>] (__irq_set_handler) from [<c0375d44>] (irq_domain_set_info+0x38/0x4c)
> [    2.650126] [<c0375d44>] (irq_domain_set_info) from [<c06cf28c>] (gpiochip_hierarchy_irq_domain_alloc+0x16c/0x22c)
> [    2.658808] [<c06cf28c>] (gpiochip_hierarchy_irq_domain_alloc) from [<c0376bac>] (__irq_domain_alloc_irqs+0x12c/0x320)

I wonder why irq_chip_ack_parent() is called there.
Like there is some pending IRQ or something.

> The parent's irq_chip struct isn't populated yet and the error occurs
> here:
>
>     void irq_chip_ack_parent(struct irq_data *data)
>     {
>             data = data->parent_data;
>             data->chip->irq_ack(data);
>                   ^^^^
>
> We haven't called irq_domain_alloc_irqs_parent() yet, which is fine.
>
> __irq_do_set_handler() has a special check for handle_bad_irq():
>
> https://elixir.bootlin.com/linux/latest/source/kernel/irq/chip.c#L974
>
> I'm not sure what the proper fix is here and not going to dig into this
> anymore this evening.

I'm a bit puzzled too. :/

> > diff --git a/Documentation/driver-api/gpio/driver.rst b/Documentation/driver-api/gpio/driver.rst
> > index 1ce7fcd0f989..3099c7fbefdb 100644
> > --- a/Documentation/driver-api/gpio/driver.rst
> > +++ b/Documentation/driver-api/gpio/driver.rst
>
> I'm still on linux next-20190701. Does this patch series of yours
> require any other patches? I get a merge conflict against driver.rst.
> Everything else applies cleanly. I honestly haven't looked in detail
> about the conflicts.

I have some pending documentation patch I think.
Sorry about that.

Yours,
Linus Walleij

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Brian Masney]

Hi Linus,

On Mon, Jun 24, 2019 at 03:25:28PM +0200, Linus Walleij wrote:
> +associated irqdomain and resource allocation callbacks. These are activated
> +by selecting the Kconfig symbol GPIOLIB_IRQCHIP. If the symbol
> +IRQ_DOMAIN_HIERARCHY is also selected, hierarchical helpers will also be
> +provided. A big portion of overhead code will be managed by gpiolib,
> +under the assumption that your interrupts are 1-to-1-mapped to the
> +GPIO line index:
> +
> +  GPIO line offset   Hardware IRQ
> +  0                  0
> +  1                  1
> +  2                  2
> +  ...                ...
> +  ngpio-1            ngpio-1
> +
> +If some GPIO lines do not have corresponding IRQs, the bitmask valid_mask
> +and the flag need_valid_mask in gpio_irq_chip can be used to mask off some
> +lines as invalid for associating with IRQs.

I forgot to call out in my patch series that the GPIOs are numbered
1..ngpio on Qualcomm and the existing to_irq and translate callbacks
in mainline take care of adding and subtracting one to / from the
offset.

I was under the (false?) assumption that GPIO numbering on all platforms
start at one. Is that not the case?

Brian

Re: [PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP

[Masahiro Yamada]

Hi Linus,

On Mon, Jun 24, 2019 at 10:25 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Use the new infrastructure for hierarchical irqchips in
> gpiolib.
>
> I have no chance to test or debug this so I need
> help.
>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Brian Masney <masneyb@onstation.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/gpio/Kconfig         |   1 +
>  drivers/gpio/gpio-uniphier.c | 172 +++++++++--------------------------
>  2 files changed, 45 insertions(+), 128 deletions(-)
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index ee34716a39aa..806d642498a6 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -552,6 +552,7 @@ config GPIO_UNIPHIER
>         tristate "UniPhier GPIO support"
>         depends on ARCH_UNIPHIER || COMPILE_TEST
>         depends on OF_GPIO
> +       select GPIOLIB_IRQCHIP
>         select IRQ_DOMAIN_HIERARCHY
>         help
>           Say yes here to support UniPhier GPIOs.
> diff --git a/drivers/gpio/gpio-uniphier.c b/drivers/gpio/gpio-uniphier.c
> index 93cdcc41e9fb..e960836b9c01 100644
> --- a/drivers/gpio/gpio-uniphier.c
> +++ b/drivers/gpio/gpio-uniphier.c
> @@ -6,7 +6,6 @@
>  #include <linux/bits.h>
>  #include <linux/gpio/driver.h>
>  #include <linux/irq.h>
> -#include <linux/irqdomain.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> @@ -30,7 +29,6 @@
>  struct uniphier_gpio_priv {
>         struct gpio_chip chip;
>         struct irq_chip irq_chip;
> -       struct irq_domain *domain;
>         void __iomem *regs;
>         spinlock_t lock;
>         u32 saved_vals[0];
> @@ -162,49 +160,33 @@ static void uniphier_gpio_set_multiple(struct gpio_chip *chip,
>         }
>  }
>
> -static int uniphier_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +static void uniphier_gpio_irq_mask(struct irq_data *d)
>  {
> -       struct irq_fwspec fwspec;
> -
> -       if (offset < UNIPHIER_GPIO_IRQ_OFFSET)
> -               return -ENXIO;
> -
> -       fwspec.fwnode = of_node_to_fwnode(chip->parent->of_node);
> -       fwspec.param_count = 2;
> -       fwspec.param[0] = offset - UNIPHIER_GPIO_IRQ_OFFSET;
> -       /*
> -        * IRQ_TYPE_NONE is rejected by the parent irq domain. Set LEVEL_HIGH
> -        * temporarily. Anyway, ->irq_set_type() will override it later.
> -        */
> -       fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
> -
> -       return irq_create_fwspec_mapping(&fwspec);
> -}
> -
> -static void uniphier_gpio_irq_mask(struct irq_data *data)
> -{
> -       struct uniphier_gpio_priv *priv = data->chip_data;
> -       u32 mask = BIT(data->hwirq);
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct uniphier_gpio_priv *priv = gpiochip_get_data(gc);
> +       u32 mask = BIT(d->hwirq);
>
>         uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, 0);
>
> -       return irq_chip_mask_parent(data);
> +       return irq_chip_mask_parent(d);
>  }
>
> -static void uniphier_gpio_irq_unmask(struct irq_data *data)
> +static void uniphier_gpio_irq_unmask(struct irq_data *d)

Are you renaming 'data' -> 'd'
just for your personal preference?


>  {
> -       struct uniphier_gpio_priv *priv = data->chip_data;
> -       u32 mask = BIT(data->hwirq);
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct uniphier_gpio_priv *priv = gpiochip_get_data(gc);
> +       u32 mask = BIT(d->hwirq);
>
>         uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_EN, mask, mask);
>
> -       return irq_chip_unmask_parent(data);
> +       return irq_chip_unmask_parent(d);
>  }
>
> -static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type)
> +static int uniphier_gpio_irq_set_type(struct irq_data *d, unsigned int type)

Again, this seems a noise change.


>  {
> -       struct uniphier_gpio_priv *priv = data->chip_data;
> -       u32 mask = BIT(data->hwirq);
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct uniphier_gpio_priv *priv = gpiochip_get_data(gc);
> +       u32 mask = BIT(d->hwirq);
>         u32 val = 0;
>
>         if (type == IRQ_TYPE_EDGE_BOTH) {
> @@ -216,7 +198,7 @@ static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type)
>         /* To enable both edge detection, the noise filter must be enabled. */
>         uniphier_gpio_reg_update(priv, UNIPHIER_GPIO_IRQ_FLT_EN, mask, val);
>
> -       return irq_chip_set_type_parent(data, type);
> +       return irq_chip_set_type_parent(d, type);
>  }
>
>  static int uniphier_gpio_irq_get_parent_hwirq(struct uniphier_gpio_priv *priv,
> @@ -245,82 +227,6 @@ static int uniphier_gpio_irq_get_parent_hwirq(struct uniphier_gpio_priv *priv,
>         return -ENOENT;
>  }
>
> -static int uniphier_gpio_irq_domain_translate(struct irq_domain *domain,
> -                                             struct irq_fwspec *fwspec,
> -                                             unsigned long *out_hwirq,
> -                                             unsigned int *out_type)
> -{
> -       if (WARN_ON(fwspec->param_count < 2))
> -               return -EINVAL;
> -
> -       *out_hwirq = fwspec->param[0];
> -       *out_type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> -
> -       return 0;
> -}
> -
> -static int uniphier_gpio_irq_domain_alloc(struct irq_domain *domain,
> -                                         unsigned int virq,
> -                                         unsigned int nr_irqs, void *arg)
> -{
> -       struct uniphier_gpio_priv *priv = domain->host_data;
> -       struct irq_fwspec parent_fwspec;
> -       irq_hw_number_t hwirq;
> -       unsigned int type;
> -       int ret;
> -
> -       if (WARN_ON(nr_irqs != 1))
> -               return -EINVAL;
> -
> -       ret = uniphier_gpio_irq_domain_translate(domain, arg, &hwirq, &type);
> -       if (ret)
> -               return ret;
> -
> -       ret = uniphier_gpio_irq_get_parent_hwirq(priv, hwirq);
> -       if (ret < 0)
> -               return ret;
> -
> -       /* parent is UniPhier AIDET */
> -       parent_fwspec.fwnode = domain->parent->fwnode;
> -       parent_fwspec.param_count = 2;
> -       parent_fwspec.param[0] = ret;
> -       parent_fwspec.param[1] = (type == IRQ_TYPE_EDGE_BOTH) ?
> -                                               IRQ_TYPE_EDGE_FALLING : type;
> -
> -       ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> -                                           &priv->irq_chip, priv);
> -       if (ret)
> -               return ret;
> -
> -       return irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
> -}
> -
> -static int uniphier_gpio_irq_domain_activate(struct irq_domain *domain,
> -                                            struct irq_data *data, bool early)
> -{
> -       struct uniphier_gpio_priv *priv = domain->host_data;
> -       struct gpio_chip *chip = &priv->chip;
> -
> -       return gpiochip_lock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET);
> -}
> -
> -static void uniphier_gpio_irq_domain_deactivate(struct irq_domain *domain,
> -                                               struct irq_data *data)
> -{
> -       struct uniphier_gpio_priv *priv = domain->host_data;
> -       struct gpio_chip *chip = &priv->chip;
> -
> -       gpiochip_unlock_as_irq(chip, data->hwirq + UNIPHIER_GPIO_IRQ_OFFSET);
> -}



I did not test this patch, but probably it would break my board.


->(de)activate hook has offset  UNIPHIER_GPIO_IRQ_OFFSET (=120),
but you are replacing it with generic  gpiochip_irq_domain_activate,
which as zero offset.




>         ret = devm_gpiochip_add_data(dev, chip, priv);
>         if (ret)
>                 return ret;
>
> -       priv->domain = irq_domain_create_hierarchy(
> -                                       parent_domain, 0,
> -                                       UNIPHIER_GPIO_IRQ_MAX_NUM,

You are replacing UNIPHIER_GPIO_IRQ_MAX_NUM with gc->ngpio,
which will much more irqs than needed.

Is it possible to provide more flexibility?






-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Masahiro Yamada]

On Mon, Jun 24, 2019 at 10:25 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Hierarchical IRQ domains can be used to stack different IRQ
> controllers on top of each other.
>
> Bring hierarchical IRQ domains into the GPIOLIB core with the
> following basic idea:
>
> Drivers that need their interrupts handled hierarchically
> specify a callback to translate the child hardware IRQ and
> IRQ type for each GPIO offset to a parent hardware IRQ and
> parent hardware IRQ type.
>
> Users have to pass the callback, fwnode, and parent irqdomain
> before calling gpiochip_irqchip_add().
>
> We use the new method of just filling in the struct
> gpio_irq_chip before adding the gpiochip for all hierarchical
> irqchips of this type.
>
> The code path for device tree is pretty straight-forward,
> while the code path for old boardfiles or anything else will
> be more convoluted requireing upfront allocation of the
> interrupts when adding the chip.
>
> One specific use-case where this can be useful is if a power
> management controller has top-level controls for wakeup
> interrupts. In such cases, the power management controller can
> be a parent to other interrupt controllers and program
> additional registers when an IRQ has its wake capability
> enabled or disabled.
>
> The hierarchical irqchip helper code will only be available
> when IRQ_DOMAIN_HIERARCHY is selected to GPIO chips using
> this should select or depend on that symbol. When using
> hierarchical IRQs, the parent interrupt controller must
> also be hierarchical all the way up to the top interrupt
> controller wireing directly into the CPU, so on systems
> that do not have this we can get rid of all the extra
> code for supporting hierarchical irqs.
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Marc Zyngier <marc.zyngier@arm.com>
> Cc: Lina Iyer <ilina@codeaurora.org>
> Cc: Jon Hunter <jonathanh@nvidia.com>
> Cc: Sowjanya Komatineni <skomatineni@nvidia.com>
> Cc: Bitan Biswas <bbiswas@nvidia.com>
> Cc: linux-tegra@vger.kernel.org
> Cc: David Daney <david.daney@cavium.com>
> Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
> Cc: Brian Masney <masneyb@onstation.org>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog RFC->v1:
> - Tested on real hardware
> - Incorporate Thierry's idea to have a translation callback.
>   He was right about this approach, I was wrong in insisting
>   on IRQ maps.
> ---
>  Documentation/driver-api/gpio/driver.rst | 120 ++++++++--
>  drivers/gpio/gpiolib.c                   | 285 ++++++++++++++++++++++-
>  include/linux/gpio/driver.h              |  46 ++++
>  3 files changed, 426 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/driver-api/gpio/driver.rst b/Documentation/driver-api/gpio/driver.rst
> index 1ce7fcd0f989..3099c7fbefdb 100644
> --- a/Documentation/driver-api/gpio/driver.rst
> +++ b/Documentation/driver-api/gpio/driver.rst
> @@ -259,7 +259,7 @@ most often cascaded off a parent interrupt controller, and in some special
>  cases the GPIO logic is melded with a SoC's primary interrupt controller.
>
>  The IRQ portions of the GPIO block are implemented using an irq_chip, using
> -the header <linux/irq.h>. So basically such a driver is utilizing two sub-
> +the header <linux/irq.h>. So this combined driver is utilizing two sub-
>  systems simultaneously: gpio and irq.
>
>  It is legal for any IRQ consumer to request an IRQ from any irqchip even if it
> @@ -391,14 +391,108 @@ Infrastructure helpers for GPIO irqchips
>  ----------------------------------------
>
>  To help out in handling the set-up and management of GPIO irqchips and the
> -associated irqdomain and resource allocation callbacks, the gpiolib has
> -some helpers that can be enabled by selecting the GPIOLIB_IRQCHIP Kconfig
> -symbol:
> -
> -- gpiochip_irqchip_add(): adds a chained cascaded irqchip to a gpiochip. It
> -  will pass the struct gpio_chip* for the chip to all IRQ callbacks, so the
> -  callbacks need to embed the gpio_chip in its state container and obtain a
> -  pointer to the container using container_of().
> +associated irqdomain and resource allocation callbacks. These are activated
> +by selecting the Kconfig symbol GPIOLIB_IRQCHIP. If the symbol
> +IRQ_DOMAIN_HIERARCHY is also selected, hierarchical helpers will also be
> +provided. A big portion of overhead code will be managed by gpiolib,
> +under the assumption that your interrupts are 1-to-1-mapped to the
> +GPIO line index:
> +
> +  GPIO line offset   Hardware IRQ
> +  0                  0
> +  1                  1
> +  2                  2
> +  ...                ...
> +  ngpio-1            ngpio-1
> +
> +If some GPIO lines do not have corresponding IRQs, the bitmask valid_mask
> +and the flag need_valid_mask in gpio_irq_chip can be used to mask off some
> +lines as invalid for associating with IRQs.
> +
> +The preferred way to set up the helpers is to fill in the
> +struct gpio_irq_chip inside struct gpio_chip before adding the gpio_chip.
> +If you do this, the additional irq_chip will be set up by gpiolib at the
> +same time as setting up the rest of the GPIO functionality. The following
> +is a typical example of a cascaded interrupt handler using gpio_irq_chip:
> +
> +  /* Typical state container with dynamic irqchip */
> +  struct my_gpio {
> +      struct gpio_chip gc;
> +      struct irq_chip irq;
> +  };
> +
> +  int irq; /* from platform etc */
> +  struct my_gpio *g;
> +  struct gpio_irq_chip *girq
> +
> +  /* Set up the irqchip dynamically */
> +  g->irq.name = "my_gpio_irq";
> +  g->irq.irq_ack = my_gpio_ack_irq;
> +  g->irq.irq_mask = my_gpio_mask_irq;
> +  g->irq.irq_unmask = my_gpio_unmask_irq;
> +  g->irq.irq_set_type = my_gpio_set_irq_type;
> +
> +  /* Get a pointer to the gpio_irq_chip */
> +  girq = &g->gc.irq;
> +  girq->chip = &g->irq;
> +  girq->parent_handler = ftgpio_gpio_irq_handler;
> +  girq->num_parents = 1;
> +  girq->parents = devm_kcalloc(dev, 1, sizeof(*girq->parents),
> +                               GFP_KERNEL);
> +  if (!girq->parents)
> +      return -ENOMEM;
> +  girq->default_type = IRQ_TYPE_NONE;
> +  girq->handler = handle_bad_irq;
> +  girq->parents[0] = irq;
> +
> +  return devm_gpiochip_add_data(dev, &g->gc, g);
> +
> +The helper support using hierarchical interrupt controllers as well.
> +In this case the typical set-up will look like this:
> +
> +  /* Typical state container with dynamic irqchip */
> +  struct my_gpio {
> +      struct gpio_chip gc;
> +      struct irq_chip irq;
> +      struct fwnode_handle *fwnode;
> +  };
> +
> +  int irq; /* from platform etc */
> +  struct my_gpio *g;
> +  struct gpio_irq_chip *girq
> +
> +  /* Set up the irqchip dynamically */
> +  g->irq.name = "my_gpio_irq";
> +  g->irq.irq_ack = my_gpio_ack_irq;
> +  g->irq.irq_mask = my_gpio_mask_irq;
> +  g->irq.irq_unmask = my_gpio_unmask_irq;
> +  g->irq.irq_set_type = my_gpio_set_irq_type;
> +
> +  /* Get a pointer to the gpio_irq_chip */
> +  girq = &g->gc.irq;
> +  girq->chip = &g->irq;
> +  girq->default_type = IRQ_TYPE_NONE;
> +  girq->handler = handle_bad_irq;
> +  girq->fwnode = g->fwnode;
> +  girq->parent_domain = parent;
> +  girq->child_to_parent_hwirq = my_gpio_child_to_parent_hwirq;
> +
> +  return devm_gpiochip_add_data(dev, &g->gc, g);
> +
> +As you can see pretty similar, but you do not supply a parent handler for
> +the IRQ, instead a parent irqdomain, an fwnode for the hardware and
> +a funcion .child_to_parent_hwirq() that has the purpose of looking up
> +the parent hardware irq from a child (i.e. this gpio chip) hardware irq.
> +As always it is good to look at examples in the kernel tree for advice
> +on how to find the required pieces.
> +
> +The old way of adding irqchips to gpiochips after registration is also still
> +available but we try to move away from this:
> +
> +- DEPRECATED: gpiochip_irqchip_add(): adds a chained cascaded irqchip to a
> +  gpiochip. It will pass the struct gpio_chip* for the chip to all IRQ
> +  callbacks, so the callbacks need to embed the gpio_chip in its state
> +  container and obtain a pointer to the container using container_of().
>    (See Documentation/driver-model/design-patterns.txt)
>
>  - gpiochip_irqchip_add_nested(): adds a nested cascaded irqchip to a gpiochip,
> @@ -406,10 +500,10 @@ symbol:
>    cascaded irq has to be handled by a threaded interrupt handler.
>    Apart from that it works exactly like the chained irqchip.
>
> -- gpiochip_set_chained_irqchip(): sets up a chained cascaded irq handler for a
> -  gpio_chip from a parent IRQ and passes the struct gpio_chip* as handler
> -  data. Notice that we pass is as the handler data, since the irqchip data is
> -  likely used by the parent irqchip.
> +- DEPRECATED: gpiochip_set_chained_irqchip(): sets up a chained cascaded irq
> +  handler for a gpio_chip from a parent IRQ and passes the struct gpio_chip*
> +  as handler data. Notice that we pass is as the handler data, since the
> +  irqchip data is likely used by the parent irqchip.
>
>  - gpiochip_set_nested_irqchip(): sets up a nested cascaded irq handler for a
>    gpio_chip from a parent IRQ. As the parent IRQ has usually been
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index e013d417a936..af72ffa02963 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1718,6 +1718,240 @@ void gpiochip_set_nested_irqchip(struct gpio_chip *gpiochip,
>  }
>  EXPORT_SYMBOL_GPL(gpiochip_set_nested_irqchip);
>
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +
> +/**
> + * gpiochip_set_hierarchical_irqchip() - connects a hierarchical irqchip
> + * to a gpiochip
> + * @gc: the gpiochip to set the irqchip hierarchical handler to
> + * @irqchip: the irqchip to handle this level of the hierarchy, the interrupt
> + * will then percolate up to the parent
> + */
> +static void gpiochip_set_hierarchical_irqchip(struct gpio_chip *gc,
> +                                             struct irq_chip *irqchip)
> +{
> +       /* DT will deal with mapping each IRQ as we go along */
> +       if (is_of_node(gc->irq.fwnode))
> +               return;
> +
> +       /*
> +        * This is for legacy and boardfile "irqchip" fwnodes: allocate
> +        * irqs upfront instead of dynamically since we don't have the
> +        * dynamic type of allocation that hardware description languages
> +        * provide. Once all GPIO drivers using board files are gone from
> +        * the kernel we can delete this code, but for a transitional period
> +        * it is necessary to keep this around.
> +        */
> +       if (is_fwnode_irqchip(gc->irq.fwnode)) {
> +               int i;
> +               int ret;
> +
> +               for (i = 0; i < gc->ngpio; i++) {
> +                       struct irq_fwspec fwspec;
> +                       unsigned int parent_hwirq;
> +                       unsigned int parent_type;
> +                       struct gpio_irq_chip *girq = &gc->irq;
> +
> +                       /*
> +                        * We call the child to parent translation function
> +                        * only to check if the child IRQ is valid or not.
> +                        * Just pick the rising edge type here as that is what
> +                        * we likely need to support.
> +                        */
> +                       ret = girq->child_to_parent_hwirq(gc, i,
> +                                                         IRQ_TYPE_EDGE_RISING,
> +                                                         &parent_hwirq,
> +                                                         &parent_type);
> +                       if (ret) {
> +                               chip_err(gc, "skip set-up on hwirq %d\n",
> +                                        i);
> +                               continue;
> +                       }
> +
> +                       fwspec.fwnode = gc->irq.fwnode;
> +                       /* This is the hwirq for the GPIO line side of things */
> +                       fwspec.param[0] = i;
> +                       /* Just pick something */
> +                       fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> +                       fwspec.param_count = 2;
> +                       ret = __irq_domain_alloc_irqs(gc->irq.domain,
> +                                                     /* just pick something */
> +                                                     -1,
> +                                                     1,
> +                                                     NUMA_NO_NODE,
> +                                                     &fwspec,
> +                                                     false,
> +                                                     NULL);
> +                       if (ret < 0) {
> +                               chip_err(gc,
> +                                        "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n",
> +                                        i, parent_hwirq,
> +                                        ret);
> +                       }
> +               }
> +       }
> +
> +       chip_err(gc, "%s unknown fwnode type proceed anyway\n", __func__);
> +
> +       return;
> +}
> +
> +static int gpiochip_hierarchy_irq_domain_translate(struct irq_domain *d,
> +                                                  struct irq_fwspec *fwspec,
> +                                                  unsigned long *hwirq,
> +                                                  unsigned int *type)
> +{
> +       /* We support standard DT translation */
> +       if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
> +               return irq_domain_translate_twocell(d, fwspec, hwirq, type);
> +       }
> +
> +       /* This is for board files and others not using DT */
> +       if (is_fwnode_irqchip(fwspec->fwnode)) {
> +               int ret;
> +
> +               ret = irq_domain_translate_twocell(d, fwspec, hwirq, type);
> +               if (ret)
> +                       return ret;
> +               WARN_ON(*type == IRQ_TYPE_NONE);
> +               return 0;
> +       }
> +       return -EINVAL;
> +}
> +
> +static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
> +                                              unsigned int irq,
> +                                              unsigned int nr_irqs,
> +                                              void *data)
> +{
> +       struct gpio_chip *gc = d->host_data;
> +       irq_hw_number_t hwirq;
> +       unsigned int type = IRQ_TYPE_NONE;
> +       struct irq_fwspec *fwspec = data;
> +       int ret;
> +       int i;


We always expect nr_irqs is 1.

As gpio-uniphier.c, you can error out with WARN_ON
if nr_irqs != 1





> +       chip_info(gc, "called %s\n", __func__);

Is this a debug message?

> +
> +       ret = gpiochip_hierarchy_irq_domain_translate(d, fwspec, &hwirq, &type);
> +       if (ret)
> +               return ret;
> +
> +       chip_info(gc, "allocate IRQ %d..%d, hwirq %lu..%lu\n",
> +                 irq, irq + nr_irqs - 1,
> +                 hwirq, hwirq + nr_irqs - 1);

Ditto.


> +
> +       for (i = 0; i < nr_irqs; i++) {

This for-loop is unneeded because nr_irqs is 1.



> +               struct irq_fwspec parent_fwspec;
> +               unsigned int parent_hwirq;
> +               unsigned int parent_type;
> +               struct gpio_irq_chip *girq = &gc->irq;
> +
> +               ret = girq->child_to_parent_hwirq(gc, hwirq, type,
> +                                                 &parent_hwirq, &parent_type);
> +               if (ret) {
> +                       chip_err(gc, "can't look up hwirq %lu\n", hwirq);
> +                       return ret;
> +               }
> +               chip_info(gc, "found parent hwirq %u\n", parent_hwirq);
> +
> +               /*
> +                * We set handle_bad_irq because the .set_type() should
> +                * always be invoked and set the right type of handler.
> +                */
> +               irq_domain_set_info(d,
> +                                   irq + i,
> +                                   hwirq + i,
> +                                   gc->irq.chip,
> +                                   gc,
> +                                   handle_bad_irq,
> +                                   NULL, NULL);
> +               irq_set_probe(irq + i);
> +
> +               /*
> +                * Create a IRQ fwspec to send up to the parent irqdomain:
> +                * specify the hwirq we address on the parent and tie it
> +                * all together up the chain.
> +                */
> +               parent_fwspec.fwnode = d->parent->fwnode;
> +               parent_fwspec.param_count = 2;
> +               parent_fwspec.param[0] = parent_hwirq;
> +               /* This parent only handles asserted level IRQs */
> +               parent_fwspec.param[1] = parent_type;
> +               chip_info(gc, "alloc_irqs_parent for %d parent hwirq %d\n",
> +                         irq + i, parent_hwirq);
> +               ret = irq_domain_alloc_irqs_parent(d, irq + i, 1,
> +                                                  &parent_fwspec);
> +               if (ret)
> +                       chip_err(gc,
> +                                "failed to allocate parent hwirq %d for hwirq %lu\n",
> +                                parent_hwirq, hwirq);
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct irq_domain_ops gpiochip_hierarchy_domain_ops = {
> +       .activate = gpiochip_irq_domain_activate,
> +       .deactivate = gpiochip_irq_domain_deactivate,
> +       .translate = gpiochip_hierarchy_irq_domain_translate,
> +       .alloc = gpiochip_hierarchy_irq_domain_alloc,
> +       .free = irq_domain_free_irqs_common,
> +};
> +
> +static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
> +{
> +       if (!gc->irq.parent_domain) {
> +               chip_err(gc, "missing parent irqdomain\n");
> +               return -EINVAL;
> +       }

Is this check needed?

gpiochip_hierarchy_is_hierarchical() has already checked this.



> +       if (!gc->irq.parent_domain ||

Redundant check second time.


> +           !gc->irq.child_to_parent_hwirq ||
> +           !gc->irq.fwnode) {
> +               chip_err(gc, "missing irqdomain vital data\n");
> +               return -EINVAL;
> +       }
> +
> +       gc->irq.domain = irq_domain_create_hierarchy(
> +               gc->irq.parent_domain,
> +               IRQ_DOMAIN_FLAG_HIERARCHY,

You do not need to pass IRQ_DOMAIN_FLAG_HIERARCHY.

This is set by irq_domain_check_hierarchy().



> +               gc->ngpio,
> +               gc->irq.fwnode,
> +               &gpiochip_hierarchy_domain_ops,
> +               gc);
> +
> +       if (!gc->irq.domain) {
> +               chip_err(gc, "failed to add hierarchical domain\n");
> +               return -EINVAL;


This should be ...

                  return -ENOMEM;

because __irq_domain_add() fails when memory allocation fails.
Obviously, this also means you should drop chip_err() above
since we do not print error message for ENOMEM.






> +       }
> +
> +       gpiochip_set_hierarchical_irqchip(gc, gc->irq.chip);
> +
> +       chip_info(gc, "set up hierarchical irqdomain\n");


I see so many chip_info().

I think they should be chip_dbg() or removed entirely.


> +       return 0;
> +}
> +
> +static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)
> +{
> +       return !!gc->irq.parent_domain;
> +}
> +
> +#else
> +
> +static int gpiochip_hierarchy_add_domain(struct gpio_chip *gc)
> +{
> +       return -EINVAL;
> +}
> +
> +static bool gpiochip_hierarchy_is_hierarchical(struct gpio_chip *gc)
> +{
> +       return false;
> +}
> +
> +#endif /* CONFIG_IRQ_DOMAIN_HIERARCHY */
> +
>  /**
>   * gpiochip_irq_map() - maps an IRQ into a GPIO irqchip
>   * @d: the irqdomain used by this irqchip
> @@ -1786,6 +2020,11 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
>         .xlate  = irq_domain_xlate_twocell,
>  };
>
> +/*
> + * TODO: move these activate/deactivate in under the hierarchicial
> + * irqchip implementation as static once SPMI and SSBI (all external
> + * users) are phased over.
> + */
>  /**
>   * gpiochip_irq_domain_activate() - Lock a GPIO to be used as an IRQ
>   * @domain: The IRQ domain used by this IRQ chip
> @@ -1825,10 +2064,23 @@ EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate);
>
>  static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
>  {
> +       struct irq_domain *domain = chip->irq.domain;
> +
>         if (!gpiochip_irqchip_irq_valid(chip, offset))
>                 return -ENXIO;
>
> -       return irq_create_mapping(chip->irq.domain, offset);
> +       if (irq_domain_is_hierarchy(domain)) {
> +               struct irq_fwspec spec;
> +
> +               spec.fwnode = domain->fwnode;
> +               spec.param_count = 2;
> +               spec.param[0] = offset;
> +               spec.param[1] = IRQ_TYPE_NONE;
> +
> +               return irq_create_fwspec_mapping(&spec);
> +       }
> +
> +       return irq_create_mapping(domain, offset);
>  }
>
>  static int gpiochip_irq_reqres(struct irq_data *d)
> @@ -1905,7 +2157,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
>                                 struct lock_class_key *request_key)
>  {
>         struct irq_chip *irqchip = gpiochip->irq.chip;
> -       const struct irq_domain_ops *ops;
> +       const struct irq_domain_ops *ops = NULL;
>         struct device_node *np;
>         unsigned int type;
>         unsigned int i;
> @@ -1941,16 +2193,25 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip,
>         gpiochip->irq.lock_key = lock_key;
>         gpiochip->irq.request_key = request_key;
>
> -       if (gpiochip->irq.domain_ops)
> -               ops = gpiochip->irq.domain_ops;
> -       else
> -               ops = &gpiochip_domain_ops;
> -
> -       gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
> -                                                    gpiochip->irq.first,
> -                                                    ops, gpiochip);
> -       if (!gpiochip->irq.domain)
> -               return -EINVAL;
> +       /* If a parent irqdomain is provided, let's build a hierarchy */
> +       if (gpiochip_hierarchy_is_hierarchical(gpiochip)) {
> +               int ret = gpiochip_hierarchy_add_domain(gpiochip);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               /* Some drivers provide custom irqdomain ops */
> +               if (gpiochip->irq.domain_ops)
> +                       ops = gpiochip->irq.domain_ops;
> +
> +               if (!ops)
> +                       ops = &gpiochip_domain_ops;
> +               gpiochip->irq.domain = irq_domain_add_simple(np,
> +                       gpiochip->ngpio,
> +                       gpiochip->irq.first,
> +                       ops, gpiochip);
> +               if (!gpiochip->irq.domain)
> +                       return -EINVAL;
> +       }
>
>         if (gpiochip->irq.parent_handler) {
>                 void *data = gpiochip->irq.parent_handler_data ?: gpiochip;
> diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
> index a1d273c96016..e32d02cb2d08 100644
> --- a/include/linux/gpio/driver.h
> +++ b/include/linux/gpio/driver.h
> @@ -22,6 +22,9 @@ enum gpiod_flags;
>  #ifdef CONFIG_GPIOLIB
>
>  #ifdef CONFIG_GPIOLIB_IRQCHIP
> +
> +struct gpio_chip;
> +
>  /**
>   * struct gpio_irq_chip - GPIO interrupt controller
>   */
> @@ -48,6 +51,49 @@ struct gpio_irq_chip {
>          */
>         const struct irq_domain_ops *domain_ops;
>
> +#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
> +       /**
> +        * @fwnode:
> +        *
> +        * Firmware node corresponding to this gpiochip/irqchip, necessary
> +        * for hierarchical irqdomain support.
> +        */
> +       struct fwnode_handle *fwnode;
> +
> +       /**
> +        * @parent_domain:
> +        *
> +        * If non-NULL, will be set as the parent of this GPIO interrupt
> +        * controller's IRQ domain to establish a hierarchical interrupt
> +        * domain. The presence of this will activate the hierarchical
> +        * interrupt support.
> +        */
> +       struct irq_domain *parent_domain;
> +
> +       /**
> +        * @child_to_parent_hwirq:
> +        *
> +        * This callback translates a child hardware IRQ offset to a parent
> +        * hardware IRQ offset on a hierarchical interrupt chip. The child
> +        * hardware IRQs correspond to the GPIO index 0..ngpio-1 (see the
> +        * ngpio field of struct gpio_chip) and the corresponding parent
> +        * hardware IRQ and type (such as IRQ_TYPE_*) shall be returned by
> +        * the driver. The driver can calculate this from an offset or using
> +        * a lookup table or whatever method is best for this chip. Return
> +        * 0 on successful translation in the driver.
> +        *
> +        * If some ranges of hardware IRQs do not have a corresponding parent
> +        * HWIRQ, return -EINVAL, but also make sure to fill in @valid_mask and
> +        * @need_valid_mask to make these GPIO lines unavailable for
> +        * translation.
> +        */
> +       int (*child_to_parent_hwirq)(struct gpio_chip *chip,
> +                                    unsigned int child_hwirq,
> +                                    unsigned int child_type,
> +                                    unsigned int *parent_hwirq,
> +                                    unsigned int *parent_type);
> +#endif
> +
>         /**
>          * @handler:
>          *
> --
> 2.21.0
>


--
Best Regards

Masahiro Yamada

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Linus Walleij]

Hi Masahiro,

On Thu, Jul 18, 2019 at 1:12 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> On Mon, Jun 24, 2019 at 10:25 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> > +static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
> > +                                              unsigned int irq,
> > +                                              unsigned int nr_irqs,
> > +                                              void *data)
> > +{
> > +       struct gpio_chip *gc = d->host_data;
> > +       irq_hw_number_t hwirq;
> > +       unsigned int type = IRQ_TYPE_NONE;
> > +       struct irq_fwspec *fwspec = data;
> > +       int ret;
> > +       int i;
>
> We always expect nr_irqs is 1.
>
> As gpio-uniphier.c, you can error out with WARN_ON
> if nr_irqs != 1

Hm, yes I am pretty sure it is always 1.

But I'd like to defer changing this until/if Marc changes
the signature of the function to not pass nr_irqs anymore.
I try to design for the current prototype because I don't
know how e.g. ACPI works with respect to this.

> I see so many chip_info().
> I think they should be chip_dbg() or removed entirely.

I am keeping that right now as we're testing on several
different systems, so some extra debug prints should be
OK in a transitional period. We might change it into dbg
with a separate patch before the merge window.

The rest of your comments are addressed!

Yours,
Linus Walleij

Re: [PATCH 1/4 v1] gpio: Add support for hierarchical IRQ domains

[Marc Zyngier]

On 07/08/2019 15:43, Linus Walleij wrote:
> Hi Masahiro,
> 
> On Thu, Jul 18, 2019 at 1:12 PM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
>> On Mon, Jun 24, 2019 at 10:25 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> 
>>> +static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d,
>>> +                                              unsigned int irq,
>>> +                                              unsigned int nr_irqs,
>>> +                                              void *data)
>>> +{
>>> +       struct gpio_chip *gc = d->host_data;
>>> +       irq_hw_number_t hwirq;
>>> +       unsigned int type = IRQ_TYPE_NONE;
>>> +       struct irq_fwspec *fwspec = data;
>>> +       int ret;
>>> +       int i;
>>
>> We always expect nr_irqs is 1.
>>
>> As gpio-uniphier.c, you can error out with WARN_ON
>> if nr_irqs != 1
> 
> Hm, yes I am pretty sure it is always 1.
> 
> But I'd like to defer changing this until/if Marc changes
> the signature of the function to not pass nr_irqs anymore.
> I try to design for the current prototype because I don't
> know how e.g. ACPI works with respect to this.

nr_irqs is only here for one single case: PCI Multi-MSI, where we have
to allocate a bunch of contiguous hwirqs. In all other cases, nr_irqs is
always 1.

So yes, you can safely assume nr_irqs == 1, and WARN_ON otherwise.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 4/4 v1] RFT: gpio: uniphier: Switch to GPIOLIB_IRQCHIP

[Linus Walleij]

Hi Masahiro,

thanks for your review!

On Thu, Jul 18, 2019 at 1:10 PM Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> On Mon, Jun 24, 2019 at 10:25 PM Linus Walleij <linus.walleij@linaro.org> wrote:

> > -static void uniphier_gpio_irq_unmask(struct irq_data *data)
> > +static void uniphier_gpio_irq_unmask(struct irq_data *d)
>
> Are you renaming 'data' -> 'd'
> just for your personal preference?

Yes, I am still looking for proof of what kind of terseness gives
the optimal perceptive qualities in written code, so I have only
intuitive ideas about what is the easiest code to read and maintain.

But it's your file, and since you seem not to like it I will
back out this change.

> > -static int uniphier_gpio_irq_set_type(struct irq_data *data, unsigned int type)
> > +static int uniphier_gpio_irq_set_type(struct irq_data *d, unsigned int type)
>
> Again, this seems a noise change.

Contrary to popular belief, coding style changes while writing
new code is not universally seen as "noise", actually the opposite,
sending pure code style changes as singular patches, is seen as
noise. See the following paragraph from
Documentation/process/4.Coding.rst:

"pure coding style fixes are seen as noise by the development community;
they tend to get a chilly reception.  So this type of patch is best
avoided.  It is natural to fix the style of a piece of code while working
on it for other reasons, but coding style changes should not be made for
their own sake."

Given the number of pure coding style patches I get,
one could believe this does not apply ... but I try to be
accepting of it anyway.

> I did not test this patch, but probably it would break my board.

Oh too bad, let's see if we can make it more plausible to
work (I will not apply it while it is in RFT state).

> ->(de)activate hook has offset  UNIPHIER_GPIO_IRQ_OFFSET (=120),
> but you are replacing it with generic  gpiochip_irq_domain_activate,
> which as zero offset.

Ah! Brian gave me the tool to fix this properly, I will try to
iterate and get this right.

> > -       priv->domain = irq_domain_create_hierarchy(
> > -                                       parent_domain, 0,
> > -                                       UNIPHIER_GPIO_IRQ_MAX_NUM,
>
> You are replacing UNIPHIER_GPIO_IRQ_MAX_NUM with gc->ngpio,
> which will much more irqs than needed.
>
> Is it possible to provide more flexibility?

UNIPHIER_GPIO_IRQ_MAX_NUM is 24 and ngpio comes
from the device tree and is compulsory. The current device
trees have:
arch/arm/boot/dts/uniphier-ld4.dtsi:                    ngpios = <136>;
arch/arm/boot/dts/uniphier-pro4.dtsi:                   ngpios = <248>;
arch/arm/boot/dts/uniphier-pro5.dtsi:                   ngpios = <248>;
arch/arm/boot/dts/uniphier-pxs2.dtsi:                   ngpios = <232>;
arch/arm/boot/dts/uniphier-sld8.dtsi:                   ngpios = <136>;

So I suppose that you mean that since only 24 GPIOs can
ever have assigned IRQs, making headroom for say 248 is
a waste of resources.

However irq descriptors are dynamically allocated, so saying
that the irqchip can have 24 descriptors rather than 248
is not going to save any memory.

What you might want is to only allow offset 0..23 to be mapped
to irqs. If I understand correctly this is how the hardware works:
the first 24 GPIOs have IRQs, the rest don't, is that right?

We have a facility for that: struct gpio_irq_chip field
.valid_mask

I will try to come up with a separate patch for that so you can
see if it works.

Yours,
Linus Walleij