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

[PATCH v2 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

Changes since v1 : [3]
 * Correct mistake in patch 4 when no compatible controller is found. Sorry
   for the inconvenience.

[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
[3] : http://lkml.kernel.org/r/1476871709-8359-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 v2 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 v2 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 v2 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 v2 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

[PATCH v2 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 v2 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 v2 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 v2 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 v2 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

[RESEND PATCH v2 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>
---

I messed up in v2 and actually sent the v1 again. Here is the actual v2
with the fix. Again, sorry for the inconvenience.

 drivers/pinctrl/Kconfig               |  2 +
 drivers/pinctrl/meson/pinctrl-meson.c | 69 +++++++++++++++++++++++++++++++++++
 drivers/pinctrl/meson/pinctrl-meson.h |  1 +
 3 files changed, 72 insertions(+)

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..e3f5241f337f 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,19 @@ static struct regmap *meson_map_resource(struct meson_pinctrl *pc,
 	return devm_regmap_init_mmio(pc->dev, base, &meson_regmap_config);
 }
 
+static void meson_pinctrl_get_irq_gpio_intc(struct meson_pinctrl *pc,
+					    struct device_node *node)
+{
+	struct device_node *np = of_irq_find_parent(node);
+
+	if (!np || !of_device_is_compatible(np, pc->data->irq_compat)) {
+		dev_info(pc->dev, "gpio interrupt disabled\n");
+		pc->fwnode = NULL;
+	} else {
+		pc->fwnode = of_node_to_fwnode(np);
+	}
+}
+
 static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
 				  struct device_node *node)
 {
@@ -643,6 +710,8 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
 		return PTR_ERR(pc->reg_gpio);
 	}
 
+	meson_pinctrl_get_irq_gpio_intc(pc, gpio_np);
+
 	return 0;
 }
 
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 v2 1/9] irqchip: meson: add support for gpio interrupt controller

[Marc Zyngier]

Jerome,

On 19/10/16 16:21, Jerome Brunet wrote:
> 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 {

The name of that structure is utterly confusing, as it doesn't contain
anything related to an IRQ domain. Can you please clarify its usage?

> +	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,
> +};

If that a guarantee that these numbers will always represent the parent
interrupt? It feels a bit odd not to get that information directly from
the device tree, in the form of a device specific property. Something like:

	upstream-interrupts = <64 65 66 ... >;

or even as a base/range:

	upstream-interrupts = <64 8>;

Also, how does it work if you have more than a single device like this?
This driver seems a do a great deal of dynamic allocation, and yet its
core resource is completely static... Please pick a side! ;-)

> +
> +static const struct meson_gpio_irq_params meson8_params = {
> +	.nhwirq  = 134,
> +	.source  = meson_parent_hwirqs,
> +	.nsource = ARRAY_SIZE(meson_parent_hwirqs),

I find it utterly confusing that you are calling source something that
is an output for this controller. It makes my brain melt, and I don't
like the feeling.

> +};
> +
> +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),
> +};

Same thing. How big is the variability of these structures? Are we going
to see more of those? or is that now set into stone?

+Mark: what's the policy to describe this kind of things?

> +
> +static const struct of_device_id meson_irq_gpio_matches[] = {
> +	{
> +		.compatible = "amlogic,meson8-gpio-intc",

If it's an independent IP (as described in the commit message),
shouldn't in be rescribed in a more SoC-independent way?

> +		.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);

Can't you use the relaxed accessors?

> +}
> +
> +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;

I'm a bit worried by this function. If you need an allocator, then
having a simple bitmap is much better than iterating over an array.

If you're using this to go from a hwirq to the structure describing your
interrupt, this is wrong. You should never have to look-up something
based on a hwirq, because that's what irq domains are for.

> +}
> +
> +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);

So assuming you turn this into a proper bitmap driven allocator...

> +	if (index < 0) {
> +		pr_err("No irq available\n");
> +		return -ENOSPC;
> +	}
> +
> +	domain_data->map[index] = hwirq;

this can go away, as there is hardly any point in tracking the hwirq on
its own. Actually, the whole map[] array looks totally useless.

> +
> +	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;

You can write this as:

	if (is_of_node() && fwspec->... == 2) {

> +
> +		*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;

How can you end-up here if the translate operation has failed?

> +
> +	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++) {

This is a pattern that gets repeated over and over, for no good reason.
The only reason we have this nr_irqs thing is to handle things like
multi-MSI, where we have to *guarantee* that the hwirqs are allocated in
a contiguous manner.

Here, you don't enforce that guarantee, so you'd rather have a big fat:

	if (WARN_ON(nr_irqs != 1))
		return -EINVAL;

and get rid of that loop, because I cannot imagine a case where you'd
allocate more than a single interrupt at a time.

> +		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;

Why do you have to to this here? This should be handled by the core code
already.

> +
> +		cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> +		if (!cd)
> +			return -ENOMEM;
> +
> +		cd->base = domain_data->base;
> +		cd->index = index;

So what is the exact purpose of this structure? The base address is
essentially a global, or could be directly derived from the domain. The
index is only used in set_type, and is the index of the pin connected to
the GIC. It looks to me like you could have:

		irq_hw_number_t *out_line = &meson_parent_hwirqs[index];

> +
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, hwirq + i,
> +					      &meson_gpio_irq_chip, cd);

and this written as

		irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
					      out_line);

In your set_type function, you just compute the index back:

	irq_hw_number_t *out_line = irq_data_get_irq_chip_data(data);
	irq_hw_number_t index = out_line - meson_parent_hwirqs;

and you're done.

> +
> +		ret = meson_gpio_irq_allocate_gic_irq(domain, virq + i,
> +						      source, type);

Resource leak on error.

> +		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++) {

Same comment as above.

> +		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;

You should now be able to kill most or all of this.

> +	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);

Please be consistent in using the fwnode API instead of the of_node one.

> +
> +	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);
> 

Overall, this driver is a bit of a mess. Tons of structures that don't
make much sense, and a false sense of being able to support multiple
instances of the device.

I'll let Mark comment on the DT side of things.

Thanks,

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

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

[Marc Zyngier]

On 19/10/16 16:21, Jerome Brunet wrote:
> 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

MESON_IRQ_GPIO?

>  	help
>  	  This enables support for the Amlogic S905 SoCs.
>  
> 


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

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

[Jerome Brunet]

On Thu, 2016-10-20 at 17:33 +0100, Marc Zyngier wrote:
> Jerome,
> 
> On 19/10/16 16:21, Jerome Brunet wrote:
> > 
> > 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/licens
> > es/>.
> > + * 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 {
> 
> The name of that structure is utterly confusing, as it doesn't
> contain
> anything related to an IRQ domain. Can you please clarify its usage?

This structure is holding the data of the controller. The name is
indeed misleading, I will change this. What about
'meson_gpio_irq_pdata' or 'meson_gpio_irq_controller' ?

> 
> > 
> > +	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,
> > +};
> 
> If that a guarantee that these numbers will always represent the
> parent interrupt?

At the moment, the 3 supported SoC use these parent interrupts, but we
have absolutely no idea (or guarantee) that is will remain the same, or
even contiguous, in the upcoming SoC (like the GXM or GXL)

I reckon, it is likely that manufacturer will keep on using these
parent irqs for a while but I would prefer not make an assumption about
it in the driver.

If a SoC get a different set of interrupts I would have added a new
table like this and passed it to the appropriate params :

static irq_hw_number_t meson_new_parent_hwirqs[] = {
	143, 144, 150, 151, 152, 173, 178, 179,
};

> It feels a bit odd not to get that information directly from
> the device tree, in the form of a device specific property. Something
> like:
> 
> 	upstream-interrupts = <64 65 66 ... >;
> 

I wondered about putting this information in DT or in the driver for a
while. Maybe DT would be a more suitable place holder for these data
(parent irq and number of provided hwirq) but I was under the
understanding that we should now put these information in the driver
and use the compatible property to get the appropriate parameters.

I'd love to get the view of the DT guys on this.

Also, since we already need to put some information about the pin the
pinctrl driver (to get the appropriate mux value of each pad), I
thought It would make sense to have the same approach here.

If there is some kind of policy saying this should be in DT, I'd be
happy to make the change.

> or even as a base/range:
> 
> 	upstream-interrupts = <64 8>;

I would prefer to avoid using ranges as we have no guarantee the
manufacturer will keep the parent irqs contiguous in the future.

> 
> Also, how does it work if you have more than a single device like
> this?
> This driver seems a do a great deal of dynamic allocation, and yet
> its
> core resource is completely static... Please pick a side! ;-)

I don't think we could have more than one instance per device and I
certainly did not have this case in mind while writing the code. If it
was the case someday, I guess having two compatibles would do the trick
:)

> 
> > 
> > +
> > +static const struct meson_gpio_irq_params meson8_params = {
> > +	.nhwirq  = 134,
> > +	.source  = meson_parent_hwirqs,
> > +	.nsource = ARRAY_SIZE(meson_parent_hwirqs),
> 
> I find it utterly confusing that you are calling source something
> that is an output for this controller. 

I had the call sequence (GIC->GPIO_IRQ->handler) in mind while writing
it. I see your point and it is indeed confusing, I'll find something
better

> It makes my brain melt, and I don't like the feeling.

Ohhhh !! it's Halloween season and you don't need a costume anymore ;)

> 
> > 
> > +};
> > +
> > +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),
> > +};
> 
> Same thing. How big is the variability of these structures? Are we
> going to see more of those? or is that now set into stone?

The number of pad mapped to the controller seems to change with every
SoC version. The parent irqs have not changed so far, but as explained
above, there is no guarantee it will keep on being this way.

So i'd say probably more of those ...

> 
> +Mark: what's the policy to describe this kind of things?

Very interested about this question as well.

> 
> > 
> > +
> > +static const struct of_device_id meson_irq_gpio_matches[] = {
> > +	{
> > +		.compatible = "amlogic,meson8-gpio-intc",
> 
> If it's an independent IP (as described in the commit message),
> shouldn't in be rescribed in a more SoC-independent way?

Ok, I was probably not very clear (again). What I meant in the cover
letter is that the interrupt gpio controller is independent for the
pad/gpio controller. For example, it could work even you did not setup
anything in pinmux or you did not instantiate pinctrl at all.

But, at least from my point of view, it is SoC dependent since the
number of pad routed to it is changing with every SoC version.

> 
> > 
> > +		.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);
> 
> Can't you use the relaxed accessors?

Indeed, this will be fixed.

> 
> > 
> > +}
> > +
> > +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;
> 
> I'm a bit worried by this function. If you need an allocator, then
> having a simple bitmap is much better than iterating over an array.
> 
> If you're using this to go from a hwirq to the structure describing
> your
> interrupt, this is wrong. You should never have to look-up something
> based on a hwirq, because that's what irq domains are for.

OK

> 
> > 
> > +}
> > +
> > +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);
> 
> So assuming you turn this into a proper bitmap driven allocator...
> 
> > 
> > +	if (index < 0) {
> > +		pr_err("No irq available\n");
> > +		return -ENOSPC;
> > +	}
> > +
> > +	domain_data->map[index] = hwirq;
> 
> this can go away, as there is hardly any point in tracking the hwirq
> on
> its own. Actually, the whole map[] array looks totally useless.
> 
> > 
> > +
> > +	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;
> 
> You can write this as:
> 
> 	if (is_of_node() && fwspec->... == 2) {

OK

> 
> > 
> > +
> > +		*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;
> 
> How can you end-up here if the translate operation has failed?

You can't, will be removed

> 
> > 
> > +
> > +	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++) {
> 
> This is a pattern that gets repeated over and over, for no good
> reason.
> The only reason we have this nr_irqs thing is to handle things like
> multi-MSI, where we have to *guarantee* that the hwirqs are allocated
> in
> a contiguous manner.
> 
> Here, you don't enforce that guarantee, so you'd rather have a big
> fat:
> 
> 	if (WARN_ON(nr_irqs != 1))
> 		return -EINVAL;
> 
> and get rid of that loop, because I cannot imagine a case where you'd
> allocate more than a single interrupt at a time.

Thanks for this clarification. I was actually very confused about
getting a single fwspec and this 'nr_irqs' parameters. I could not
figure a case where one would want multiple irqs in one call, and
looking at the other irqchip drivers did not really help. In the end, I
tried to implement the API the best I could, thinking that somebody
probably had a very good reason for this.

I'm perfectly happy using your solution, it makes more sense.

> 
> > 
> > +		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;
> 
> Why do you have to to this here? This should be handled by the core
> code already.

OK

> 
> > 
> > +
> > +		cd = kzalloc(sizeof(*cd), GFP_KERNEL);
> > +		if (!cd)
> > +			return -ENOMEM;
> > +
> > +		cd->base = domain_data->base;
> > +		cd->index = index;
> 
> So what is the exact purpose of this structure? The base address is
> essentially a global, or could be directly derived from the domain.
> The
> index is only used in set_type, and is the index of the pin connected
> to
> the GIC. It looks to me like you could have:
> 
> 		irq_hw_number_t *out_line =
> &meson_parent_hwirqs[index];

meson_parent_hwirq is only declared this way to avoid writing the
parent irqs 3 times (in each SoC params). 
Using it this way would make it global and imply it is the same
whatever the SoC. This something I can't guarantee and I would prefer
to avoid that.

> 
> > 
> > +
> > +		irq_domain_set_hwirq_and_chip(domain, virq + i,
> > hwirq + i,
> > +					      &meson_gpio_irq_chip
> > , cd);
> 
> and this written as
> 
> 		irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> 					      out_line);
> 
> In your set_type function, you just compute the index back:
> 
> 	irq_hw_number_t *out_line = irq_data_get_irq_chip_data(data);
> 	irq_hw_number_t index = out_line - meson_parent_hwirqs;
> 
> and you're done.
> 

Since I can derive the base address, I only need the index in set_type.
I'll rework this to get rid of this structure.

> > 
> > +
> > +		ret = meson_gpio_irq_allocate_gic_irq(domain, virq
> > + i,
> > +						      source,
> > type);
> 
> Resource leak on error.

Ok, Thx.

> 
> > 
> > +		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++) {
> 
> Same comment as above.

OK

> 
> > 
> > +		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;
> 
> You should now be able to kill most or all of this.
> 
> > 
> > +	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);
> 
> Please be consistent in using the fwnode API instead of the of_node
> one.

OK

> 
> > 
> > +
> > +	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);
> > 
> 
> Overall, this driver is a bit of a mess. Tons of structures that
> don't
> make much sense, and a false sense of being able to support multiple
> instances of the device.
> 
> I'll let Mark comment on the DT side of things.
> 
> Thanks,
> 
> 	M.

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

[Mark Rutland]

On Fri, Oct 21, 2016 at 10:49:11AM +0200, Jerome Brunet wrote:
> On Thu, 2016-10-20 at 17:33 +0100, Marc Zyngier wrote:
> > On 19/10/16 16:21, Jerome Brunet wrote:
> > > +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,
> > > +};
> > 
> > If that a guarantee that these numbers will always represent the
> > parent interrupt?
> 
> At the moment, the 3 supported SoC use these parent interrupts, but we
> have absolutely no idea (or guarantee) that is will remain the same, or
> even contiguous, in the upcoming SoC (like the GXM or GXL)
> 
> I reckon, it is likely that manufacturer will keep on using these
> parent irqs for a while but I would prefer not make an assumption about
> it in the driver.
> 
> If a SoC get a different set of interrupts I would have added a new
> table like this and passed it to the appropriate params :
> 
> static irq_hw_number_t meson_new_parent_hwirqs[] = {
> 	143, 144, 150, 151, 152, 173, 178, 179,
> };
> 
> > It feels a bit odd not to get that information directly from
> > the device tree, in the form of a device specific property. Something
> > like:
> > 
> > 	upstream-interrupts = <64 65 66 ... >;
> > 
> 
> I wondered about putting this information in DT or in the driver for a
> while. Maybe DT would be a more suitable place holder for these data
> (parent irq and number of provided hwirq) but I was under the
> understanding that we should now put these information in the driver
> and use the compatible property to get the appropriate parameters.
> 
> I'd love to get the view of the DT guys on this.

Please describe inter-device relationships in DT when you are aware of
them. The SoC-specific compatible string is more of a future-proofing
thing / last restort for things we realise too late.

To be clear, we should *also* have an soc-specific compatible string,
but for differences we already know about, we should use DT properties.

> > > +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),
> > > +};
> > 
> > Same thing. How big is the variability of these structures? Are we
> > going to see more of those? or is that now set into stone?
> 
> The number of pad mapped to the controller seems to change with every
> SoC version. The parent irqs have not changed so far, but as explained
> above, there is no guarantee it will keep on being this way.
> 
> So i'd say probably more of those ...
> 
> > +Mark: what's the policy to describe this kind of things?

Generally, I'd prefer that we describe this in DT rather than
accumulating a set of string -> number mappings in the driver.

Thanks,
Mark.

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

[Jerome Brunet]

On Fri, 2016-10-21 at 11:10 +0100, Mark Rutland wrote:
> On Fri, Oct 21, 2016 at 10:49:11AM +0200, Jerome Brunet wrote:
> > 
> > On Thu, 2016-10-20 at 17:33 +0100, Marc Zyngier wrote:
> > > 
> > > On 19/10/16 16:21, Jerome Brunet wrote:
> > > > 
> > > > +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,
> > > > +};
> > > 
> > > If that a guarantee that these numbers will always represent the
> > > parent interrupt?
> > 
> > At the moment, the 3 supported SoC use these parent interrupts, but
> > we
> > have absolutely no idea (or guarantee) that is will remain the
> > same, or
> > even contiguous, in the upcoming SoC (like the GXM or GXL)
> > 
> > I reckon, it is likely that manufacturer will keep on using these
> > parent irqs for a while but I would prefer not make an assumption
> > about
> > it in the driver.
> > 
> > If a SoC get a different set of interrupts I would have added a new
> > table like this and passed it to the appropriate params :
> > 
> > static irq_hw_number_t meson_new_parent_hwirqs[] = {
> > 	143, 144, 150, 151, 152, 173, 178, 179,
> > };
> > 
> > > 
> > > It feels a bit odd not to get that information directly from
> > > the device tree, in the form of a device specific property.
> > > Something
> > > like:
> > > 
> > > 	upstream-interrupts = <64 65 66 ... >;
> > > 
> > 
> > I wondered about putting this information in DT or in the driver
> > for a
> > while. Maybe DT would be a more suitable place holder for these
> > data
> > (parent irq and number of provided hwirq) but I was under the
> > understanding that we should now put these information in the
> > driver
> > and use the compatible property to get the appropriate parameters.
> > 
> > I'd love to get the view of the DT guys on this.
> 
> Please describe inter-device relationships in DT when you are aware
> of
> them. The SoC-specific compatible string is more of a future-proofing
> thing / last restort for things we realise too late.
> 
> To be clear, we should *also* have an soc-specific compatible string,
> but for differences we already know about, we should use DT
> properties.
> 
> > 
> > > 
> > > > 
> > > > +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),
> > > > +};
> > > 
> > > Same thing. How big is the variability of these structures? Are
> > > we
> > > going to see more of those? or is that now set into stone?
> > 
> > The number of pad mapped to the controller seems to change with
> > every
> > SoC version. The parent irqs have not changed so far, but as
> > explained
> > above, there is no guarantee it will keep on being this way.
> > 
> > So i'd say probably more of those ...
> > 
> > > 
> > > +Mark: what's the policy to describe this kind of things?
> 
> Generally, I'd prefer that we describe this in DT rather than
> accumulating a set of string -> number mappings in the driver.

Thx Marc. I will change it.

> 
> Thanks,
> Mark.

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

[Marc Zyngier]

On 21/10/16 09:49, Jerome Brunet wrote:
> On Thu, 2016-10-20 at 17:33 +0100, Marc Zyngier wrote:
>> Jerome,
>>
>> On 19/10/16 16:21, Jerome Brunet wrote:
>>>
>>> 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/licens
>>> es/>.
>>> + * 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 {
>>
>> The name of that structure is utterly confusing, as it doesn't
>> contain
>> anything related to an IRQ domain. Can you please clarify its usage?
> 
> This structure is holding the data of the controller. The name is
> indeed misleading, I will change this. What about
> 'meson_gpio_irq_pdata' or 'meson_gpio_irq_controller' ?
> 
>>
>>>
>>> +	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,
>>> +};
>>
>> If that a guarantee that these numbers will always represent the
>> parent interrupt?
> 
> At the moment, the 3 supported SoC use these parent interrupts, but we
> have absolutely no idea (or guarantee) that is will remain the same, or
> even contiguous, in the upcoming SoC (like the GXM or GXL)
> 
> I reckon, it is likely that manufacturer will keep on using these
> parent irqs for a while but I would prefer not make an assumption about
> it in the driver.
> 
> If a SoC get a different set of interrupts I would have added a new
> table like this and passed it to the appropriate params :
> 
> static irq_hw_number_t meson_new_parent_hwirqs[] = {
> 	143, 144, 150, 151, 152, 173, 178, 179,
> };
> 
>> It feels a bit odd not to get that information directly from
>> the device tree, in the form of a device specific property. Something
>> like:
>>
>> 	upstream-interrupts = <64 65 66 ... >;
>>
> 
> I wondered about putting this information in DT or in the driver for a
> while. Maybe DT would be a more suitable place holder for these data
> (parent irq and number of provided hwirq) but I was under the
> understanding that we should now put these information in the driver
> and use the compatible property to get the appropriate parameters.
> 
> I'd love to get the view of the DT guys on this.
> 
> Also, since we already need to put some information about the pin the
> pinctrl driver (to get the appropriate mux value of each pad), I
> thought It would make sense to have the same approach here.
> 
> If there is some kind of policy saying this should be in DT, I'd be
> happy to make the change.
> 
>> or even as a base/range:
>>
>> 	upstream-interrupts = <64 8>;
> 
> I would prefer to avoid using ranges as we have no guarantee the
> manufacturer will keep the parent irqs contiguous in the future.
> 
>>
>> Also, how does it work if you have more than a single device like
>> this?
>> This driver seems a do a great deal of dynamic allocation, and yet
>> its
>> core resource is completely static... Please pick a side! ;-)
> 
> I don't think we could have more than one instance per device and I
> certainly did not have this case in mind while writing the code. If it
> was the case someday, I guess having two compatibles would do the trick
> :)

This has nothing to do with compatible strings. Your current approach of
defining a static list of interrupts and then dynamically allocating
things that point directly to it feels wrong. just define a second
instance of this IP to be convinced of it.

So either you support something that is fully dynamic (supporting
multiple instances at every level), or you only support *one*, and
everything becomes mostly static.

> 
>>
>>>
>>> +
>>> +static const struct meson_gpio_irq_params meson8_params = {
>>> +	.nhwirq  = 134,
>>> +	.source  = meson_parent_hwirqs,
>>> +	.nsource = ARRAY_SIZE(meson_parent_hwirqs),
>>
>> I find it utterly confusing that you are calling source something
>> that is an output for this controller. 
> 
> I had the call sequence (GIC->GPIO_IRQ->handler) in mind while writing
> it. I see your point and it is indeed confusing, I'll find something
> better

It infinitely better to describe the signal path rather than the call
stack (which is actually GIC->GPIO_IRQ->GIC->handler).

[...]


>>>
>>> +
>>> +		cd = kzalloc(sizeof(*cd), GFP_KERNEL);
>>> +		if (!cd)
>>> +			return -ENOMEM;
>>> +
>>> +		cd->base = domain_data->base;
>>> +		cd->index = index;
>>
>> So what is the exact purpose of this structure? The base address is
>> essentially a global, or could be directly derived from the domain.
>> The
>> index is only used in set_type, and is the index of the pin connected
>> to
>> the GIC. It looks to me like you could have:
>>
>> 		irq_hw_number_t *out_line =
>> &meson_parent_hwirqs[index];
> 
> meson_parent_hwirq is only declared this way to avoid writing the
> parent irqs 3 times (in each SoC params). 
> Using it this way would make it global and imply it is the same
> whatever the SoC. This something I can't guarantee and I would prefer
> to avoid that.

Well, let's face it: It is global (see my earlier rant). And if you
device to support multiple instances, you can attach the parent array at
the irq domain level, and still perform that same index calculation.

Thanks,

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

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

[Rob Herring]

On Wed, Oct 19, 2016 at 05:21:13PM +0200, Jerome Brunet wrote:
> 
> 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

As Mark R already said, you should keep the interrupts property.

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

[Mark Rutland]

On Wed, Oct 26, 2016 at 04:42:35PM -0500, Rob Herring wrote:
> On Wed, Oct 19, 2016 at 05:21:13PM +0200, Jerome Brunet wrote:
> > 
> > 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
> 
> As Mark R already said, you should keep the interrupts property.

To be clear, the interrupt routing should be described *somehow*, though
I don't think the generic interrupts property is correct, as this is an
interrupt *router*, i.e. this device doesn't own the interrupt, it just
joins two parts of the line together (and so flags are meaningless).

Thanks,
Mark.

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

[Jerome Brunet]

On Thu, 2016-10-27 at 10:32 +0100, Mark Rutland wrote:
> On Wed, Oct 26, 2016 at 04:42:35PM -0500, Rob Herring wrote:
> > 
> > On Wed, Oct 19, 2016 at 05:21:13PM +0200, Jerome Brunet wrote:
> > > 
> > > 
> > > 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
> > 
> > As Mark R already said, you should keep the interrupts property.
> 
> To be clear, the interrupt routing should be described *somehow*,
> though
> I don't think the generic interrupts property is correct, as this is
> an
> interrupt *router*, i.e. this device doesn't own the interrupt, it
> just
> joins two parts of the line together (and so flags are meaningless).
> 
> Thanks,
> Mark.

Indeed Mark,

I already rewritten the driver taking Marc's comment into account.
There will be 2 properties added to the DT:
 - meson,upstream-interrupts : an array with the 8 gic interrupt used
as output of the controller
-  meson,num-input-mux : the number of inputs (pads) available to the
muxes

I'm looking for solution to the "create mapping" issue we have in
pinctrl (patch 4) before posting a v3.

If you think these properties should be different, feel free to tell me
now.

Thx
Jerome