[PATCH 0/9] irqchip: meson: add support for the gpio interrupt controller

[PATCH 0/9] irqchip: meson: add support for the gpio interrupt controller

[Jerome Brunet]

This patch series adds support for the GPIO interrupt controller found
on Amlogic's meson SoC families.

Unlike what the name suggests, this controller is not part of the SoC GPIO
subsystem. It's an indepedent controller which can watch almost all pad of
the SoC and generate and interrupt from it. Some pins, which are not part
of the public datasheet, don't seem to have this capability though.

Hardware wise, the controller is a 256 to 8 multiplexer. It can take up
to 256 input pads and route them to any of 8 GIC's interrupts. There is
also a filter block in the middle to select the appropriate edge or level.

The number of interrupt declared by the irqchip is lowered from 256 to the
actual number of signal routed to the controller on each SoC family. As we
have access to only 8 GIC’s interrupts, these are allocated when an
interrupt is requested from the controller, on a first come, first served
basis.

This series has been tested on Amlogic S905-P200 board with the front
panel power button. Directly passing an IRQ or using gpio_to_irq both work
with this driver.

This work is derived from the previous work of Carlo Caione [1].

Changes since RFC : [2]
 * Remove interrupt property in device tree: the controller cannot generate
   interrupts on its own and is merely routing the interrupt to the GIC,
   therefore it should not use the interrupt property. This data is now
   stored directly in the driver, same as the pinctrl data.

 * Improve compatibility checking of meson pinctrl on its interrupt parent
   to activate gpio_to_irq callback

 * Drop IRQ_BOTH hack. Need more work to have an acceptable solution for
   this

[1] : http://lkml.kernel.org/r/1448987062-31225-1-git-send-email-carlo@caione.org
[2] : http://lkml.kernel.org/r/1475593708-10526-1-git-send-email-jbrunet@baylibre.com

Jerome Brunet (9):
  irqchip: meson: add support for gpio interrupt controller
  dt-bindings: interrupt-controller: add DT binding for meson GPIO
    interrupt controller
  pinctrl: meson: update pinctrl data with gpio irq data
  pinctrl: meson: allow gpio to request irq
  dt-bindings: pinctrl: meson: update gpio dt-bindings
  ARM64: meson: enable MESON_IRQ_GPIO in Kconfig
  ARM: meson: enable MESON_IRQ_GPIO in Kconfig for meson8
  ARM64: dts: amlogic: enable gpio interrupt controller on gxbb
  ARM: dts: amlogic: enable gpio interrupt controller on meson8

 .../amlogic,meson-gpio-intc.txt                    |  31 ++
 .../devicetree/bindings/pinctrl/meson,pinctrl.txt  |   4 +
 arch/arm/boot/dts/meson8.dtsi                      |  11 +
 arch/arm/boot/dts/meson8b.dtsi                     |  11 +
 arch/arm/mach-meson/Kconfig                        |   2 +
 arch/arm64/Kconfig.platforms                       |   1 +
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |   9 +
 drivers/irqchip/Kconfig                            |   9 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-meson-gpio.c                   | 423 +++++++++++++++++++++
 drivers/pinctrl/Kconfig                            |   2 +
 drivers/pinctrl/meson/pinctrl-meson-gxbb.c         |  24 +-
 drivers/pinctrl/meson/pinctrl-meson.c              |  77 +++-
 drivers/pinctrl/meson/pinctrl-meson.h              |  17 +-
 drivers/pinctrl/meson/pinctrl-meson8.c             |  22 +-
 drivers/pinctrl/meson/pinctrl-meson8b.c            |  34 +-
 16 files changed, 641 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

-- 
2.7.4

[PATCH 1/9] irqchip: meson: add support for gpio interrupt controller

[Jerome Brunet]

Add support for the interrupt gpio controller found on Amlogic's meson
SoC family.

Unlike what the IP name suggest, it is not directly linked to the gpio
subsystem. It is actually an independent IP that is able to spy on the
SoC pad. For that purpose, it can mux and filter (edge or level and
polarity) any single SoC pad to one of the 8 GIC's interrupts it owns.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/irqchip/Kconfig          |   9 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 423 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 433 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 82b0b5daf3f5..168837263e80 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -279,3 +279,12 @@ config EZNPS_GIC
 config STM32_EXTI
 	bool
 	select IRQ_DOMAIN
+
+config MESON_GPIO_IRQ
+       bool "Meson GPIO Interrupt Multiplexer"
+       depends on ARCH_MESON || COMPILE_TEST
+       select IRQ_DOMAIN
+       select IRQ_DOMAIN_HIERARCHY
+       help
+         Support Meson SoC Family GPIO Interrupt Multiplexer
+
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index e4dbfc85abdb..33f913d037d0 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -74,3 +74,4 @@ obj-$(CONFIG_LS_SCFG_MSI)		+= irq-ls-scfg-msi.o
 obj-$(CONFIG_EZNPS_GIC)			+= irq-eznps.o
 obj-$(CONFIG_ARCH_ASPEED)		+= irq-aspeed-vic.o
 obj-$(CONFIG_STM32_EXTI) 		+= irq-stm32-exti.o
+obj-$(CONFIG_MESON_GPIO_IRQ)		+= irq-meson-gpio.o
diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c
new file mode 100644
index 000000000000..869b4df8c483
--- /dev/null
+++ b/drivers/irqchip/irq-meson-gpio.c
@@ -0,0 +1,423 @@
+/*
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <carlo@endlessm.com>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of version 2 of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ * The full GNU General Public License is included in this distribution
+ * in the file called COPYING.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+
+#define IRQ_FREE (-1)
+
+#define REG_EDGE_POL	0x00
+#define REG_PIN_03_SEL	0x04
+#define REG_PIN_47_SEL	0x08
+#define REG_FILTER_SEL	0x0c
+
+#define REG_EDGE_POL_MASK(x)	(BIT(x) | BIT(16 + (x)))
+#define REG_EDGE_POL_EDGE(x)	BIT(x)
+#define REG_EDGE_POL_LOW(x)	BIT(16 + (x))
+#define REG_PIN_SEL_SHIFT(x)	(((x) % 4) * 8)
+#define REG_FILTER_SEL_SHIFT(x)	((x) * 4)
+
+struct meson_gpio_irq_params {
+	unsigned int nhwirq;
+	irq_hw_number_t *source;
+	int nsource;
+};
+
+struct meson_gpio_irq_domain {
+	void __iomem *base;
+	int *map;
+	const struct meson_gpio_irq_params *params;
+};
+
+struct meson_gpio_irq_chip_data {
+	void __iomem *base;
+	int index;
+};
+
+static irq_hw_number_t meson_parent_hwirqs[] = {
+	64, 65, 66, 67, 68, 69, 70, 71,
+};
+
+static const struct meson_gpio_irq_params meson8_params = {
+	.nhwirq  = 134,
+	.source  = meson_parent_hwirqs,
+	.nsource = ARRAY_SIZE(meson_parent_hwirqs),
+};
+
+static const struct meson_gpio_irq_params meson8b_params = {
+	.nhwirq  = 119,
+	.source  = meson_parent_hwirqs,
+	.nsource = ARRAY_SIZE(meson_parent_hwirqs),
+};
+
+static const struct meson_gpio_irq_params meson_gxbb_params = {
+	.nhwirq  = 133,
+	.source  = meson_parent_hwirqs,
+	.nsource = ARRAY_SIZE(meson_parent_hwirqs),
+};
+
+static const struct of_device_id meson_irq_gpio_matches[] = {
+	{
+		.compatible = "amlogic,meson8-gpio-intc",
+		.data = &meson8_params
+	},
+	{
+		.compatible = "amlogic,meson8b-gpio-intc",
+		.data = &meson8b_params
+	},
+	{
+		.compatible = "amlogic,meson-gxbb-gpio-intc",
+		.data = &meson_gxbb_params
+	},
+	{}
+};
+
+static void meson_gpio_irq_update_bits(void __iomem *base, unsigned int reg,
+				       u32 mask, u32 val)
+{
+	u32 tmp;
+
+	tmp = readl(base + reg);
+	tmp &= ~mask;
+	tmp |= val;
+
+	writel(tmp, base + reg);
+}
+
+static int meson_gpio_irq_get_index(struct meson_gpio_irq_domain *domain_data,
+				    int hwirq)
+{
+	int i;
+
+	for (i = 0; i < domain_data->params->nsource; i++) {
+		if (domain_data->map[i] == hwirq)
+			return i;
+	}
+
+	return -1;
+}
+
+static int mesion_gpio_irq_map_source(struct meson_gpio_irq_domain *domain_data,
+				      irq_hw_number_t hwirq,
+				      irq_hw_number_t *source)
+{
+	int index;
+	unsigned int reg;
+
+	index = meson_gpio_irq_get_index(domain_data, IRQ_FREE);
+	if (index < 0) {
+		pr_err("No irq available\n");
+		return -ENOSPC;
+	}
+
+	domain_data->map[index] = hwirq;
+
+	reg = (index < 4) ? REG_PIN_03_SEL : REG_PIN_47_SEL;
+	meson_gpio_irq_update_bits(domain_data->base, reg,
+				   0xff << REG_PIN_SEL_SHIFT(index),
+				   hwirq << REG_PIN_SEL_SHIFT(index));
+
+	*source = domain_data->params->source[index];
+
+	pr_debug("hwirq %lu assigned to channel %d - source %lu\n",
+		 hwirq, index, *source);
+
+	return index;
+}
+
+static int meson_gpio_irq_type_setup(unsigned int type, void __iomem *base,
+				     int index)
+{
+	u32 val = 0;
+
+	type &= IRQ_TYPE_SENSE_MASK;
+
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		return -EINVAL;
+
+	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_EDGE(index);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(index);
+
+	meson_gpio_irq_update_bits(base, REG_EDGE_POL,
+				   REG_EDGE_POL_MASK(index), val);
+
+	return 0;
+}
+
+static unsigned int meson_gpio_irq_type_output(unsigned int type)
+{
+	unsigned int sense = type & IRQ_TYPE_SENSE_MASK;
+
+	type &= ~IRQ_TYPE_SENSE_MASK;
+
+	/*
+	 * If the polarity of interrupt is low, the controller will
+	 * invert the signal for gic
+	 */
+	if (sense & (IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+		type |= IRQ_TYPE_LEVEL_HIGH;
+	else if (sense & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
+		type |= IRQ_TYPE_EDGE_RISING;
+
+	return type;
+}
+
+static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct meson_gpio_irq_chip_data *cd = irq_data_get_irq_chip_data(data);
+	int ret;
+
+	pr_debug("set type of hwirq %lu to %u\n", data->hwirq, type);
+
+	ret = meson_gpio_irq_type_setup(type, cd->base, cd->index);
+	if (ret)
+		return ret;
+
+	return irq_chip_set_type_parent(data,
+					meson_gpio_irq_type_output(type));
+}
+
+static struct irq_chip meson_gpio_irq_chip = {
+	.name			= "meson-gpio-irqchip",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_type		= meson_gpio_irq_set_type,
+	.irq_retrigger		= irq_chip_retrigger_hierarchy,
+#ifdef CONFIG_SMP
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+#endif
+};
+
+static int meson_gpio_irq_domain_translate(struct irq_domain *domain,
+					   struct irq_fwspec *fwspec,
+					   unsigned long *hwirq,
+					   unsigned int *type)
+{
+	if (is_of_node(fwspec->fwnode)) {
+		if (fwspec->param_count != 2)
+			return -EINVAL;
+
+		*hwirq	= fwspec->param[0];
+		*type	= fwspec->param[1];
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int meson_gpio_irq_allocate_gic_irq(struct irq_domain *domain,
+					   unsigned int virq,
+					   irq_hw_number_t source,
+					   unsigned int type)
+{
+	struct irq_fwspec fwspec;
+
+	if (!irq_domain_get_of_node(domain->parent))
+		return -EINVAL;
+
+	fwspec.fwnode = domain->parent->fwnode;
+	fwspec.param_count = 3;
+	fwspec.param[0] = 0;	/* SPI */
+	fwspec.param[1] = source;
+	fwspec.param[2] = meson_gpio_irq_type_output(type);
+
+	return irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
+}
+
+static int meson_gpio_irq_domain_alloc(struct irq_domain *domain,
+				       unsigned int virq,
+				       unsigned int nr_irqs,
+				       void *data)
+{
+	struct irq_fwspec *fwspec = data;
+	struct meson_gpio_irq_domain *domain_data = domain->host_data;
+	struct meson_gpio_irq_chip_data *cd;
+	unsigned long hwirq, source;
+	unsigned int type;
+	int i, index, ret;
+
+	ret = meson_gpio_irq_domain_translate(domain, fwspec, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	pr_debug("irq %d, nr_irqs %d, hwirqs %lu\n", virq, nr_irqs, hwirq);
+
+	for (i = 0; i < nr_irqs; i++) {
+		index = mesion_gpio_irq_map_source(domain_data, hwirq + i,
+						   &source);
+		if (index < 0)
+			return index;
+
+		ret = meson_gpio_irq_type_setup(type, domain_data->base,
+						index);
+		if (ret)
+			return ret;
+
+		cd = kzalloc(sizeof(*cd), GFP_KERNEL);
+		if (!cd)
+			return -ENOMEM;
+
+		cd->base = domain_data->base;
+		cd->index = index;
+
+		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
+					      &meson_gpio_irq_chip, cd);
+
+		ret = meson_gpio_irq_allocate_gic_irq(domain, virq + i,
+						      source, type);
+		if (ret < 0)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void meson_gpio_irq_domain_free(struct irq_domain *domain,
+				       unsigned int virq,
+				       unsigned int nr_irqs)
+{
+	struct meson_gpio_irq_domain *domain_data = domain->host_data;
+	struct meson_gpio_irq_chip_data *cd;
+	struct irq_data *irq_data;
+	int i;
+
+	for (i = 0; i < nr_irqs; i++) {
+		irq_data = irq_domain_get_irq_data(domain, virq + i);
+		cd = irq_data_get_irq_chip_data(irq_data);
+
+		domain_data->map[cd->index] = IRQ_FREE;
+		kfree(cd);
+	}
+
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+
+}
+
+static const struct irq_domain_ops meson_gpio_irq_domain_ops = {
+	.alloc		= meson_gpio_irq_domain_alloc,
+	.free		= meson_gpio_irq_domain_free,
+	.translate	= meson_gpio_irq_domain_translate,
+};
+
+static int __init
+meson_gpio_irq_init_domain(struct device_node *node,
+			   struct meson_gpio_irq_domain *domain_data,
+			   const struct meson_gpio_irq_params *params)
+{
+	int i;
+	int nsource = params->nsource;
+	int *map;
+
+	map = kcalloc(nsource, sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return -ENOMEM;
+
+	for (i = 0; i < nsource; i++)
+		map[i] = IRQ_FREE;
+
+	domain_data->map = map;
+	domain_data->params = params;
+
+	return 0;
+}
+
+static int __init meson_gpio_irq_of_init(struct device_node *node,
+					 struct device_node *parent)
+{
+	struct irq_domain *domain, *parent_domain;
+	const struct of_device_id *match;
+	const struct meson_gpio_irq_params *params;
+	struct meson_gpio_irq_domain *domain_data;
+	int ret;
+
+	match = of_match_node(meson_irq_gpio_matches, node);
+	if (!match)
+		return -ENODEV;
+	params = match->data;
+
+	if (!parent) {
+		pr_err("missing parent interrupt node\n");
+		return -ENODEV;
+	}
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain) {
+		pr_err("unable to obtain parent domain\n");
+		return -ENXIO;
+	}
+
+	domain_data = kzalloc(sizeof(*domain_data), GFP_KERNEL);
+	if (!domain_data)
+		return -ENOMEM;
+
+	domain_data->base = of_iomap(node, 0);
+	if (!domain_data->base) {
+		ret = -ENOMEM;
+		goto out_free_dev;
+	}
+
+	ret = meson_gpio_irq_init_domain(node, domain_data, params);
+	if (ret < 0)
+		goto out_free_dev_content;
+
+	domain = irq_domain_add_hierarchy(parent_domain, 0, params->nhwirq,
+					  node, &meson_gpio_irq_domain_ops,
+					  domain_data);
+
+	if (!domain) {
+		pr_err("failed to allocated domain\n");
+		ret = -ENOMEM;
+		goto out_free_dev_content;
+	}
+
+	pr_info("%d to %d gpio interrupt mux initialized\n",
+		params->nhwirq, params->nsource);
+
+	return 0;
+
+out_free_dev_content:
+	kfree(domain_data->map);
+	iounmap(domain_data->base);
+
+out_free_dev:
+	kfree(domain_data);
+
+	return ret;
+}
+
+IRQCHIP_DECLARE(meson8_gpio_intc, "amlogic,meson8-gpio-intc",
+		meson_gpio_irq_of_init);
+IRQCHIP_DECLARE(meson8b_gpio_intc, "amlogic,meson8b-gpio-intc",
+		meson_gpio_irq_of_init);
+IRQCHIP_DECLARE(gxbb_gpio_intc, "amlogic,meson-gxbb-gpio-intc",
+		meson_gpio_irq_of_init);
-- 
2.7.4

[PATCH 2/9] dt-bindings: interrupt-controller: add DT binding for meson GPIO interrupt controller

[Jerome Brunet]

This commit adds the device tree bindings description for Amlogic's GPIO
interrupt controller available on the meson8, meson8b and gxbb SoC families

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---

Rob, I did not include the Ack you gave for the RFC as bindings have slightly
changed. Only the interrupt property has be removed following a discussion I
had with Marc.

 .../amlogic,meson-gpio-intc.txt                    | 31 ++++++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt b/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
new file mode 100644
index 000000000000..2464d9a0865d
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
@@ -0,0 +1,31 @@
+Amlogic meson GPIO interrupt controller
+
+Meson SoCs contains an interrupt controller which is able watch the SoC pads
+and generate an interrupt on edges or level. The controller is essentially a
+256 pads to 8 GIC interrupt multiplexer, with a filter block to select edge
+or level and polarity. We don’t expose all 256 mux inputs because the
+documentation shows that upper part is not mapped to any pad. The actual number
+of interrupt exposed depends on the SoC.
+
+Required properties:
+
+- compatible : should be either
+   "amlogic,meson8-gpio-intc” for meson8 SoCs (AML7826MX) or
+   “amlogic,meson8b-gpio-intc” for meson8b SoCs (S805) or
+   “amlogic,meson-gxbb-gpio-intc” for GXBB SoCs (S905)
+- interrupt-parent : a phandle to the GIC the interrupts are routed to.
+   Usually this is provided at the root level of the device tree as it is
+   common to most of the SoC
+- reg : Specifies base physical address and size of the registers.
+- interrupt-controller : Identifies the node as an interrupt controller.
+- #interrupt-cells : Specifies the number of cells needed to encode an
+   interrupt source. The value must be 2.
+
+Example:
+
+gpio_interrupt: interrupt-controller@9880 {
+	compatible = "amlogic,meson-gxbb-gpio-intc";
+	reg = <0x0 0x9880 0x0 0x10>;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+};
-- 
2.7.4

[PATCH 3/9] pinctrl: meson: update pinctrl data with gpio irq data

[Jerome Brunet]

This patch extends meson's pinctrl SoC specific data by adding the gpio
irq base number and the compatible controller for each SoC. This will
allow gpios to request an interrupt on the gpio interrupt controller
using this base irq number the pin offset inbank

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/pinctrl/meson/pinctrl-meson-gxbb.c | 24 +++++++++++----------
 drivers/pinctrl/meson/pinctrl-meson.h      | 16 +++++++++-----
 drivers/pinctrl/meson/pinctrl-meson8.c     | 22 ++++++++++---------
 drivers/pinctrl/meson/pinctrl-meson8b.c    | 34 +++++++++++++++++++++---------
 4 files changed, 60 insertions(+), 36 deletions(-)

diff --git a/drivers/pinctrl/meson/pinctrl-meson-gxbb.c b/drivers/pinctrl/meson/pinctrl-meson-gxbb.c
index c3928aa3fefa..ed8a1222de3b 100644
--- a/drivers/pinctrl/meson/pinctrl-meson-gxbb.c
+++ b/drivers/pinctrl/meson/pinctrl-meson-gxbb.c
@@ -715,24 +715,25 @@ static struct meson_pmx_func meson_gxbb_aobus_functions[] = {
 };
 
 static struct meson_bank meson_gxbb_periphs_banks[] = {
-	/*   name    first                      last                    pullen  pull    dir     out     in  */
-	BANK("X",    PIN(GPIOX_0, EE_OFF),	PIN(GPIOX_22, EE_OFF),  4,  0,  4,  0,  12, 0,  13, 0,  14, 0),
-	BANK("Y",    PIN(GPIOY_0, EE_OFF),	PIN(GPIOY_16, EE_OFF),  1,  0,  1,  0,  3,  0,  4,  0,  5,  0),
-	BANK("DV",   PIN(GPIODV_0, EE_OFF),	PIN(GPIODV_29, EE_OFF), 0,  0,  0,  0,  0,  0,  1,  0,  2,  0),
-	BANK("H",    PIN(GPIOH_0, EE_OFF),	PIN(GPIOH_3, EE_OFF),   1, 20,  1, 20,  3, 20,  4, 20,  5, 20),
-	BANK("Z",    PIN(GPIOZ_0, EE_OFF),	PIN(GPIOZ_15, EE_OFF),  3,  0,  3,  0,  9,  0,  10, 0, 11,  0),
-	BANK("CARD", PIN(CARD_0, EE_OFF),	PIN(CARD_6, EE_OFF),    2, 20,  2, 20,  6, 20,  7, 20,  8, 20),
-	BANK("BOOT", PIN(BOOT_0, EE_OFF),	PIN(BOOT_17, EE_OFF),   2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
-	BANK("CLK",  PIN(GPIOCLK_0, EE_OFF),	PIN(GPIOCLK_3, EE_OFF), 3, 28,  3, 28,  9, 28, 10, 28, 11, 28),
+	/*   name    first                      last                    irq       pullen  pull    dir     out     in  */
+	BANK("X",    PIN(GPIOX_0, EE_OFF),	PIN(GPIOX_22, EE_OFF),  106, 128, 4,  0,  4,  0,  12, 0,  13, 0,  14, 0),
+	BANK("Y",    PIN(GPIOY_0, EE_OFF),	PIN(GPIOY_16, EE_OFF),   89, 105, 1,  0,  1,  0,  3,  0,  4,  0,  5,  0),
+	BANK("DV",   PIN(GPIODV_0, EE_OFF),	PIN(GPIODV_29, EE_OFF),  59,  88, 0,  0,  0,  0,  0,  0,  1,  0,  2,  0),
+	BANK("H",    PIN(GPIOH_0, EE_OFF),	PIN(GPIOH_3, EE_OFF),    30,  33, 1, 20,  1, 20,  3, 20,  4, 20,  5, 20),
+	BANK("Z",    PIN(GPIOZ_0, EE_OFF),	PIN(GPIOZ_15, EE_OFF),   14,  29, 3,  0,  3,  0,  9,  0,  10, 0, 11,  0),
+	BANK("CARD", PIN(CARD_0, EE_OFF),	PIN(CARD_6, EE_OFF),     52,  58, 2, 20,  2, 20,  6, 20,  7, 20,  8, 20),
+	BANK("BOOT", PIN(BOOT_0, EE_OFF),	PIN(BOOT_17, EE_OFF),    34,  51, 2,  0,  2,  0,  6,  0,  7,  0,  8,  0),
+	BANK("CLK",  PIN(GPIOCLK_0, EE_OFF),	PIN(GPIOCLK_3, EE_OFF), 129, 132, 3, 28,  3, 28,  9, 28, 10, 28, 11, 28),
 };
 
 static struct meson_bank meson_gxbb_aobus_banks[] = {
-	/*   name    first              last               pullen  pull    dir     out     in  */
-	BANK("AO",   PIN(GPIOAO_0, 0),  PIN(GPIOAO_13, 0), 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
+	/*   name    first              last               irq    pullen  pull    dir     out     in  */
+	BANK("AO",   PIN(GPIOAO_0, 0),  PIN(GPIOAO_13, 0), 0, 13, 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
 };
 
 struct meson_pinctrl_data meson_gxbb_periphs_pinctrl_data = {
 	.name		= "periphs-banks",
+	.irq_compat	= "amlogic,meson-gxbb-gpio-intc",
 	.pin_base	= 14,
 	.pins		= meson_gxbb_periphs_pins,
 	.groups		= meson_gxbb_periphs_groups,
@@ -746,6 +747,7 @@ struct meson_pinctrl_data meson_gxbb_periphs_pinctrl_data = {
 
 struct meson_pinctrl_data meson_gxbb_aobus_pinctrl_data = {
 	.name		= "aobus-banks",
+	.irq_compat	= "amlogic,meson-gxbb-gpio-intc",
 	.pin_base	= 0,
 	.pins		= meson_gxbb_aobus_pins,
 	.groups		= meson_gxbb_aobus_groups,
diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
index 98b5080650c1..b90d69e366df 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.h
+++ b/drivers/pinctrl/meson/pinctrl-meson.h
@@ -81,6 +81,7 @@ enum meson_reg_type {
  * @name:	bank name
  * @first:	first pin of the bank
  * @last:	last pin of the bank
+ * @irq:	hwirq base number of the bank
  * @regs:	array of register descriptors
  *
  * A bank represents a set of pins controlled by a contiguous set of
@@ -92,11 +93,14 @@ struct meson_bank {
 	const char *name;
 	unsigned int first;
 	unsigned int last;
+	int irq_first;
+	int irq_last;
 	struct meson_reg_desc regs[NUM_REG];
 };
 
 struct meson_pinctrl_data {
 	const char *name;
+	const char *irq_compat;
 	const struct pinctrl_pin_desc *pins;
 	struct meson_pmx_group *groups;
 	struct meson_pmx_func *funcs;
@@ -147,12 +151,14 @@ struct meson_pinctrl {
 		.num_groups = ARRAY_SIZE(fn ## _groups),		\
 	}
 
-#define BANK(n, f, l, per, peb, pr, pb, dr, db, or, ob, ir, ib)		\
+#define BANK(n, f, l, fi, li, per, peb, pr, pb, dr, db, or, ob, ir, ib)	\
 	{								\
-		.name	= n,						\
-		.first	= f,						\
-		.last	= l,						\
-		.regs	= {						\
+		.name		= n,					\
+		.first		= f,					\
+		.last		= l,					\
+		.irq_first	= fi,					\
+		.irq_last	= li,					\
+		.regs = {						\
 			[REG_PULLEN]	= { per, peb },			\
 			[REG_PULL]	= { pr, pb },			\
 			[REG_DIR]	= { dr, db },			\
diff --git a/drivers/pinctrl/meson/pinctrl-meson8.c b/drivers/pinctrl/meson/pinctrl-meson8.c
index 07f1cb21c1b8..eddab1091408 100644
--- a/drivers/pinctrl/meson/pinctrl-meson8.c
+++ b/drivers/pinctrl/meson/pinctrl-meson8.c
@@ -916,23 +916,24 @@ static struct meson_pmx_func meson8_aobus_functions[] = {
 };
 
 static struct meson_bank meson8_cbus_banks[] = {
-	/*   name    first             last                 pullen  pull    dir     out     in  */
-	BANK("X",    PIN(GPIOX_0, 0),  PIN(GPIOX_21, 0),    4,  0,  4,  0,  0,  0,  1,  0,  2,  0),
-	BANK("Y",    PIN(GPIOY_0, 0),  PIN(GPIOY_16, 0),    3,  0,  3,  0,  3,  0,  4,  0,  5,  0),
-	BANK("DV",   PIN(GPIODV_0, 0), PIN(GPIODV_29, 0),   0,  0,  0,  0,  7,  0,  8,  0,  9,  0),
-	BANK("H",    PIN(GPIOH_0, 0),  PIN(GPIOH_9, 0),     1, 16,  1, 16,  9, 19, 10, 19, 11, 19),
-	BANK("Z",    PIN(GPIOZ_0, 0),  PIN(GPIOZ_14, 0),    1,  0,  1,  0,  3, 17,  4, 17,  5, 17),
-	BANK("CARD", PIN(CARD_0, 0),   PIN(CARD_6, 0),      2, 20,  2, 20,  0, 22,  1, 22,  2, 22),
-	BANK("BOOT", PIN(BOOT_0, 0),   PIN(BOOT_18, 0),     2,  0,  2,  0,  9,  0, 10,  0, 11,  0),
+	/*   name    first             last                 irq       pullen  pull    dir     out     in  */
+	BANK("X",    PIN(GPIOX_0, 0),  PIN(GPIOX_21, 0),    112, 133, 4,  0,  4,  0,  0,  0,  1,  0,  2,  0),
+	BANK("Y",    PIN(GPIOY_0, 0),  PIN(GPIOY_16, 0),    95,  111, 3,  0,  3,  0,  3,  0,  4,  0,  5,  0),
+	BANK("DV",   PIN(GPIODV_0, 0), PIN(GPIODV_29, 0),   65,   94, 0,  0,  0,  0,  7,  0,  8,  0,  9,  0),
+	BANK("H",    PIN(GPIOH_0, 0),  PIN(GPIOH_9, 0),     29,   38, 1, 16,  1, 16,  9, 19, 10, 19, 11, 19),
+	BANK("Z",    PIN(GPIOZ_0, 0),  PIN(GPIOZ_14, 0),    14,   28, 1,  0,  1,  0,  3, 17,  4, 17,  5, 17),
+	BANK("CARD", PIN(CARD_0, 0),   PIN(CARD_6, 0),      58,   64, 2, 20,  2, 20,  0, 22,  1, 22,  2, 22),
+	BANK("BOOT", PIN(BOOT_0, 0),   PIN(BOOT_18, 0),     39,   57, 2,  0,  2,  0,  9,  0, 10,  0, 11,  0),
 };
 
 static struct meson_bank meson8_aobus_banks[] = {
-	/*   name    first                  last                      pullen  pull    dir     out     in  */
-	BANK("AO",   PIN(GPIOAO_0, AO_OFF), PIN(GPIO_TEST_N, AO_OFF), 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
+	/*   name    first                  last                      irq    pullen  pull    dir     out     in  */
+	BANK("AO",   PIN(GPIOAO_0, AO_OFF), PIN(GPIO_TEST_N, AO_OFF), 0, 13, 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
 };
 
 struct meson_pinctrl_data meson8_cbus_pinctrl_data = {
 	.name		= "cbus-banks",
+	.irq_compat	= "amlogic,meson8-gpio-intc",
 	.pin_base	= 0,
 	.pins		= meson8_cbus_pins,
 	.groups		= meson8_cbus_groups,
@@ -946,6 +947,7 @@ struct meson_pinctrl_data meson8_cbus_pinctrl_data = {
 
 struct meson_pinctrl_data meson8_aobus_pinctrl_data = {
 	.name		= "ao-bank",
+	.irq_compat	= "amlogic,meson8-gpio-intc",
 	.pin_base	= 120,
 	.pins		= meson8_aobus_pins,
 	.groups		= meson8_aobus_groups,
diff --git a/drivers/pinctrl/meson/pinctrl-meson8b.c b/drivers/pinctrl/meson/pinctrl-meson8b.c
index 76f077f18193..d7505f492639 100644
--- a/drivers/pinctrl/meson/pinctrl-meson8b.c
+++ b/drivers/pinctrl/meson/pinctrl-meson8b.c
@@ -124,6 +124,12 @@ static const struct pinctrl_pin_desc meson8b_aobus_pins[] = {
 	MESON_PIN(GPIOAO_11, AO_OFF),
 	MESON_PIN(GPIOAO_12, AO_OFF),
 	MESON_PIN(GPIOAO_13, AO_OFF),
+
+	/*
+	 * The following 2 pins are not mentionned in the public datasheet
+	 * According to this datasheet, they can't be used with the gpio
+	 * interrupt controller
+	 */
 	MESON_PIN(GPIO_BSD_EN, AO_OFF),
 	MESON_PIN(GPIO_TEST_N, AO_OFF),
 };
@@ -881,23 +887,30 @@ static struct meson_pmx_func meson8b_aobus_functions[] = {
 };
 
 static struct meson_bank meson8b_cbus_banks[] = {
-	/*   name    first                      last                   pullen  pull    dir     out     in  */
-	BANK("X",    PIN(GPIOX_0, 0),		PIN(GPIOX_21, 0),      4,  0,  4,  0,  0,  0,  1,  0,  2,  0),
-	BANK("Y",    PIN(GPIOY_0, 0),		PIN(GPIOY_14, 0),      3,  0,  3,  0,  3,  0,  4,  0,  5,  0),
-	BANK("DV",   PIN(GPIODV_9, 0),		PIN(GPIODV_29, 0),     0,  0,  0,  0,  7,  0,  8,  0,  9,  0),
-	BANK("H",    PIN(GPIOH_0, 0),		PIN(GPIOH_9, 0),       1, 16,  1, 16,  9, 19, 10, 19, 11, 19),
-	BANK("CARD", PIN(CARD_0, 0),		PIN(CARD_6, 0),        2, 20,  2, 20,  0, 22,  1, 22,  2, 22),
-	BANK("BOOT", PIN(BOOT_0, 0),		PIN(BOOT_18, 0),       2,  0,  2,  0,  9,  0, 10,  0, 11,  0),
-	BANK("DIF",  PIN(DIF_0_P, 0),		PIN(DIF_4_N, 0),       5,  8,  5,  8, 12, 12, 13, 12, 14, 12),
+	/*   name    first                      last                irq      pullen  pull    dir     out     in  */
+	BANK("X",    PIN(GPIOX_0, 0),		PIN(GPIOX_21, 0),   97, 118, 4,  0,  4,  0,  0,  0,  1,  0,  2,  0),
+	BANK("Y",    PIN(GPIOY_0, 0),		PIN(GPIOY_14, 0),   80,  96, 3,  0,  3,  0,  3,  0,  4,  0,  5,  0),
+	BANK("DV",   PIN(GPIODV_9, 0),		PIN(GPIODV_29, 0),  59,  79, 0,  0,  0,  0,  7,  0,  8,  0,  9,  0),
+	BANK("H",    PIN(GPIOH_0, 0),		PIN(GPIOH_9, 0),    14,  23, 1, 16,  1, 16,  9, 19, 10, 19, 11, 19),
+	BANK("CARD", PIN(CARD_0, 0),		PIN(CARD_6, 0),     43,  49, 2, 20,  2, 20,  0, 22,  1, 22,  2, 22),
+	BANK("BOOT", PIN(BOOT_0, 0),		PIN(BOOT_18, 0),    24,  42, 2,  0,  2,  0,  9,  0, 10,  0, 11,  0),
+
+	/*
+	 * The following bank is not mentionned in the public datasheet
+	 * There is no information whether it can be used with the gpio
+	 * interrupt controller
+	 */
+	BANK("DIF",  PIN(DIF_0_P, 0),		PIN(DIF_4_N, 0),    -1,  -1, 5,  8,  5,  8, 12, 12, 13, 12, 14, 12),
 };
 
 static struct meson_bank meson8b_aobus_banks[] = {
-	/*   name    first                  last                      pullen  pull    dir     out     in  */
-	BANK("AO",   PIN(GPIOAO_0, AO_OFF), PIN(GPIO_TEST_N, AO_OFF), 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
+	/*   name    first                  last                      irq    pullen  pull    dir     out     in  */
+	BANK("AO",   PIN(GPIOAO_0, AO_OFF), PIN(GPIO_TEST_N, AO_OFF), 0, 13, 0,  0,  0, 16,  0,  0,  0, 16,  1,  0),
 };
 
 struct meson_pinctrl_data meson8b_cbus_pinctrl_data = {
 	.name		= "cbus-banks",
+	.irq_compat	= "amlogic,meson8b-gpio-intc",
 	.pin_base	= 0,
 	.pins		= meson8b_cbus_pins,
 	.groups		= meson8b_cbus_groups,
@@ -911,6 +924,7 @@ struct meson_pinctrl_data meson8b_cbus_pinctrl_data = {
 
 struct meson_pinctrl_data meson8b_aobus_pinctrl_data = {
 	.name		= "aobus-banks",
+	.irq_compat	= "amlogic,meson8b-gpio-intc",
 	.pin_base	= 130,
 	.pins		= meson8b_aobus_pins,
 	.groups		= meson8b_aobus_groups,
-- 
2.7.4

[PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Jerome Brunet]

Add the ability for gpio to request irq from the gpio interrupt controller
if present. We have to specificaly that the parent interrupt controller is
the gpio interrupt controller because gpio on meson SoCs can't generate
interrupt directly on the GIC.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/pinctrl/Kconfig               |  2 +
 drivers/pinctrl/meson/pinctrl-meson.c | 77 ++++++++++++++++++++++++++++++++++-
 drivers/pinctrl/meson/pinctrl-meson.h |  1 +
 3 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 0e75d94972ba..d5bfbfcddab0 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -126,7 +126,9 @@ config PINCTRL_MESON
 	select PINCONF
 	select GENERIC_PINCONF
 	select GPIOLIB
+	select IRQ_DOMAIN
 	select OF_GPIO
+	select OF_IRQ
 	select REGMAP_MMIO
 
 config PINCTRL_OXNAS
diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index 57122eda155a..fd3c1d44978b 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -50,6 +50,7 @@
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/of_irq.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -481,6 +482,58 @@ static void meson_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
 			   value ? BIT(bit) : 0);
 }
 
+static int meson_gpio_to_hwirq(struct meson_bank *bank, unsigned int offset)
+{
+	unsigned int hwirq;
+
+	if (bank->irq_first < 0)
+		/* this bank cannot generate irqs */
+		return -1;
+
+	hwirq = offset - bank->first + bank->irq_first;
+
+	if (hwirq > bank->irq_last)
+		/* this pin cannot generate irqs */
+		return -1;
+
+	return hwirq;
+}
+
+static int meson_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct meson_pinctrl *pc = gpiochip_get_data(chip);
+	struct meson_bank *bank;
+	struct irq_fwspec fwspec;
+	unsigned int hwirq;
+	int ret;
+
+	ret = meson_get_bank(pc, offset, &bank);
+	if (ret)
+		return ret;
+
+	/*
+	 * The interrupt controller might be missing, in such case we can't
+	 * provide an interrupt for a pin
+	 */
+	if (is_fwnode_irqchip(pc->fwnode)) {
+		dev_info(pc->dev, "interrupt controller not found\n");
+		return 0;
+	}
+
+	hwirq = meson_gpio_to_hwirq(bank, offset);
+	if (hwirq < 0) {
+		dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
+		return 0;
+	}
+
+	fwspec.fwnode = pc->fwnode;
+	fwspec.param_count = 2;
+	fwspec.param[0] = hwirq;
+	fwspec.param[1] = IRQ_TYPE_NONE;
+
+	return irq_create_fwspec_mapping(&fwspec);
+}
+
 static int meson_gpio_get(struct gpio_chip *chip, unsigned gpio)
 {
 	struct meson_pinctrl *pc = gpiochip_get_data(chip);
@@ -539,6 +592,7 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc)
 	pc->chip.direction_output = meson_gpio_direction_output;
 	pc->chip.get = meson_gpio_get;
 	pc->chip.set = meson_gpio_set;
+	pc->chip.to_irq = meson_gpio_to_irq;
 	pc->chip.base = pc->data->pin_base;
 	pc->chip.ngpio = pc->data->num_pins;
 	pc->chip.can_sleep = false;
@@ -598,6 +652,27 @@ static struct regmap *meson_map_resource(struct meson_pinctrl *pc,
 	return devm_regmap_init_mmio(pc->dev, base, &meson_regmap_config);
 }
 
+static int meson_pinctrl_get_irq_gpio_intc(struct meson_pinctrl *pc,
+					   struct device_node *node)
+{
+	struct device_node *np;
+
+	np = of_irq_find_parent(node);
+	if (unlikely(!np)) {
+		dev_err(pc->dev, "interrupt parent not found\n");
+		return -EINVAL;
+	}
+
+	if (!of_device_is_compatible(np, pc->data->irq_compat)) {
+		dev_info(pc->dev, "gpio interrupt disabled\n");
+		pc->fwnode = NULL;
+	}
+
+	pc->fwnode = of_node_to_fwnode(np);
+
+	return 0;
+}
+
 static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
 				  struct device_node *node)
 {
@@ -643,7 +718,7 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
 		return PTR_ERR(pc->reg_gpio);
 	}
 
-	return 0;
+	return meson_pinctrl_get_irq_gpio_intc(pc, gpio_np);
 }
 
 static int meson_pinctrl_probe(struct platform_device *pdev)
diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
index b90d69e366df..2e6c83adbd1f 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.h
+++ b/drivers/pinctrl/meson/pinctrl-meson.h
@@ -123,6 +123,7 @@ struct meson_pinctrl {
 	struct regmap *reg_gpio;
 	struct gpio_chip chip;
 	struct device_node *of_node;
+	struct fwnode_handle *fwnode;
 };
 
 #define PIN(x, b)	(b + x)
-- 
2.7.4

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Linus Walleij]

On Wed, Oct 19, 2016 at 12:08 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:

> Add the ability for gpio to request irq from the gpio interrupt controller
> if present. We have to specificaly that the parent interrupt controller is
> the gpio interrupt controller because gpio on meson SoCs can't generate
> interrupt directly on the GIC.
>
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
(...)
> +       select IRQ_DOMAIN
>         select OF_GPIO
> +       select OF_IRQ
(...)
> +static int meson_gpio_to_hwirq(struct meson_bank *bank, unsigned int offset)
> +{
> +       unsigned int hwirq;
> +
> +       if (bank->irq_first < 0)
> +               /* this bank cannot generate irqs */
> +               return -1;
> +
> +       hwirq = offset - bank->first + bank->irq_first;
> +
> +       if (hwirq > bank->irq_last)
> +               /* this pin cannot generate irqs */
> +               return -1;
> +
> +       return hwirq;
> +}

This is reimplementing irqdomain.

> +static int meson_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
(...)
> +       hwirq = meson_gpio_to_hwirq(bank, offset);
> +       if (hwirq < 0) {
> +               dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
> +               return 0;
> +       }

Isn't this usecase (also as described in the cover letter) a textbook
example of when you should be using hierarchical irqdomain?

Please check with Marc et al on hierarchical irqdomains.

Yours,
Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Jerome Brunet]

On Thu, 2016-10-20 at 21:21 +0200, Linus Walleij wrote:
> On Wed, Oct 19, 2016 at 12:08 PM, Jerome Brunet <jbrunet@baylibre.com
> > wrote:
> 
> > 
> > Add the ability for gpio to request irq from the gpio interrupt
> > controller
> > if present. We have to specificaly that the parent interrupt
> > controller is
> > the gpio interrupt controller because gpio on meson SoCs can't
> > generate
> > interrupt directly on the GIC.
> > 
> > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> (...)
> > 
> > +       select IRQ_DOMAIN
> >         select OF_GPIO
> > +       select OF_IRQ
> (...)
> > 
> > +static int meson_gpio_to_hwirq(struct meson_bank *bank, unsigned
> > int offset)
> > +{
> > +       unsigned int hwirq;
> > +
> > +       if (bank->irq_first < 0)
> > +               /* this bank cannot generate irqs */
> > +               return -1;
> > +
> > +       hwirq = offset - bank->first + bank->irq_first;
> > +
> > +       if (hwirq > bank->irq_last)
> > +               /* this pin cannot generate irqs */
> > +               return -1;
> > +
> > +       return hwirq;
> > +}
> 
> This is reimplementing irqdomain.
> 
> > 
> > +static int meson_gpio_to_irq(struct gpio_chip *chip, unsigned int
> > offset)
> > +{
> (...)
> > 
> > +       hwirq = meson_gpio_to_hwirq(bank, offset);
> > +       if (hwirq < 0) {
> > +               dev_dbg(pc->dev, "no interrupt for pin %u\n",
> > offset);
> > +               return 0;
> > +       }
> 
> Isn't this usecase (also as described in the cover letter) a textbook
> example of when you should be using hierarchical irqdomain?
> 
> Please check with Marc et al on hierarchical irqdomains.

Linus,
Do you mean I should create a new hierarchical irqdomains in each of
the two pinctrl instances we have in these SoC, these domains being
stacked on the one I just added for controller in irqchip ?

I did not understand this is what you meant when I asked you the
question at ELCE.

> 
> Yours,
> Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Linus Walleij]

On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:

>> Isn't this usecase (also as described in the cover letter) a textbook
>> example of when you should be using hierarchical irqdomain?
>>
>> Please check with Marc et al on hierarchical irqdomains.
>
> Linus,
> Do you mean I should create a new hierarchical irqdomains in each of
> the two pinctrl instances we have in these SoC, these domains being
> stacked on the one I just added for controller in irqchip ?
>
> I did not understand this is what you meant when I asked you the
> question at ELCE.

Honestly, I do not understand when and where to properly use
hierarchical irqdomain, even after Marc's talk at ELC-E.

Which is problematic since quite a few GPIO drivers now
need to use them.

I will review his slides, in the meantime I would say: whatever
Marc ACKs is fine with me. I trust this guy 100%. So I guess I
could ask that he ACK the entire chain of patches
from GIC->specialchip->GPIO.

Yours,
Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Marc Zyngier]

On 25/10/16 10:14, Linus Walleij wrote:
> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 
>>> Isn't this usecase (also as described in the cover letter) a textbook
>>> example of when you should be using hierarchical irqdomain?
>>>
>>> Please check with Marc et al on hierarchical irqdomains.
>>
>> Linus,
>> Do you mean I should create a new hierarchical irqdomains in each of
>> the two pinctrl instances we have in these SoC, these domains being
>> stacked on the one I just added for controller in irqchip ?
>>
>> I did not understand this is what you meant when I asked you the
>> question at ELCE.
> 
> Honestly, I do not understand when and where to properly use
> hierarchical irqdomain, even after Marc's talk at ELC-E.

I probably didn't do that good a job explaining it then. Let's try
again. You want to use hierarchical domains when you want to describe an
interrupt whose path traverses multiple controllers without ever being
multiplexed with other signals. As long as you have this 1:1
relationship between controllers, you can use them.

> Which is problematic since quite a few GPIO drivers now
> need to use them.
> 
> I will review his slides, in the meantime I would say: whatever
> Marc ACKs is fine with me. I trust this guy 100%. So I guess I
> could ask that he ACK the entire chain of patches
> from GIC->specialchip->GPIO.

Man, I don't even trust myself... ;-)

Thanks,

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

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Jerome Brunet]

On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote:
> > 
> On 25/10/16 10:14, Linus Walleij wrote:
> > 
> > On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.c
> > om> wrote:
> > 
> > > 
> > > > 
> > > > Isn't this usecase (also as described in the cover letter) a
> > > > textbook
> > > > example of when you should be using hierarchical irqdomain?
> > > > 
> > > > Please check with Marc et al on hierarchical irqdomains.
> > > 
> > > Linus,
> > > Do you mean I should create a new hierarchical irqdomains in each
> > > of
> > > the two pinctrl instances we have in these SoC, these domains
> > > being
> > > stacked on the one I just added for controller in irqchip ?
> > > 
> > > I did not understand this is what you meant when I asked you the
> > > question at ELCE.
> > 
> > Honestly, I do not understand when and where to properly use
> > hierarchical irqdomain, even after Marc's talk at ELC-E.
> 
> I probably didn't do that good a job explaining it then. Let's try
> again. You want to use hierarchical domains when you want to describe
> an
> interrupt whose path traverses multiple controllers without ever
> being
> multiplexed with other signals. As long as you have this 1:1
> relationship between controllers, you can use them.
> 

Linus, Marc,

The calculation is question here is meant to get the appropriate hwirq
number from a particular gpio (and deal with the gpios that can't
provide an irq at all). 

If I look at other gpio drivers, many are doing exactly this kind of
calculation before calling 'irq_create_mapping' in the to_irq callback.
For example:
- pinctrl/nomadik/pinctrl-abx500.c
- pinctrl/samsung/pinctrl-exynos5440.c

Some can afford to create all the mappings in the probe and just call
'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work here. We
have only 8 upstream irqs for 130+ pins, so only 8 mappings possible at
a time. 

My understanding is that irqdomain provide a way to map hwirq to linux
virq (and back), not map gpio number to hwirq, right?

Even if I implement an another irqdomain at the gpio level, I would
still have to perform this kind of calculation, one way or the other.

> > Which is problematic since quite a few GPIO drivers now
> > need to use them.
> > 
> > I will review his slides, in the meantime I would say: whatever
> > Marc ACKs is fine with me. I trust this guy 100%. So I guess I
> > could ask that he ACK the entire chain of patches
> > from GIC->specialchip->GPIO.

Actually this discussion go me thinking about another issue we have
with this hardware.
We are looking for a way to implement support for IRQ_TYPE_EDGE_BOTH
(needed for things like gpio-keys or mmc card detect). 
The controller can do each edge but not both at the same time.
I'm thinking that implementing another irqdomain at the gpio level
would allow to properly check the pad level in the EOI callback then
set the next expected edge type accordingly (using
'irq_chip_set_type_parent')

Would it be acceptable ?

It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a similar
way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)

> Man, I don't even trust myself... ;-)
> 
> Thanks,
> 
> 	M.

Thx
Regards

Jerome

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Marc Zyngier]

On 25/10/16 14:08, Jerome Brunet wrote:
> On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote:
>>>
>> On 25/10/16 10:14, Linus Walleij wrote:
>>>
>>> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.c
>>> om> wrote:
>>>
>>>>
>>>>>
>>>>> Isn't this usecase (also as described in the cover letter) a
>>>>> textbook
>>>>> example of when you should be using hierarchical irqdomain?
>>>>>
>>>>> Please check with Marc et al on hierarchical irqdomains.
>>>>
>>>> Linus,
>>>> Do you mean I should create a new hierarchical irqdomains in each
>>>> of
>>>> the two pinctrl instances we have in these SoC, these domains
>>>> being
>>>> stacked on the one I just added for controller in irqchip ?
>>>>
>>>> I did not understand this is what you meant when I asked you the
>>>> question at ELCE.
>>>
>>> Honestly, I do not understand when and where to properly use
>>> hierarchical irqdomain, even after Marc's talk at ELC-E.
>>
>> I probably didn't do that good a job explaining it then. Let's try
>> again. You want to use hierarchical domains when you want to describe
>> an
>> interrupt whose path traverses multiple controllers without ever
>> being
>> multiplexed with other signals. As long as you have this 1:1
>> relationship between controllers, you can use them.
>>
> 
> Linus, Marc,
> 
> The calculation is question here is meant to get the appropriate hwirq
> number from a particular gpio (and deal with the gpios that can't
> provide an irq at all). 
> 
> If I look at other gpio drivers, many are doing exactly this kind of
> calculation before calling 'irq_create_mapping' in the to_irq callback.
> For example:
> - pinctrl/nomadik/pinctrl-abx500.c
> - pinctrl/samsung/pinctrl-exynos5440.c
> 
> Some can afford to create all the mappings in the probe and just call
> 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work here. We
> have only 8 upstream irqs for 130+ pins, so only 8 mappings possible at
> a time. 
> 
> My understanding is that irqdomain provide a way to map hwirq to linux
> virq (and back), not map gpio number to hwirq, right?

But why are those number different? Why don't you use the same
namespace? If gpio == hwirq, all your problems are already solved. If
you don't find the mapping in the irqdomain, then there is no irq, end
of story. What am I missing?

> 
> Even if I implement an another irqdomain at the gpio level, I would
> still have to perform this kind of calculation, one way or the other.
> 
>>> Which is problematic since quite a few GPIO drivers now
>>> need to use them.
>>>
>>> I will review his slides, in the meantime I would say: whatever
>>> Marc ACKs is fine with me. I trust this guy 100%. So I guess I
>>> could ask that he ACK the entire chain of patches
>>> from GIC->specialchip->GPIO.
> 
> Actually this discussion go me thinking about another issue we have
> with this hardware.
> We are looking for a way to implement support for IRQ_TYPE_EDGE_BOTH
> (needed for things like gpio-keys or mmc card detect). 
> The controller can do each edge but not both at the same time.
> I'm thinking that implementing another irqdomain at the gpio level
> would allow to properly check the pad level in the EOI callback then
> set the next expected edge type accordingly (using
> 'irq_chip_set_type_parent')
> 
> Would it be acceptable ?

I really don't see what another irqdomain brings to the table. This is
not a separate piece of HW, so the hwirq:irq mapping is still the same.
I fail to see what the benefit is.

> It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a similar
> way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)

Being already done doesn't make it reliable. If the line goes low
between latching the rising edge and reprogramming the trigger, you've
lost at least *two* interrupts (the falling edge and the following
rising edge).

Thanks,

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

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Jerome Brunet]

On Tue, 2016-10-25 at 14:38 +0100, Marc Zyngier wrote:
> On 25/10/16 14:08, Jerome Brunet wrote:
> > 
> > On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote:
> > > 
> > > > 
> > > > 
> > > On 25/10/16 10:14, Linus Walleij wrote:
> > > > 
> > > > 
> > > > On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylib
> > > > re.c
> > > > om> wrote:
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > Isn't this usecase (also as described in the cover letter)
> > > > > > a
> > > > > > textbook
> > > > > > example of when you should be using hierarchical irqdomain?
> > > > > > 
> > > > > > Please check with Marc et al on hierarchical irqdomains.
> > > > > 
> > > > > Linus,
> > > > > Do you mean I should create a new hierarchical irqdomains in
> > > > > each
> > > > > of
> > > > > the two pinctrl instances we have in these SoC, these domains
> > > > > being
> > > > > stacked on the one I just added for controller in irqchip ?
> > > > > 
> > > > > I did not understand this is what you meant when I asked you
> > > > > the
> > > > > question at ELCE.
> > > > 
> > > > Honestly, I do not understand when and where to properly use
> > > > hierarchical irqdomain, even after Marc's talk at ELC-E.
> > > 
> > > I probably didn't do that good a job explaining it then. Let's
> > > try
> > > again. You want to use hierarchical domains when you want to
> > > describe
> > > an
> > > interrupt whose path traverses multiple controllers without ever
> > > being
> > > multiplexed with other signals. As long as you have this 1:1
> > > relationship between controllers, you can use them.
> > > 
> > 
> > Linus, Marc,
> > 
> > The calculation is question here is meant to get the appropriate
> > hwirq
> > number from a particular gpio (and deal with the gpios that can't
> > provide an irq at all). 
> > 
> > If I look at other gpio drivers, many are doing exactly this kind
> > of
> > calculation before calling 'irq_create_mapping' in the to_irq
> > callback.
> > For example:
> > - pinctrl/nomadik/pinctrl-abx500.c
> > - pinctrl/samsung/pinctrl-exynos5440.c
> > 
> > Some can afford to create all the mappings in the probe and just
> > call
> > 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work
> > here. We
> > have only 8 upstream irqs for 130+ pins, so only 8 mappings
> > possible at
> > a time. 
> > 
> > My understanding is that irqdomain provide a way to map hwirq to
> > linux
> > virq (and back), not map gpio number to hwirq, right?
> 
> But why are those number different? Why don't you use the same
> namespace? If gpio == hwirq, all your problems are already solved. If
> you don't find the mapping in the irqdomain, then there is no irq,
> end
> of story. What am I missing?
> 

There is a few problems to guarantee that gpio == hwirq.
1. We have 2 instances of pinctrl, to guarantee that the linux gpio
number == hwirq, we would have to guarantee the order in which they are
probed. At least this my understanding 
2. Inside each instance, we may have banks that can't have irq. We even
have a bank on meson8b which has a mixed state (the last pins don't
have irqs, while the first ones do). those banks/pins are still valid
gpios with gpio numbers. This introduce a shift between the gpio
numbering and the hwirq.

The point of this calculation is take the offset given to the 'to_irq'
callback, remove the gpio bank base number and add irq base number.
There is a few trick added to handled the case in 2.

In addition, to call 'irq_find_mapping', we would first have to create
the mapping, in the probe I suppose. This would call the allocate
callback of the domain, in which we can allocate only 8 interrupts.

That's why I create the mapping in the .to_irq callback.

> > 
> > 
> > Even if I implement an another irqdomain at the gpio level, I would
> > still have to perform this kind of calculation, one way or the
> > other.
> > 
> > > 
> > > > 
> > > > Which is problematic since quite a few GPIO drivers now
> > > > need to use them.
> > > > 
> > > > I will review his slides, in the meantime I would say: whatever
> > > > Marc ACKs is fine with me. I trust this guy 100%. So I guess I
> > > > could ask that he ACK the entire chain of patches
> > > > from GIC->specialchip->GPIO.
> > 
> > Actually this discussion go me thinking about another issue we have
> > with this hardware.
> > We are looking for a way to implement support for
> > IRQ_TYPE_EDGE_BOTH
> > (needed for things like gpio-keys or mmc card detect). 
> > The controller can do each edge but not both at the same time.
> > I'm thinking that implementing another irqdomain at the gpio level
> > would allow to properly check the pad level in the EOI callback
> > then
> > set the next expected edge type accordingly (using
> > 'irq_chip_set_type_parent')
> > 
> > Would it be acceptable ?
> 
> I really don't see what another irqdomain brings to the table. This
> is
> not a separate piece of HW, so the hwirq:irq mapping is still the
> same.
> I fail to see what the benefit is.

The separate piece of hw is the gpio itself. 
The irq-controller (in irqchip) can't read the gpio state because it is
simply another device.

The domain I'm thinking about wouldn't do much, I reckon. 
It would just register an irqchip which would only implement eoi.
For everything else, it would rely the parent controller.
>From your explanation, I understood this is the purpose of hierarchy
domains, For each hw in the chain to handle only what it can, and rely
on its parent for the rest.

> 
> > 
> > It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a
> > similar
> > way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)
> 
> Being already done doesn't make it reliable. If the line goes low
> between latching the rising edge and reprogramming the trigger,
> you've
> lost at least *two* interrupts (the falling edge and the following
> rising edge).

If a 'usual' controller support IRQ_TYPE_EDGE_BOTH and the line goes
low between latching rising edge and handling the interrupt, wouldn't
you miss the falling edge anyway ? The signal is just going too fast
the HW to handle everything.

For the second rising edge, I disagree, it would not be lost, since we
would read the pad state, get a low level, and reprogram the controller
for another rising edge.

> 
> Thanks,
> 
> 	M.

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Marc Zyngier]

On 25/10/16 15:22, Jerome Brunet wrote:
> On Tue, 2016-10-25 at 14:38 +0100, Marc Zyngier wrote:
>> On 25/10/16 14:08, Jerome Brunet wrote:
>>>
>>> On Tue, 2016-10-25 at 11:38 +0100, Marc Zyngier wrote:
>>>>
>>>>>
>>>>>
>>>> On 25/10/16 10:14, Linus Walleij wrote:
>>>>>
>>>>>
>>>>> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylib
>>>>> re.c
>>>>> om> wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Isn't this usecase (also as described in the cover letter)
>>>>>>> a
>>>>>>> textbook
>>>>>>> example of when you should be using hierarchical irqdomain?
>>>>>>>
>>>>>>> Please check with Marc et al on hierarchical irqdomains.
>>>>>>
>>>>>> Linus,
>>>>>> Do you mean I should create a new hierarchical irqdomains in
>>>>>> each
>>>>>> of
>>>>>> the two pinctrl instances we have in these SoC, these domains
>>>>>> being
>>>>>> stacked on the one I just added for controller in irqchip ?
>>>>>>
>>>>>> I did not understand this is what you meant when I asked you
>>>>>> the
>>>>>> question at ELCE.
>>>>>
>>>>> Honestly, I do not understand when and where to properly use
>>>>> hierarchical irqdomain, even after Marc's talk at ELC-E.
>>>>
>>>> I probably didn't do that good a job explaining it then. Let's
>>>> try
>>>> again. You want to use hierarchical domains when you want to
>>>> describe
>>>> an
>>>> interrupt whose path traverses multiple controllers without ever
>>>> being
>>>> multiplexed with other signals. As long as you have this 1:1
>>>> relationship between controllers, you can use them.
>>>>
>>>
>>> Linus, Marc,
>>>
>>> The calculation is question here is meant to get the appropriate
>>> hwirq
>>> number from a particular gpio (and deal with the gpios that can't
>>> provide an irq at all). 
>>>
>>> If I look at other gpio drivers, many are doing exactly this kind
>>> of
>>> calculation before calling 'irq_create_mapping' in the to_irq
>>> callback.
>>> For example:
>>> - pinctrl/nomadik/pinctrl-abx500.c
>>> - pinctrl/samsung/pinctrl-exynos5440.c
>>>
>>> Some can afford to create all the mappings in the probe and just
>>> call
>>> 'irq_find_mapping' (gpio/gpio_tegra.c) but this would not work
>>> here. We
>>> have only 8 upstream irqs for 130+ pins, so only 8 mappings
>>> possible at
>>> a time. 
>>>
>>> My understanding is that irqdomain provide a way to map hwirq to
>>> linux
>>> virq (and back), not map gpio number to hwirq, right?
>>
>> But why are those number different? Why don't you use the same
>> namespace? If gpio == hwirq, all your problems are already solved. If
>> you don't find the mapping in the irqdomain, then there is no irq,
>> end
>> of story. What am I missing?
>>
> 
> There is a few problems to guarantee that gpio == hwirq.
> 1. We have 2 instances of pinctrl, to guarantee that the linux gpio
> number == hwirq, we would have to guarantee the order in which they are
> probed. At least this my understanding 

Maybe I wasn't clear enough, and my use of gpio is probably wrong. So
Linux has a gpio number, which is obviously an abstract number (just
like the Linux irq number). But the pad number, in the context of given
SoC, is constant. So we have:

	pad->gpio
	hwirq->irq

Why can't you have pad == hwirq, always? This is already what you have
in the irqchip driver. This would simplify a lot of things.

> 2. Inside each instance, we may have banks that can't have irq. We even
> have a bank on meson8b which has a mixed state (the last pins don't
> have irqs, while the first ones do). those banks/pins are still valid
> gpios with gpio numbers. This introduce a shift between the gpio
> numbering and the hwirq.
> 
> The point of this calculation is take the offset given to the 'to_irq'
> callback, remove the gpio bank base number and add irq base number.
> There is a few trick added to handled the case in 2.

You seem to assume linear mappings, which is pretty dangerous.

> 
> In addition, to call 'irq_find_mapping', we would first have to create
> the mapping, in the probe I suppose. This would call the allocate
> callback of the domain, in which we can allocate only 8 interrupts.
> 
> That's why I create the mapping in the .to_irq callback.

Is gpio_to_irq() supposed to allocate an interrupt? Or merely to report
the existence of a mapping?

> 
>>>
>>>
>>> Even if I implement an another irqdomain at the gpio level, I would
>>> still have to perform this kind of calculation, one way or the
>>> other.
>>>
>>>>
>>>>>
>>>>> Which is problematic since quite a few GPIO drivers now
>>>>> need to use them.
>>>>>
>>>>> I will review his slides, in the meantime I would say: whatever
>>>>> Marc ACKs is fine with me. I trust this guy 100%. So I guess I
>>>>> could ask that he ACK the entire chain of patches
>>>>> from GIC->specialchip->GPIO.
>>>
>>> Actually this discussion go me thinking about another issue we have
>>> with this hardware.
>>> We are looking for a way to implement support for
>>> IRQ_TYPE_EDGE_BOTH
>>> (needed for things like gpio-keys or mmc card detect). 
>>> The controller can do each edge but not both at the same time.
>>> I'm thinking that implementing another irqdomain at the gpio level
>>> would allow to properly check the pad level in the EOI callback
>>> then
>>> set the next expected edge type accordingly (using
>>> 'irq_chip_set_type_parent')
>>>
>>> Would it be acceptable ?
>>
>> I really don't see what another irqdomain brings to the table. This
>> is
>> not a separate piece of HW, so the hwirq:irq mapping is still the
>> same.
>> I fail to see what the benefit is.
> 
> The separate piece of hw is the gpio itself. 
> The irq-controller (in irqchip) can't read the gpio state because it is
> simply another device.
> 
> The domain I'm thinking about wouldn't do much, I reckon. 
> It would just register an irqchip which would only implement eoi.
> For everything else, it would rely the parent controller.
> From your explanation, I understood this is the purpose of hierarchy
> domains, For each hw in the chain to handle only what it can, and rely
> on its parent for the rest.

Right. But that doesn't make it more reliable, see below.

> 
>>
>>>
>>> It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a
>>> similar
>>> way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)
>>
>> Being already done doesn't make it reliable. If the line goes low
>> between latching the rising edge and reprogramming the trigger,
>> you've
>> lost at least *two* interrupts (the falling edge and the following
>> rising edge).
> 
> If a 'usual' controller support IRQ_TYPE_EDGE_BOTH and the line goes
> low between latching rising edge and handling the interrupt, wouldn't
> you miss the falling edge anyway ? The signal is just going too fast
> the HW to handle everything.

That's the role of the HW to ensure that you don't loose any interrupt,
up to the sampling frequency of the controller. Doing it in SW is always
going to be a wonderful case of "it mostly work".

> For the second rising edge, I disagree, it would not be lost, since we
> would read the pad state, get a low level, and reprogram the controller
> for another rising edge.

You always have a race between checking your level and switching the
configuration, which is likely to be slow anyway. In the meantime, the
level has changed and you're dead.

Anyway, I suggest you focus on getting something simple up and running,
and postpone the funky (read broken) stuff for later, once you have
something that looks vaguely sane (because we're not quite there yet).

Thanks,

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

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Jerome Brunet]

On Tue, 2016-10-25 at 15:47 +0100, Marc Zyngier wrote:
> 
> > > But why are those number different? Why don't you use the same
> > > namespace? If gpio == hwirq, all your problems are already
> > > solved. If
> > > you don't find the mapping in the irqdomain, then there is no
> > > irq,
> > > end
> > > of story. What am I missing?
> > > 
> > 
> > There is a few problems to guarantee that gpio == hwirq.
> > 1. We have 2 instances of pinctrl, to guarantee that the linux gpio
> > number == hwirq, we would have to guarantee the order in which they
> > are
> > probed. At least this my understanding 
> 
> Maybe I wasn't clear enough, and my use of gpio is probably wrong. So
> Linux has a gpio number, which is obviously an abstract number (just
> like the Linux irq number). But the pad number, in the context of
> given
> SoC, is constant. So we have:
> 
> 	pad->gpio
> 	hwirq->irq
> 
> Why can't you have pad == hwirq, always? This is already what you
> have
> in the irqchip driver. This would simplify a lot of things.

The meson pinctrl driver is designed so that, the pad numbering (or the
equivalent), is linear within a bank. This makes the code simpler for a
lot of things in the meson-pinctrl driver (like finding the appropriate
register and bit). Because of this, and the fact that some gpio might
not have an irq, pad == hwirq cannot hold.

In addition the pad numbering starts from 0 on each gpiochip.

The solution for that is to have the first and last irq number (which
is actually the mux setting of the controller) in the data of each gpio
bank, as described in the Datasheet.

> 
> > 
> > 2. Inside each instance, we may have banks that can't have irq. We
> > even
> > have a bank on meson8b which has a mixed state (the last pins don't
> > have irqs, while the first ones do). those banks/pins are still
> > valid
> > gpios with gpio numbers. This introduce a shift between the gpio
> > numbering and the hwirq.
> > 
> > The point of this calculation is take the offset given to the
> > 'to_irq'
> > callback, remove the gpio bank base number and add irq base number.
> > There is a few trick added to handled the case in 2.
> 
> You seem to assume linear mappings, which is pretty dangerous.

That's the way the meson pinctrl driver works (linear numbering in each
banks) and the HW is described in DS. Why would it be dangerous ?

> 
> > 
> > 
> > In addition, to call 'irq_find_mapping', we would first have to
> > create
> > the mapping, in the probe I suppose. This would call the allocate
> > callback of the domain, in which we can allocate only 8 interrupts.
> > 
> > That's why I create the mapping in the .to_irq callback.
> 
> Is gpio_to_irq() supposed to allocate an interrupt? Or merely to
> report
> the existence of a mapping?

Linus, please correct me if I'm wrong,
.to_irq gets the linux gpio number and returns the linux virtual irq
numbers, 0 if there is no interrupt.

So could either find or create the mapping.
   A. With the hardware at hand, and the limited upstream interrupt
      number, i don't see how we could create the mapping beforehand. 

> 
> > 
> > 
> > > 
> > > > 
> > > > 
> > > > 
> > > > Even if I implement an another irqdomain at the gpio level, I
> > > > would
> > > > still have to perform this kind of calculation, one way or the
> > > > other.
> > > > 
> > > > > 
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > Which is problematic since quite a few GPIO drivers now
> > > > > > need to use them.
> > > > > > 
> > > > > > I will review his slides, in the meantime I would say:
> > > > > > whatever
> > > > > > Marc ACKs is fine with me. I trust this guy 100%. So I
> > > > > > guess I
> > > > > > could ask that he ACK the entire chain of patches
> > > > > > from GIC->specialchip->GPIO.
> > > > 
> > > > Actually this discussion go me thinking about another issue we
> > > > have
> > > > with this hardware.
> > > > We are looking for a way to implement support for
> > > > IRQ_TYPE_EDGE_BOTH
> > > > (needed for things like gpio-keys or mmc card detect). 
> > > > The controller can do each edge but not both at the same time.
> > > > I'm thinking that implementing another irqdomain at the gpio
> > > > level
> > > > would allow to properly check the pad level in the EOI callback
> > > > then
> > > > set the next expected edge type accordingly (using
> > > > 'irq_chip_set_type_parent')
> > > > 
> > > > Would it be acceptable ?
> > > 
> > > I really don't see what another irqdomain brings to the table.
> > > This
> > > is
> > > not a separate piece of HW, so the hwirq:irq mapping is still the
> > > same.
> > > I fail to see what the benefit is.
> > 
> > The separate piece of hw is the gpio itself. 
> > The irq-controller (in irqchip) can't read the gpio state because
> > it is
> > simply another device.
> > 
> > The domain I'm thinking about wouldn't do much, I reckon. 
> > It would just register an irqchip which would only implement eoi.
> > For everything else, it would rely the parent controller.
> > From your explanation, I understood this is the purpose of
> > hierarchy
> > domains, For each hw in the chain to handle only what it can, and
> > rely
> > on its parent for the rest.
> 
> Right. But that doesn't make it more reliable, see below.
> 
> > 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > It looks a few other drivers deal with IRQ_TYPE_EDGE_BOTH in a
> > > > similar
> > > > way (gpio/gpio-omap.c, gpio/gpio-dwapb.c)
> > > 
> > > Being already done doesn't make it reliable. If the line goes low
> > > between latching the rising edge and reprogramming the trigger,
> > > you've
> > > lost at least *two* interrupts (the falling edge and the
> > > following
> > > rising edge).
> > 
> > If a 'usual' controller support IRQ_TYPE_EDGE_BOTH and the line
> > goes
> > low between latching rising edge and handling the interrupt,
> > wouldn't
> > you miss the falling edge anyway ? The signal is just going too
> > fast
> > the HW to handle everything.
> 
> That's the role of the HW to ensure that you don't loose any
> interrupt,
> up to the sampling frequency of the controller. Doing it in SW is
> always
> going to be a wonderful case of "it mostly work".
> 
> > 
> > For the second rising edge, I disagree, it would not be lost, since
> > we
> > would read the pad state, get a low level, and reprogram the
> > controller
> > for another rising edge.
> 
> You always have a race between checking your level and switching the
> configuration, which is likely to be slow anyway. In the meantime,
> the
> level has changed and you're dead.
> 
> Anyway, I suggest you focus on getting something simple up and
> running,
> and postpone the funky (read broken) stuff for later, once you have
> something that looks vaguely sane (because we're not quite there
> yet).
> 
> Thanks,
> 
> 	M.

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Linus Walleij]

On Tue, Oct 25, 2016 at 5:31 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Tue, 2016-10-25 at 15:47 +0100, Marc Zyngier wrote:

>> Is gpio_to_irq() supposed to allocate an interrupt? Or merely to
>> report the existence of a mapping?

It should provide an IRQ corresponding to the gpio line, if possible.

However the semantic is such, that it is not necessary to call to_irq()
before using an IRQ: the irqchip and gpiochip abstractions should be
orthogonal.

This goes especially when using device tree or ACPI, where you
may reference an IRQ from something modeled as irqchip, which
is simultaneously a gpiochip.

> Linus, please correct me if I'm wrong,
> .to_irq gets the linux gpio number and returns the linux virtual irq
> numbers, 0 if there is no interrupt.

Yes. But it may *or may not* be called before using the IRQ.

So it should look up or try to create a mapping on request, but not
assume to have been called before using some line as IRQ.

The only thing you should assume to be called before an interrupt
is put to use is the stuff in irqchip. So you have to do your
dynamic irqdomain mapping elsewhere than .to_irq().

Yours,
Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Jerome Brunet]

On Tue, 2016-10-25 at 20:20 +0200, Linus Walleij wrote:
> On Tue, Oct 25, 2016 at 5:31 PM, Jerome Brunet <jbrunet@baylibre.com>
> wrote:
> > 
> > On Tue, 2016-10-25 at 15:47 +0100, Marc Zyngier wrote:
> 
> > 
> > > 
> > > Is gpio_to_irq() supposed to allocate an interrupt? Or merely to
> > > report the existence of a mapping?
> 
> It should provide an IRQ corresponding to the gpio line, if possible.
> 
> However the semantic is such, that it is not necessary to call
> to_irq()
> before using an IRQ: the irqchip and gpiochip abstractions should be
> orthogonal.

Linus,

They are orthogonal. You can request an irq from the irqchip controller
without the gpiochip, like any other irq controller.

> 
> This goes especially when using device tree or ACPI, where you
> may reference an IRQ from something modeled as irqchip, which
> is simultaneously a gpiochip.
> 
> > 
> > Linus, please correct me if I'm wrong,
> > .to_irq gets the linux gpio number and returns the linux virtual
> > irq
> > numbers, 0 if there is no interrupt.
> 
> Yes. But it may *or may not* be called before using the IRQ.
> 
> So it should look up or try to create a mapping on request, but not
> assume to have been called before using some line as IRQ.
> 
> The only thing you should assume to be called before an interrupt
> is put to use is the stuff in irqchip. So you have to do your
> dynamic irqdomain mapping elsewhere than .to_irq().

irq_create_mapping (and irq_create_fwspec_mapping) internally calls
irq_find_mapping. So if the mapping already exist (the irq is already
used before calling to_irq), the existing mapping will be returned. The
mapping will be actually created only if needed. It seems to be in line
with your explanation, no ?

There is really a *lot* of gpio drivers which use irq_create_mapping in
the to_irq callback, are these all wrong ?
If this should not be used, what should we all do instead ? 

> 
> Yours,
> Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Linus Walleij]

On Wed, Oct 26, 2016 at 4:22 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> On Tue, 2016-10-25 at 20:20 +0200, Linus Walleij wrote:

>> However the semantic is such, that it is not necessary to call
>> to_irq()
>> before using an IRQ: the irqchip and gpiochip abstractions should be
>> orthogonal.
>
> Linus,
>
> They are orthogonal. You can request an irq from the irqchip controller
> without the gpiochip, like any other irq controller.

OK good, sorry if I'm stating the obvious.

> irq_create_mapping (and irq_create_fwspec_mapping) internally calls
> irq_find_mapping. So if the mapping already exist (the irq is already
> used before calling to_irq), the existing mapping will be returned. The
> mapping will be actually created only if needed. It seems to be in line
> with your explanation, no ?

Yes, but you want to call irq_create_mapping() in slowpath (irq setup)
and irq_find_mapping() in fastpath (irq handler). Else the first IRQ
may result in unwelcomed surprises.

> There is really a *lot* of gpio drivers which use irq_create_mapping in
> the to_irq callback, are these all wrong ?

Yes they are all wrong. They should all be using irq_find_mapping().

> If this should not be used, what should we all do instead ?

Call irq_create_mapping() in some other place.

Yours,
Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Kevin Hilman]

Hi Linus,

Linus Walleij <linus.walleij@linaro.org> writes:

> On Wed, Oct 26, 2016 at 4:22 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
>> On Tue, 2016-10-25 at 20:20 +0200, Linus Walleij wrote:
>
>>> However the semantic is such, that it is not necessary to call
>>> to_irq()
>>> before using an IRQ: the irqchip and gpiochip abstractions should be
>>> orthogonal.
>>
>> Linus,
>>
>> They are orthogonal. You can request an irq from the irqchip controller
>> without the gpiochip, like any other irq controller.
>
> OK good, sorry if I'm stating the obvious.
>
>> irq_create_mapping (and irq_create_fwspec_mapping) internally calls
>> irq_find_mapping. So if the mapping already exist (the irq is already
>> used before calling to_irq), the existing mapping will be returned. The
>> mapping will be actually created only if needed. It seems to be in line
>> with your explanation, no ?
>
> Yes, but you want to call irq_create_mapping() in slowpath (irq setup)
> and irq_find_mapping() in fastpath (irq handler). Else the first IRQ
> may result in unwelcomed surprises.
>
>> There is really a *lot* of gpio drivers which use irq_create_mapping in
>> the to_irq callback, are these all wrong ?
>
> Yes they are all wrong. They should all be using irq_find_mapping().

So, dumb question from someone trying (but having a hard time) to follow
and understand the rationale...

If it's wrong enough to completely reject, why are changes still being
merged that are doing it so wrong?  (e.g. like this one[1], just merged
for v4.9)

Kevin

[1] 0eb9f683336d pinctrl: Add IRQ support to STM32 gpios
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/stm32/pinctrl-stm32.c?id=0eb9f683336d7eb99a3b75987620417c574ffb57

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Linus Walleij]

On Wed, Oct 26, 2016 at 5:50 PM, Kevin Hilman <khilman@baylibre.com> wrote:

>> Yes they are all wrong. They should all be using irq_find_mapping().
>
> So, dumb question from someone trying (but having a hard time) to follow
> and understand the rationale...
>
> If it's wrong enough to completely reject, why are changes still being
> merged that are doing it so wrong?  (e.g. like this one[1], just merged
> for v4.9)

It's a bug.

It's that problem that Wolfram brought up in a recent lecture
about maintainer scaling: if noone but the subsystem maintainer
reviews the code, things like this will happen.

I need more review...

> [1] 0eb9f683336d pinctrl: Add IRQ support to STM32 gpios
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/pinctrl/stm32/pinctrl-stm32.c?id=0eb9f683336d7eb99a3b75987620417c574ffb57

Alexandre, Maxime: can you please make a patch for the STM32
driver that remove the semantic dependence for .to_irq() to be called
before an interrupt can be used? It should be possible to use
the irqs directly from the irqchip.

Yours,
Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Linus Walleij]

On Tue, Oct 25, 2016 at 4:47 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 25/10/16 15:22, Jerome Brunet wrote:

>> There is a few problems to guarantee that gpio == hwirq.
>> 1. We have 2 instances of pinctrl, to guarantee that the linux gpio
>> number == hwirq, we would have to guarantee the order in which they are
>> probed. At least this my understanding
>
> Maybe I wasn't clear enough, and my use of gpio is probably wrong. So
> Linux has a gpio number, which is obviously an abstract number (just
> like the Linux irq number). But the pad number, in the context of given
> SoC, is constant. So we have:
>
>         pad->gpio
>         hwirq->irq
>
> Why can't you have pad == hwirq, always? This is already what you have
> in the irqchip driver. This would simplify a lot of things.

My thought as well.

We usually refer to the local numberspace on the GPIO controller
as "offsets", so line offsets 0...31 on a gpiochip with 31 lines.

The ngpio in struct gpio_chip is the number of lines on that controller,
and should nominally map 1:1 to hwirq sources.

Yours,
Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Jerome Brunet]

On Tue, 2016-10-25 at 20:10 +0200, Linus Walleij wrote:
> On Tue, Oct 25, 2016 at 4:47 PM, Marc Zyngier <marc.zyngier@arm.com>
> wrote:
> > 
> > On 25/10/16 15:22, Jerome Brunet wrote:
> 
> > 
> > > 
> > > There is a few problems to guarantee that gpio == hwirq.
> > > 1. We have 2 instances of pinctrl, to guarantee that the linux
> > > gpio
> > > number == hwirq, we would have to guarantee the order in which
> > > they are
> > > probed. At least this my understanding
> > 
> > Maybe I wasn't clear enough, and my use of gpio is probably wrong.
> > So
> > Linux has a gpio number, which is obviously an abstract number
> > (just
> > like the Linux irq number). But the pad number, in the context of
> > given
> > SoC, is constant. So we have:
> > 
> >         pad->gpio
> >         hwirq->irq
> > 
> > Why can't you have pad == hwirq, always? This is already what you
> > have
> > in the irqchip driver. This would simplify a lot of things.
> 
> My thought as well.
> 
> We usually refer to the local numberspace on the GPIO controller
> as "offsets", so line offsets 0...31 on a gpiochip with 31 lines.
> 
> The ngpio in struct gpio_chip is the number of lines on that
> controller,
> and should nominally map 1:1 to hwirq sources.

Indeed it should be the the case, and for meson, it is pretty close.
The irqchip controller provide a number of hwirq. Each hwirq maps to
one, and only one, pin. But since not every pins are connected to the
irqchip controller, the opposite is not true.

Taking an example with 16 gpios, here is what it could look like with
the exception we have on meson :

gpio offset [ 0  1  2  3  4  5  6  7  8  9  10 11 12 13 14 15 ]
hwirq num   [ 0  1  2  3] NC NC[4  5  6  7  8  9  10]NC NC NC

Like gpio offset are used (internally) in the driver to find
appropriate gpio registers and bit, the hwirq has a meaning too.
It is the setting you put in the channel multiplexer of the controller
to select the proper pin to spy on.

In the end, these gpio offset and hwirq number are different. I would
prefer to have hwirq == gpio and go your way, it would make my life
easier, but I don't see how it would work.

The irqchip controller cares only about the hwirq number. You can
actually request an interrupt directly to the controller by asking the
proper hwirq number (in DT for example), without involving the gpio
driver (tested).

The relation between the pins and the interrupt number is provided by
the manufacturer in the Datasheet [1], in the section GPIO Interrupt.

Looking at other gpio drivers, it is not uncommon to have some simple
calculation to get from gpio offset to the hwirq number. I don't get
what is the specific problem here ?

If I missed something, feel free to point it out.

[1] http://dn.odroid.com/S805/Datasheet/S805_Datasheet%20V0.8%202015012
6.pdf
> 
> Yours,
> Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Linus Walleij]

On Wed, Oct 26, 2016 at 4:23 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> [Me]
>> We usually refer to the local numberspace on the GPIO controller
>> as "offsets", so line offsets 0...31 on a gpiochip with 31 lines.
>>
>> The ngpio in struct gpio_chip is the number of lines on that
>> controller,
>> and should nominally map 1:1 to hwirq sources.
>
> Indeed it should be the the case, and for meson, it is pretty close.
> The irqchip controller provide a number of hwirq. Each hwirq maps to
> one, and only one, pin. But since not every pins are connected to the
> irqchip controller, the opposite is not true.
>
> Taking an example with 16 gpios, here is what it could look like with
> the exception we have on meson :
>
> gpio offset [ 0  1  2  3  4  5  6  7  8  9  10 11 12 13 14 15 ]
> hwirq num   [ 0  1  2  3] NC NC[4  5  6  7  8  9  10]NC NC NC
>
> Like gpio offset are used (internally) in the driver to find
> appropriate gpio registers and bit, the hwirq has a meaning too.
> It is the setting you put in the channel multiplexer of the controller
> to select the proper pin to spy on.
>
> In the end, these gpio offset and hwirq number are different. I would
> prefer to have hwirq == gpio and go your way, it would make my life
> easier, but I don't see how it would work.
>
> The irqchip controller cares only about the hwirq number. You can
> actually request an interrupt directly to the controller by asking the
> proper hwirq number (in DT for example), without involving the gpio
> driver (tested).
>
> The relation between the pins and the interrupt number is provided by
> the manufacturer in the Datasheet [1], in the section GPIO Interrupt.

I think I kind of get it.

This reminds me of recent patches to the generic GPIOLIB_IRQCHIP
where we made it possible to "mask" set of IRQ lines, saying
"these are in the irqdomain, but you cannot map them".

See
commit 79b804cb6af4f128b2c53f0887c02537a7eb5824
"gpiolib: Make it possible to exclude GPIOs from IRQ domain"
commit 96b2cca64fa3e1a31b573bb308b2944c802aacf3
"gpio: stmpe: forbid unused lines to be mapped as IRQs"

So what we do in the generic case is put a linear map over all
the lines/offsets, then punch holes in that map and say
"this and this and this can not be mapped as IRQ".

As you can see in _gpiochip_irqchip_add() we pre-map all
except those with irq_create_mapping().

Does this scheme fit with your usecase? I would guess so,
just that your domain is hierarchic, not simple/linear.

Maybe the takeaway is that your map does not need to
be "dense", i.e. every hwirq is in use. It can be sparse.
It is stored in a radix tree anyways.

> Looking at other gpio drivers, it is not uncommon to have some simple
> calculation to get from gpio offset to the hwirq number. I don't get
> what is the specific problem here ?

It's OK to use the offset vs hwirq.

I just misunderstand it as the global GPIO number, that is
not OK.

Yours,
Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Jerome Brunet]

On Wed, 2016-10-26 at 16:44 +0200, Linus Walleij wrote:
> On Wed, Oct 26, 2016 at 4:23 PM, Jerome Brunet <jbrunet@baylibre.com>
> wrote:
> > 
> > [Me]
> > > 
> > > We usually refer to the local numberspace on the GPIO controller
> > > as "offsets", so line offsets 0...31 on a gpiochip with 31 lines.
> > > 
> > > The ngpio in struct gpio_chip is the number of lines on that
> > > controller,
> > > and should nominally map 1:1 to hwirq sources.
> > 
> > Indeed it should be the the case, and for meson, it is pretty
> > close.
> > The irqchip controller provide a number of hwirq. Each hwirq maps
> > to
> > one, and only one, pin. But since not every pins are connected to
> > the
> > irqchip controller, the opposite is not true.
> > 
> > Taking an example with 16 gpios, here is what it could look like
> > with
> > the exception we have on meson :
> > 
> > gpio offset [ 0  1  2  3  4  5  6  7  8  9  10 11 12 13 14 15 ]
> > hwirq num   [ 0  1  2  3] NC NC[4  5  6  7  8  9  10]NC NC NC
> > 
> > Like gpio offset are used (internally) in the driver to find
> > appropriate gpio registers and bit, the hwirq has a meaning too.
> > It is the setting you put in the channel multiplexer of the
> > controller
> > to select the proper pin to spy on.
> > 
> > In the end, these gpio offset and hwirq number are different. I
> > would
> > prefer to have hwirq == gpio and go your way, it would make my life
> > easier, but I don't see how it would work.
> > 
> > The irqchip controller cares only about the hwirq number. You can
> > actually request an interrupt directly to the controller by asking
> > the
> > proper hwirq number (in DT for example), without involving the gpio
> > driver (tested).
> > 
> > The relation between the pins and the interrupt number is provided
> > by
> > the manufacturer in the Datasheet [1], in the section GPIO
> > Interrupt.
> 
> I think I kind of get it.
> 
> This reminds me of recent patches to the generic GPIOLIB_IRQCHIP
> where we made it possible to "mask" set of IRQ lines, saying
> "these are in the irqdomain, but you cannot map them".
> 
> See
> commit 79b804cb6af4f128b2c53f0887c02537a7eb5824
> "gpiolib: Make it possible to exclude GPIOs from IRQ domain"
> commit 96b2cca64fa3e1a31b573bb308b2944c802aacf3
> "gpio: stmpe: forbid unused lines to be mapped as IRQs"
> 
> So what we do in the generic case is put a linear map over all
> the lines/offsets, then punch holes in that map and say
> "this and this and this can not be mapped as IRQ".
> 
> As you can see in _gpiochip_irqchip_add() we pre-map all
> except those with irq_create_mapping().
> 
> Does this scheme fit with your usecase? I would guess so,
> just that your domain is hierarchic, not simple/linear.

Thanks for pointing this out, however I don't think this solve my
issue. I'll try to be as clear as possible but feel free to ask me
question if needed:

Ressource issue : When you create an irq mapping, in case of hierarchic
domain, it calls the "alloc" function of the domain, which will
eventually call the "alloc" function of the parent domain ... until you
reach the "root" domain (here the gic).

The particular HW at hand (meson gpio interrupt controller) is a set of
8 muxes (or channels). Each channel output its signal on 1 specific GIC
input. Its the HW wiring, not programmable.
The inputs are the all pad that can be seen by the controller (*almost*
all the SoC gpio, but not all, as explain earlier). When you call
"alloc", the driver find an available channel, set the mux input to
forward the appropriate signal to the GIC.

As you may understand, the driver can accept only 8 mappings at a time
before being out of GIC irqs, and returning -ENOSPC.

If we were trying the use the punch hole method, we would have to know
at boot time the only eight pin we want, and this for every platform.
Also there not might be 8 available to the gpio subsys, since someone
could request an irq directly to controller, w/o going through the gpio
subsys. This would be putting restriction on the gpio because of an
issue in the controller. This looks very complicated to setup, static
and platform specific. That's not really what we were aiming for.

We are looking to create mapping "on-demand" to make the best use of
the little number of interrupts we have. To catch request of drivers,
like gpio-keys, which use gpio_to_irq, it looks the only viable place
is the to_irq callback (at the moment).

Drivers using gpio_to_irq in their probe function expect that this will
give them the corresponding virq, so create the mapping if need be.

However, I now get why you don't want that, it seems we have 2 types of
platforms in the kernel right now: 

1. The one creating the mapping in the to_irq callback. It might be
because they just copy/paste from another driver doing it, or they may
have a good reason for it (like I think we do)

2. the one which call gpio_to_irq in interrupt handlers. Honestly I did
not know that one could that, but they are in the mainline too, and
probably have a good reason for doing it.

irq_find_mapping looks safe in interrupt handler, I does not seem to
sleep (please correct me if I'm wrong).
irq_create_mapping definitely isn't, because of the irq_domain mutex.

We probably got into this situation because it wasn't clear enough
whether to_irq was fast or slow (at least it took me a few mails to
understand this ...)
The two platform groups are most likely exclusive so nobody is sleeping
in irq, everybody is happy. As a maintainer, I understand that you
can't allow a dangerous situation like this to continue.

To fix the situation we could add a few things in the gpio subsys:
- Make it clear that to_irq is fast (like you just did)
- add a new callback (to_irq_slow ? provide_irq ? something else) which
would be allowed to sleep and create mappings.
- in gpio_to_irq function keeps calling to_irq like it does but also
call to_irq_slow only if we are not in an interrupt context and a
mapping could not be found. We could maybe use "in_interrupt()" for
this ?

This way, we could keep the existing drivers working as they are (even
if they are wrong) and slowly fix things up.

What do you think about this ? Do you have something else in mind ?
I'd be happy to help on this.

Sorry, it was kind of long explanation but I hope things are more clear
now.

> 
> Maybe the takeaway is that your map does not need to
> be "dense", i.e. every hwirq is in use. It can be sparse.
> It is stored in a radix tree anyways.
> 
> > 
> > Looking at other gpio drivers, it is not uncommon to have some
> > simple
> > calculation to get from gpio offset to the hwirq number. I don't
> > get
> > what is the specific problem here ?
> 
> It's OK to use the offset vs hwirq.
> 
> I just misunderstand it as the global GPIO number, that is
> not OK.

Ok. Just to be clear, you are ok with the function
"meson_gpio_to_hwirq" which just does this translation, right ?

> 
> Yours,
> Linus Walleij


Cheers
Jerome

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Linus Walleij]

On Thu, Oct 27, 2016 at 12:42 PM, Jerome Brunet <jbrunet@baylibre.com> wrote:

> Ressource issue : When you create an irq mapping, in case of hierarchic
> domain, it calls the "alloc" function of the domain, which will
> eventually call the "alloc" function of the parent domain ... until you
> reach the "root" domain (here the gic).
(...)
> We are looking to create mapping "on-demand" to make the best use of
> the little number of interrupts we have. To catch request of drivers,
> like gpio-keys, which use gpio_to_irq, it looks the only viable place
> is the to_irq callback (at the moment).
>
> Drivers using gpio_to_irq in their probe function expect that this will
> give them the corresponding virq, so create the mapping if need be.

OK what I need to know is the following:

If this was not a gpio chip, just some random hierarchical irqchip
or mux from drivers/irqchip, where would you make the translation?

The answer to that question applies equally to any GPIO controller
that also provides an irqchip. .to_irq() is not the place to do the
translation.

I looked around and for example irq-s3c24xx.c seems to do this
in the irqdomain xlate() callback, which should only be called
when the interrupt is resolved for a consumer.

If that is the code that you partitioned over to drivers/irqchip,
then maybe this is a sign that this code should not be there
at all, but instead inside this gpiochip driver, or atleast accessible
by it so that your gpiochip driver has direct access to the
irqdomain it is using.

> However, I now get why you don't want that, it seems we have 2 types of
> platforms in the kernel right now:
>
> 1. The one creating the mapping in the to_irq callback. It might be
> because they just copy/paste from another driver doing it, or they may
> have a good reason for it (like I think we do)
>
> 2. the one which call gpio_to_irq in interrupt handlers. Honestly I did
> not know that one could that, but they are in the mainline too, and
> probably have a good reason for doing it.

They are probably all bad or legacy. None of this works with a
irqchip from DT like this:

foo: gpio@0 {
    #gpio-cells = <2>;
    gpio-controller;
    interrupt-cells = <2>;
    interrupt-controller;
}

bar: bar@1 {
    interrupt-parent = <&foo>;
    interrupt = <4>;
};

Here notice that bar is NOT doing gpios = <&foo 4>;
which is what you would do to get a GPIO and then call
.to_irq() on it. It just uses it as an interrupt controller.

This MUST work, or you cannot put interrupt-controller;
as a keyword on the gpiochip.

> irq_find_mapping looks safe in interrupt handler, I does not seem to
> sleep (please correct me if I'm wrong).
> irq_create_mapping definitely isn't, because of the irq_domain mutex.

Yep.

> We probably got into this situation because it wasn't clear enough
> whether to_irq was fast or slow (at least it took me a few mails to
> understand this ...)

I don't know either. It's just supposed to be a quick helper
to find the corresponding interrupt for a GPIO, it is not supposed
to have semantic side-effects.

> The two platform groups are most likely exclusive so nobody is sleeping
> in irq, everybody is happy. As a maintainer, I understand that you
> can't allow a dangerous situation like this to continue.

It's a mess allright. I need everyone's help to fix the mess.

> To fix the situation we could add a few things in the gpio subsys:
> - Make it clear that to_irq is fast (like you just did)

Sure patches accepted.

> - add a new callback (to_irq_slow ? provide_irq ? something else) which
> would be allowed to sleep and create mappings.
> - in gpio_to_irq function keeps calling to_irq like it does but also
> call to_irq_slow only if we are not in an interrupt context and a
> mapping could not be found. We could maybe use "in_interrupt()" for
> this ?

None of them should be allowed to create mappings because of the
explanation above: gpiochips and irqchips need to be
orthogonal.

> This way, we could keep the existing drivers working as they are (even
> if they are wrong) and slowly fix things up.

It doesn't seem to help with anything.

Instead go back to what I described above: if it was just
two irqchips: forget the fact that one of them is a GPIO chip
for a while, just two irqchips.

How does one irqchip map to another one?

That is what needs to happen, solely using the irqchip
infrastructure, NOT using .to_irq().

> Sorry, it was kind of long explanation but I hope things are more clear
> now.

I think I understand... famous last words.

>> I just misunderstand it as the global GPIO number, that is
>> not OK.
>
> Ok. Just to be clear, you are ok with the function
> "meson_gpio_to_hwirq" which just does this translation, right ?

That seems all right, the problem is relying on gpio_to_irq()
being called for the interrupts to work.

Yours,
Linus Walleij

Re: [PATCH 4/9] pinctrl: meson: allow gpio to request irq

[Thomas Gleixner]

On Tue, 25 Oct 2016, Linus Walleij wrote:
> On Fri, Oct 21, 2016 at 11:06 AM, Jerome Brunet <jbrunet@baylibre.com> wrote:
> 
> >> Isn't this usecase (also as described in the cover letter) a textbook
> >> example of when you should be using hierarchical irqdomain?
> >>
> >> Please check with Marc et al on hierarchical irqdomains.
> >
> > Linus,
> > Do you mean I should create a new hierarchical irqdomains in each of
> > the two pinctrl instances we have in these SoC, these domains being
> > stacked on the one I just added for controller in irqchip ?
> >
> > I did not understand this is what you meant when I asked you the
> > question at ELCE.
> 
> Honestly, I do not understand when and where to properly use
> hierarchical irqdomain, even after Marc's talk at ELC-E.

Hierarchical irqdomains are used when you have several levels of interrupt
hardware to deliver an interrupt.

For example on x86 we have:

 device --- [IOAPIC] -- [VECTOR]

and we can have this expanded to

 device --- [IOAPIC] -- [IRQ Remapping] -- [VECTOR]

and we have more things hanging off the VECTOR domain

 device --- [IOAPIC] ---
                       |--- [VECTOR]
 device --- [PCIMSI] ---

So with irq remapping this might look like this:

 device --- [IOAPIC] ---
                       |-----------------------
 device --- [PCIMSI] ---                      |
                                              |---[VECTOR]
 device --- [IOAPIC] ---                      |
                       |--[IRQ Remapping]------
 device --- [PCIMSI] ---                      

The important part is that this hierarchy deals with a single Linux virq
and all parts of the hierarchy are required for setup and possibly for
mask/ack/eoi.

This is different from a demultiplex interrupt

 device --- [DEMUX] --- [GIC]

where the demultiplex interrupt is a different virq than the device
virq. The demux interrupt chip can have a parent relation ship, which can
be required to propagate information, e.g. wake on a device behind the
demux must keep the gic as a wake irq as well. But it's not hierarchical in
the sense of our hierarchical irq domains.

Hope that helps.

Thanks,

	tglx

[PATCH 5/9] dt-bindings: pinctrl: meson: update gpio dt-bindings

[Jerome Brunet]

Add description for the interrupt-parent property of the gpio sub-node
If provided here, this property must be a phandle to an interrupt
controller suitable for meson pinctrl, like the meson gpio interrupt
controller.

Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
index fe7fe0b03cfb..39932c4dfb32 100644
--- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
@@ -23,6 +23,10 @@ Required properties for sub-nodes are:
  - gpio-controller: identifies the node as a gpio controller
  - #gpio-cells: must be 2
 
+Optional property for sub-nodes is:
+ - interrupt-parent: must be a phandle to the meson gpio interrupt controller.
+   if this property is provided, enables gpio ability to generate interrupts
+
 === Other sub-nodes ===
 
 Child nodes without the "gpio-controller" represent some desired
-- 
2.7.4

[PATCH 6/9] ARM64: meson: enable MESON_IRQ_GPIO in Kconfig

[Jerome Brunet]

Add select MESON_IRQ_GPIO in Kconfig for Amlogic's meson SoC family

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index cfbdf02ef566..846479d4492d 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -95,6 +95,7 @@ config ARCH_MESON
 	select PINCTRL_MESON
 	select COMMON_CLK_AMLOGIC
 	select COMMON_CLK_GXBB
+	select MESON_GPIO_IRQ
 	help
 	  This enables support for the Amlogic S905 SoCs.
 
-- 
2.7.4

[PATCH 7/9] ARM: meson: enable MESON_IRQ_GPIO in Kconfig for meson8

[Jerome Brunet]

Add select MESON_IRQ_GPIO in Kconfig for Amlogic's meson8 and meson8b SoC

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 arch/arm/mach-meson/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/mach-meson/Kconfig b/arch/arm/mach-meson/Kconfig
index b6e3acc63e14..63157295cd9d 100644
--- a/arch/arm/mach-meson/Kconfig
+++ b/arch/arm/mach-meson/Kconfig
@@ -21,11 +21,13 @@ config MACH_MESON8
 	bool "Amlogic Meson8 SoCs support"
 	default ARCH_MESON
 	select MESON6_TIMER
+	select MESON_IRQ_GPIO
 
 config MACH_MESON8B
 	bool "Amlogic Meson8b SoCs support"
 	default ARCH_MESON
 	select MESON6_TIMER
 	select COMMON_CLK_MESON8B
+	select MESON_IRQ_GPIO
 
 endif
-- 
2.7.4

[PATCH 8/9] ARM64: dts: amlogic: enable gpio interrupt controller on gxbb

[Jerome Brunet]

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index aad639ab0112..5208cb80b55e 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -141,6 +141,13 @@
 		#reset-cells = <1>;
 	};
 
+	gpio_interrupt: interrupt-controller@9880 {
+		compatible = "amlogic,meson-gxbb-gpio-intc";
+		reg = <0x0 0x9880 0x0 0x10>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
+
 	uart_B: serial@84dc {
 		compatible = "amlogic,meson-uart";
 		reg = <0x0 0x84dc 0x0 0x14>;
@@ -238,6 +245,7 @@
 			reg-names = "mux", "pull", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-parent = <&gpio_interrupt>;
 		};
 
 		uart_ao_a_pins: uart_ao_a {
@@ -343,6 +351,7 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-parent = <&gpio_interrupt>;
 		};
 
 		emmc_pins: emmc {
-- 
2.7.4

[PATCH 9/9] ARM: dts: amlogic: enable gpio interrupt controller on meson8

[Jerome Brunet]

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 arch/arm/boot/dts/meson8.dtsi  | 11 +++++++++++
 arch/arm/boot/dts/meson8b.dtsi | 11 +++++++++++
 2 files changed, 22 insertions(+)

diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index 45619f6162c5..713a22456ff1 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -43,6 +43,8 @@
  *     OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/gpio/meson8-gpio.h>
 /include/ "meson.dtsi"
 
@@ -91,6 +93,13 @@
 		clock-frequency = <141666666>;
 	};
 
+	gpio_interrupt: interrupt-controller@c1109880 {
+		compatible = "amlogic,meson8-gpio-intc";
+		reg = <0xc1109880 0x10>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+	};
+
 	pinctrl_cbus: pinctrl@c1109880 {
 		compatible = "amlogic,meson8-cbus-pinctrl";
 		reg = <0xc1109880 0x10>;
@@ -106,6 +115,7 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-parent = <&gpio_interrupt>;
 		};
 
 		spi_nor_pins: nor {
@@ -148,6 +158,7 @@
 			reg-names = "mux", "pull", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-parent = <&gpio_interrupt>;
 		};
 
 		uart_ao_a_pins: uart_ao_a {
diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index 41fd53671859..36a239a645f5 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -44,6 +44,8 @@
  *     OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/clock/meson8b-clkc.h>
 #include <dt-bindings/gpio/meson8b-gpio.h>
 #include <dt-bindings/reset/amlogic,meson8b-reset.h>
@@ -183,6 +185,13 @@
 			status = "disabled";
 		};
 
+		gpio_interrupt: interrupt-controller@c1109880 {
+			compatible = "amlogic,meson8b-gpio-intc";
+			reg = <0xc1109880 0x10>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
 		pinctrl_cbus: pinctrl@c1109880 {
 			compatible = "amlogic,meson8b-cbus-pinctrl";
 			reg = <0xc1109880 0x10>;
@@ -198,6 +207,7 @@
 				reg-names = "mux", "pull", "pull-enable", "gpio";
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-parent = <&gpio_interrupt>;
 			};
 		};
 
@@ -215,6 +225,7 @@
 				reg-names = "mux", "pull", "gpio";
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-parent = <&gpio_interrupt>;
 			};
 
 			uart_ao_a_pins: uart_ao_a {
-- 
2.7.4