[PATCH v7 0/9] pinctrl: meson: add support for GPIO IRQs

[PATCH v7 0/9] pinctrl: meson: add support for GPIO IRQs

[Heiner Kallweit]

This patch series is partially based on a series Jerome Brunet
submitted about half a year ago. Due to open questions this series never
made it to mainline, see https://patchwork.kernel.org/patch/9384431/

This new attempt uses GPIOLIB_IRQCHIP resulting in less needed code.

The series was successfully tested on a Odroid-C2.

Changes in v2:
- separate the GPIO IRQ controller from the pinctrl driver
- minor improvements to the GPIO IRQ controller

Changes in v3:
- replace the request_irq based allocation of parent irq's with
  chained irq handling, this also fixes the spurious interrupts issue
  and allows to remove the workaround code.
  Last but not least the parent irq's are no longer visible in
  /proc/interrupts.
- minor improvements to the GPIO IRQ controller

Changes in v4:
- separate the gpio-independent interrupt controller part and make
  it a driver under drivers/irqchip

Changes in v5:
- smaller changes based on review comments
- split DT patches per ARM / ARM64

Changes in v6:
- based on suggestion by Rob Herring: rename DT property parent-interrupts to interrupts

Changes in v7:
- remove patch 1 as it was applied to the pinctrl tree already
- add patch 5 as prerequisite for reworked patch 6
- remove IRQ_TYPE_EDGE_BOTH support from patch 6

Heiner Kallweit (7):
  irqchip: add Amlogic Meson GPIO irqchip driver
  dt-bindings: add Amlogic Meson GPIO interrupt-controller DT binding documentation
  ARM: dts: meson: add GPIO interrupt-controller support
  ARM64: dts: meson: add GPIO interrupt-controller support
  gpiolib: export gpiochip_irq_reqres and gpiochip_irq_relres
  pinctrl: meson: add support for GPIO interrupts
  pinctrl: meson: update DT binding documentation
  ARM: dts: meson: mark gpio controllers as interrupt controllers
  ARM64: dts: meson: mark gpio controllers as interrupt controllers

.../amlogic,meson-gpio-intc.txt                    |  26 ++
 .../devicetree/bindings/pinctrl/meson,pinctrl.txt  |   4 +
 arch/arm/boot/dts/meson8.dtsi                      |  12 +
 arch/arm/boot/dts/meson8b.dtsi                     |  12 +
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi          |   8 +
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi        |   4 +
 arch/arm64/boot/dts/amlogic/meson-gxl.dtsi         |   4 +
 drivers/gpio/gpiolib.c                             |   6 +-
 drivers/irqchip/Kconfig                            |   5 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-meson-gpio.c                   | 295 +++++++++++++++++++++
 drivers/pinctrl/Kconfig                            |   1 +
 drivers/pinctrl/meson/pinctrl-meson.c              | 176 +++++++++++-
 include/linux/gpio/driver.h                        |   3 +
 14 files changed, 554 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

-- 
2.13.1

[PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Heiner Kallweit]

Add a driver supporting the GPIO interrupt controller on certain
Amlogic meson SoC's.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v5:
- changed Kconfig entry based on Neil's suggestion
- added authors
- extended explanation why 2 * n hwirqs are used
v6:
- change DT property parent-interrupts to interrupts
v7:
- no changes
---
 drivers/irqchip/Kconfig          |   5 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 301 insertions(+)
 create mode 100644 drivers/irqchip/irq-meson-gpio.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 478f8ace..bdc86e14 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
 	help
 	  Say yes here to add support for the IRQ combiner devices embedded
 	  in Qualcomm Technologies chips.
+
+config MESON_GPIO_INTC
+	bool
+	depends on ARCH_MESON
+	default y
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index b64c59b8..1be482bd 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
+obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
--- /dev/null
+++ b/drivers/irqchip/irq-meson-gpio.c
@@ -0,0 +1,295 @@
+/*
+ * Amlogic Meson GPIO IRQ chip driver
+ *
+ * Copyright (c) 2015 Endless Mobile, Inc.
+ * Author: Carlo Caione <carlo@endlessm.com>
+ * Copyright (c) 2016 BayLibre, SAS.
+ * Author: Jerome Brunet <jbrunet@baylibre.com>
+ * Copyright (c) 2017 Heiner Kallweit <hkallweit1@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation, version 2.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include <linux/regmap.h>
+
+#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 MAX_PARENT_IRQ_NUM	8
+
+/* maximum number of GPIO IRQs on supported platforms */
+#define MAX_NUM_GPIO_IRQ	133
+
+/*
+ * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
+ * edge. That's due to HW constraints.
+ * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
+ * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
+ * one bit to the right.
+ */
+#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
+
+struct meson_irq_slot {
+	unsigned int irq;
+	unsigned int owner;
+};
+
+struct meson_data {
+	struct regmap *regmap;
+	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
+	unsigned int num_slots;
+	struct mutex lock;
+};
+
+static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -ENOSPC;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (!md->slots[i].owner) {
+			md->slots[i].owner = virq;
+			slot = i;
+		}
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots; i++)
+		if (md->slots[i].owner == virq) {
+			md->slots[i].owner = 0;
+			break;
+		}
+
+	mutex_unlock(&md->lock);
+}
+
+static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
+{
+	int i, slot = -EINVAL;
+
+	mutex_lock(&md->lock);
+
+	for (i = 0; i < md->num_slots && slot < 0; i++)
+		if (md->slots[i].owner == virq)
+			slot = i;
+
+	mutex_unlock(&md->lock);
+
+	return slot;
+}
+
+static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int hwirq)
+{
+	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
+	int shift = 8 * (idx % 4);
+
+	regmap_update_bits(md->regmap, reg, 0xff << shift,
+			   hwirq << shift);
+}
+
+static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot = meson_find_irq_slot(md, data->irq);
+
+	if (slot >= 0)
+		meson_set_hwirq(md, slot, hwirq);
+}
+
+static int meson_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct meson_data *md = data->domain->host_data;
+	int slot;
+	unsigned int val = 0;
+
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		return -EINVAL;
+
+	slot = meson_find_irq_slot(md, data->irq);
+	if (slot < 0)
+		return slot;
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val |= REG_EDGE_POL_EDGE(slot);
+
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
+		val |= REG_EDGE_POL_LOW(slot);
+
+	regmap_update_bits(md->regmap, REG_EDGE_POL,
+			   REG_EDGE_POL_MASK(slot), val);
+
+	if (type & IRQ_TYPE_EDGE_BOTH)
+		val = IRQ_TYPE_EDGE_RISING;
+	else
+		val = IRQ_TYPE_LEVEL_HIGH;
+
+	return irq_chip_set_type_parent(data, val);
+}
+
+static unsigned int meson_irq_startup(struct irq_data *data)
+{
+	irq_chip_unmask_parent(data);
+	/*
+	 * An extra bit was added to allow having the same gpio hwirq twice
+	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
+	 * gpio hwirq.
+	 */
+	meson_irq_set_hwirq(data, data->hwirq >> 1);
+
+	return 0;
+}
+
+static void meson_irq_shutdown(struct irq_data *data)
+{
+	meson_irq_set_hwirq(data, 0xff);
+	irq_chip_mask_parent(data);
+}
+
+static struct irq_chip meson_irq_chip = {
+	.name = "meson_gpio_intc",
+	.irq_set_type = meson_irq_set_type,
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_startup = meson_irq_startup,
+	.irq_shutdown = meson_irq_shutdown,
+	.irq_set_affinity = irq_chip_set_affinity_parent,
+};
+
+static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs, void *data)
+{
+	struct irq_fwspec parent_fwspec, *fwspec = data;
+	struct meson_data *md = d->host_data;
+	irq_hw_number_t hwirq;
+	int ret, slot;
+
+	slot = meson_alloc_irq_slot(md, virq);
+	if (slot < 0)
+		return slot;
+
+	hwirq = fwspec->param[0];
+	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
+
+	parent_fwspec.fwnode = d->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0] = 0; /* SPI */
+	parent_fwspec.param[1] = md->slots[slot].irq;
+	parent_fwspec.param[2] = IRQ_TYPE_NONE;
+
+	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
+	if (ret)
+		meson_free_irq_slot(md, virq);
+
+	return ret;
+}
+
+static void meson_irq_free(struct irq_domain *d, unsigned int virq,
+			   unsigned int nr_irqs)
+{
+	struct meson_data *md = d->host_data;
+
+	irq_domain_free_irqs_common(d, virq, nr_irqs);
+	meson_free_irq_slot(md, virq);
+}
+
+static const struct irq_domain_ops meson_irq_ops = {
+	.alloc = meson_irq_alloc,
+	.free = meson_irq_free,
+	.xlate = irq_domain_xlate_twocell,
+};
+
+static int meson_get_irqs(struct meson_data *md, struct device_node *node)
+{
+	int ret, i;
+	u32 irq;
+
+	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
+		ret = of_property_read_u32_index(node, "interrupts", i, &irq);
+		if (ret)
+			break;
+		md->slots[i].irq = irq;
+	}
+
+	md->num_slots = i;
+
+	return i ? 0 : -EINVAL;
+}
+
+static const struct regmap_config meson_regmap_config = {
+	.reg_bits       = 32,
+	.reg_stride     = 4,
+	.val_bits       = 32,
+	.max_register	= REG_FILTER_SEL,
+};
+
+static int __init meson_gpio_irq_init(struct device_node *node,
+				      struct device_node *parent)
+{
+	struct irq_domain *meson_irq_domain, *parent_domain;
+	struct meson_data *md;
+	void __iomem *io_base;
+	int ret;
+
+	md = kzalloc(sizeof(*md), GFP_KERNEL);
+	if (!md)
+		return -ENOMEM;
+
+	mutex_init(&md->lock);
+
+	io_base = of_iomap(node, 0);
+	if (!io_base)
+		return -EINVAL;
+
+	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
+	if (IS_ERR(md->regmap))
+		return PTR_ERR(md->regmap);
+
+	/* initialize to IRQ_TYPE_LEVEL_HIGH */
+	regmap_write(md->regmap, REG_EDGE_POL, 0);
+	/* disable all GPIO interrupt sources */
+	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
+	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
+	/* disable filtering */
+	regmap_write(md->regmap, REG_FILTER_SEL, 0);
+
+	ret = meson_get_irqs(md, node);
+	if (ret)
+		return ret;
+
+	parent_domain = irq_find_host(parent);
+	if (!parent_domain)
+		return -ENXIO;
+
+	meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
+						    NUM_GPIO_HWIRQ, node,
+						    &meson_irq_ops, md);
+	return meson_irq_domain ? 0 : -EINVAL;
+}
+
+IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc", meson_gpio_irq_init);
-- 
2.13.1

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Jerome Brunet]

On Sat, 2017-06-10 at 23:57 +0200, Heiner Kallweit wrote:
> Add a driver supporting the GPIO interrupt controller on certain
> Amlogic meson SoC's.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
> v5:
> - changed Kconfig entry based on Neil's suggestion
> - added authors
> - extended explanation why 2 * n hwirqs are used
> v6:
> - change DT property parent-interrupts to interrupts
> v7:
> - no changes
> ---
>  drivers/irqchip/Kconfig          |   5 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-meson-gpio.c | 295
> +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 301 insertions(+)
>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 478f8ace..bdc86e14 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>  	help
>  	  Say yes here to add support for the IRQ combiner devices embedded
>  	  in Qualcomm Technologies chips.
> +
> +config MESON_GPIO_INTC
> +	bool
> +	depends on ARCH_MESON
> +	default y
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b64c59b8..1be482bd 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
> --- /dev/null
> +++ b/drivers/irqchip/irq-meson-gpio.c
> @@ -0,0 +1,295 @@
> +/*
> + * Amlogic Meson GPIO IRQ chip driver
> + *
> + * Copyright (c) 2015 Endless Mobile, Inc.
> + * Author: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
> + * Copyright (c) 2016 BayLibre, SAS.
> + * Author: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +
> +#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 MAX_PARENT_IRQ_NUM	8
> +
> +/* maximum number of GPIO IRQs on supported platforms */
> +#define MAX_NUM_GPIO_IRQ	133

Comment on V6 still applies here. 133 being the max on gxbb does not make it the
max on the other supported chipset. 

Review from october suggested to put this value in DT.

> +
> +/*
> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
> + * edge. That's due to HW constraints.
> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
> + * one bit to the right.
> + */
> +#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)

????

> +
> +struct meson_irq_slot {
> +	unsigned int irq;
> +	unsigned int owner;
> +};
> +
> +struct meson_data {
> +	struct regmap *regmap;
> +	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
> +	unsigned int num_slots;
> +	struct mutex lock;
> +};
> +
> +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
> +{
> +	int i, slot = -ENOSPC;
> +
> +	mutex_lock(&md->lock);
> +
> +	for (i = 0; i < md->num_slots && slot < 0; i++)
> +		if (!md->slots[i].owner) {
> +			md->slots[i].owner = virq;
> +			slot = i;
> +		}
> +
> +	mutex_unlock(&md->lock);
> +
> +	return slot;
> +}
> +
> +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
> +{
> +	int i;
> +
> +	mutex_lock(&md->lock);
> +
> +	for (i = 0; i < md->num_slots; i++)
> +		if (md->slots[i].owner == virq) {
> +			md->slots[i].owner = 0;
> +			break;
> +		}
> +
> +	mutex_unlock(&md->lock);
> +}
> +
> +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
> +{
> +	int i, slot = -EINVAL;
> +
> +	mutex_lock(&md->lock);
> +
> +	for (i = 0; i < md->num_slots && slot < 0; i++)
> +		if (md->slots[i].owner == virq)
> +			slot = i;
> +
> +	mutex_unlock(&md->lock);
> +
> +	return slot;
> +}
> +
> +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int
> hwirq)
> +{
> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
> +	int shift = 8 * (idx % 4);
> +
> +	regmap_update_bits(md->regmap, reg, 0xff << shift,
> +			   hwirq << shift);
> +}
> +
> +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
> +{
> +	struct meson_data *md = data->domain->host_data;
> +	int slot = meson_find_irq_slot(md, data->irq);
> +
> +	if (slot >= 0)
> +		meson_set_hwirq(md, slot, hwirq);
> +}
> +
> +static int meson_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> +	struct meson_data *md = data->domain->host_data;
> +	int slot;
> +	unsigned int val = 0;
> +
> +	if (type == IRQ_TYPE_EDGE_BOTH)
> +		return -EINVAL;
> +
> +	slot = meson_find_irq_slot(md, data->irq);
> +	if (slot < 0)
> +		return slot;
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		val |= REG_EDGE_POL_EDGE(slot);
> +
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
> +		val |= REG_EDGE_POL_LOW(slot);
> +
> +	regmap_update_bits(md->regmap, REG_EDGE_POL,
> +			   REG_EDGE_POL_MASK(slot), val);
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		val = IRQ_TYPE_EDGE_RISING;
> +	else
> +		val = IRQ_TYPE_LEVEL_HIGH;
> +
> +	return irq_chip_set_type_parent(data, val);
> +}
> +
> +static unsigned int meson_irq_startup(struct irq_data *data)
> +{
> +	irq_chip_unmask_parent(data);
> +	/*
> +	 * An extra bit was added to allow having the same gpio hwirq twice
> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
> +	 * gpio hwirq.
> +	 */
> +	meson_irq_set_hwirq(data, data->hwirq >> 1);

Comments from v6 still applies here, and this has nothing to do with the so-
called percentage of niceness - whatever this means.

This is an interrupt controller, a critical part of the system. It has be
correct.

Multiplying the number of IRQ lines and so changing the interface is not
correct.
DT is OS independent, property should not be os-specific, let alone driver-
hacked specific.

Interfaces, including DT, are important. These are supposed to be stable. We
don't to change the irq numbers in every dts because "yeahhh we found another
way!"

So yes, the corner cases should be covered, what drives you is not the matter
here. If you have a critical need for this, then feel free to apply the patch to
your kernel, no one is stopping you. If it goes upstream, people should be able
to understand it and trust it, identified corner cases should be covered.

> +
> +	return 0;
> +}
> +
> +static void meson_irq_shutdown(struct irq_data *data)
> +{
> +	meson_irq_set_hwirq(data, 0xff);
> +	irq_chip_mask_parent(data);
> +}
> +
> +static struct irq_chip meson_irq_chip = {
> +	.name = "meson_gpio_intc",
> +	.irq_set_type = meson_irq_set_type,
> +	.irq_eoi = irq_chip_eoi_parent,
> +	.irq_mask = irq_chip_mask_parent,
> +	.irq_unmask = irq_chip_unmask_parent,
> +	.irq_startup = meson_irq_startup,
> +	.irq_shutdown = meson_irq_shutdown,
> +	.irq_set_affinity = irq_chip_set_affinity_parent,
> +};
> +
> +static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
> +			   unsigned int nr_irqs, void *data)
> +{
> +	struct irq_fwspec parent_fwspec, *fwspec = data;
> +	struct meson_data *md = d->host_data;
> +	irq_hw_number_t hwirq;
> +	int ret, slot;
> +
> +	slot = meson_alloc_irq_slot(md, virq);
> +	if (slot < 0)
> +		return slot;
> +
> +	hwirq = fwspec->param[0];
> +	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
> +
> +	parent_fwspec.fwnode = d->parent->fwnode;
> +	parent_fwspec.param_count = 3;
> +	parent_fwspec.param[0] = 0; /* SPI */
> +	parent_fwspec.param[1] = md->slots[slot].irq;
> +	parent_fwspec.param[2] = IRQ_TYPE_NONE;
> +
> +	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
> +	if (ret)
> +		meson_free_irq_slot(md, virq);
> +
> +	return ret;
> +}
> +
> +static void meson_irq_free(struct irq_domain *d, unsigned int virq,
> +			   unsigned int nr_irqs)
> +{
> +	struct meson_data *md = d->host_data;
> +
> +	irq_domain_free_irqs_common(d, virq, nr_irqs);
> +	meson_free_irq_slot(md, virq);
> +}
> +
> +static const struct irq_domain_ops meson_irq_ops = {
> +	.alloc = meson_irq_alloc,
> +	.free = meson_irq_free,
> +	.xlate = irq_domain_xlate_twocell,
> +};
> +
> +static int meson_get_irqs(struct meson_data *md, struct device_node *node)
> +{
> +	int ret, i;
> +	u32 irq;
> +
> +	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
> +		ret = of_property_read_u32_index(node, "interrupts", i,
> &irq);
> +		if (ret)
> +			break;
> +		md->slots[i].irq = irq;
> +	}
> +
> +	md->num_slots = i;
> +
> +	return i ? 0 : -EINVAL;
> +}
> +
> +static const struct regmap_config meson_regmap_config = {
> +	.reg_bits       = 32,
> +	.reg_stride     = 4,
> +	.val_bits       = 32,
> +	.max_register	= REG_FILTER_SEL,
> +};
> +
> +static int __init meson_gpio_irq_init(struct device_node *node,
> +				      struct device_node *parent)
> +{
> +	struct irq_domain *meson_irq_domain, *parent_domain;
> +	struct meson_data *md;
> +	void __iomem *io_base;
> +	int ret;
> +
> +	md = kzalloc(sizeof(*md), GFP_KERNEL);
> +	if (!md)
> +		return -ENOMEM;
> +
> +	mutex_init(&md->lock);
> +
> +	io_base = of_iomap(node, 0);
> +	if (!io_base)
> +		return -EINVAL;
> +
> +	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
> +	if (IS_ERR(md->regmap))
> +		return PTR_ERR(md->regmap);
> +
> +	/* initialize to IRQ_TYPE_LEVEL_HIGH */
> +	regmap_write(md->regmap, REG_EDGE_POL, 0);
> +	/* disable all GPIO interrupt sources */
> +	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
> +	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
> +	/* disable filtering */
> +	regmap_write(md->regmap, REG_FILTER_SEL, 0);
> +
> +	ret = meson_get_irqs(md, node);
> +	if (ret)
> +		return ret;
> +
> +	parent_domain = irq_find_host(parent);
> +	if (!parent_domain)
> +		return -ENXIO;
> +
> +	meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
> +						    NUM_GPIO_HWIRQ, node,
> +						    &meson_irq_ops, md);
> +	return meson_irq_domain ? 0 : -EINVAL;
> +}
> +
> +IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc",
> meson_gpio_irq_init);

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Heiner Kallweit]

Am 12.06.2017 um 10:54 schrieb Jerome Brunet:
> On Sat, 2017-06-10 at 23:57 +0200, Heiner Kallweit wrote:
>> Add a driver supporting the GPIO interrupt controller on certain
>> Amlogic meson SoC's.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> v5:
>> - changed Kconfig entry based on Neil's suggestion
>> - added authors
>> - extended explanation why 2 * n hwirqs are used
>> v6:
>> - change DT property parent-interrupts to interrupts
>> v7:
>> - no changes
>> ---
>>  drivers/irqchip/Kconfig          |   5 +
>>  drivers/irqchip/Makefile         |   1 +
>>  drivers/irqchip/irq-meson-gpio.c | 295
>> +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 301 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 478f8ace..bdc86e14 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>>  	help
>>  	  Say yes here to add support for the IRQ combiner devices embedded
>>  	  in Qualcomm Technologies chips.
>> +
>> +config MESON_GPIO_INTC
>> +	bool
>> +	depends on ARCH_MESON
>> +	default y
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b64c59b8..1be482bd 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-meson-gpio.c
>> @@ -0,0 +1,295 @@
>> +/*
>> + * Amlogic Meson GPIO IRQ chip driver
>> + *
>> + * Copyright (c) 2015 Endless Mobile, Inc.
>> + * Author: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation, version 2.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/regmap.h>
>> +
>> +#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 MAX_PARENT_IRQ_NUM	8
>> +
>> +/* maximum number of GPIO IRQs on supported platforms */
>> +#define MAX_NUM_GPIO_IRQ	133
> 
> Comment on V6 still applies here. 133 being the max on gxbb does not make it the
> max on the other supported chipset. 
> 
133 is the highest number occurring on any supported chip.

> Review from october suggested to put this value in DT.
> 
I checked few irqchip drivers and none of them gets the number of hwirqs
from DT. Most of the time the number of hwirq's is a #define in the driver.
Also I don't see which benefit this would provide.

This value is used only to determine the size of the irq mapping array
in the irq domain. By using chip-specific values we could save a few bytes
in this array. But I think that's not worth the effort of introducing an extra
dt parameter.
And using a hwirq greater than the max on the specific chips (if any driver
should ever try this) results in: nothing.

Last but not least the dt binding was acked by Rob and if the number of
hwirq's should be in the DT I think he would have mentioned this.

>> +
>> +/*
>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
>> + * edge. That's due to HW constraints.
>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
>> + * one bit to the right.
>> + */
>> +#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
> 
> ????
> 
>> +
>> +struct meson_irq_slot {
>> +	unsigned int irq;
>> +	unsigned int owner;
>> +};
>> +
>> +struct meson_data {
>> +	struct regmap *regmap;
>> +	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
>> +	unsigned int num_slots;
>> +	struct mutex lock;
>> +};
>> +
>> +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
>> +{
>> +	int i, slot = -ENOSPC;
>> +
>> +	mutex_lock(&md->lock);
>> +
>> +	for (i = 0; i < md->num_slots && slot < 0; i++)
>> +		if (!md->slots[i].owner) {
>> +			md->slots[i].owner = virq;
>> +			slot = i;
>> +		}
>> +
>> +	mutex_unlock(&md->lock);
>> +
>> +	return slot;
>> +}
>> +
>> +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
>> +{
>> +	int i;
>> +
>> +	mutex_lock(&md->lock);
>> +
>> +	for (i = 0; i < md->num_slots; i++)
>> +		if (md->slots[i].owner == virq) {
>> +			md->slots[i].owner = 0;
>> +			break;
>> +		}
>> +
>> +	mutex_unlock(&md->lock);
>> +}
>> +
>> +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
>> +{
>> +	int i, slot = -EINVAL;
>> +
>> +	mutex_lock(&md->lock);
>> +
>> +	for (i = 0; i < md->num_slots && slot < 0; i++)
>> +		if (md->slots[i].owner == virq)
>> +			slot = i;
>> +
>> +	mutex_unlock(&md->lock);
>> +
>> +	return slot;
>> +}
>> +
>> +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int
>> hwirq)
>> +{
>> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
>> +	int shift = 8 * (idx % 4);
>> +
>> +	regmap_update_bits(md->regmap, reg, 0xff << shift,
>> +			   hwirq << shift);
>> +}
>> +
>> +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
>> +{
>> +	struct meson_data *md = data->domain->host_data;
>> +	int slot = meson_find_irq_slot(md, data->irq);
>> +
>> +	if (slot >= 0)
>> +		meson_set_hwirq(md, slot, hwirq);
>> +}
>> +
>> +static int meson_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +	struct meson_data *md = data->domain->host_data;
>> +	int slot;
>> +	unsigned int val = 0;
>> +
>> +	if (type == IRQ_TYPE_EDGE_BOTH)
>> +		return -EINVAL;
>> +
>> +	slot = meson_find_irq_slot(md, data->irq);
>> +	if (slot < 0)
>> +		return slot;
>> +
>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>> +		val |= REG_EDGE_POL_EDGE(slot);
>> +
>> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
>> +		val |= REG_EDGE_POL_LOW(slot);
>> +
>> +	regmap_update_bits(md->regmap, REG_EDGE_POL,
>> +			   REG_EDGE_POL_MASK(slot), val);
>> +
>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>> +		val = IRQ_TYPE_EDGE_RISING;
>> +	else
>> +		val = IRQ_TYPE_LEVEL_HIGH;
>> +
>> +	return irq_chip_set_type_parent(data, val);
>> +}
>> +
>> +static unsigned int meson_irq_startup(struct irq_data *data)
>> +{
>> +	irq_chip_unmask_parent(data);
>> +	/*
>> +	 * An extra bit was added to allow having the same gpio hwirq twice
>> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
>> +	 * gpio hwirq.
>> +	 */
>> +	meson_irq_set_hwirq(data, data->hwirq >> 1);
> 
> Comments from v6 still applies here, and this has nothing to do with the so-
> called percentage of niceness - whatever this means.
> 
> This is an interrupt controller, a critical part of the system. It has be
> correct.
> 
> Multiplying the number of IRQ lines and so changing the interface is not
> correct.

Why changing the interface? Using gpio_hwirq << 1 as irq domain hwirq
is the interface. A hwirq in a irq domain is a number, not more and not less.
AFAIK there's no pre-defined semantics.
Of course it should be clear what the number means and the interface should
not change later.

> DT is OS independent, property should not be os-specific, let alone driver-
> hacked specific.
> 
> Interfaces, including DT, are important. These are supposed to be stable. We
> don't to change the irq numbers in every dts because "yeahhh we found another
> way!"
> 
Which DT property are you referring to? There is no irq number in the DT.

> So yes, the corner cases should be covered, what drives you is not the matter
> here. If you have a critical need for this, then feel free to apply the patch to
> your kernel, no one is stopping you. If it goes upstream, people should be able
> to understand it and trust it, identified corner cases should be covered.
> 
>> +
>> +	return 0;
>> +}
>> +
>> +static void meson_irq_shutdown(struct irq_data *data)
>> +{
>> +	meson_irq_set_hwirq(data, 0xff);
>> +	irq_chip_mask_parent(data);
>> +}
>> +
>> +static struct irq_chip meson_irq_chip = {
>> +	.name = "meson_gpio_intc",
>> +	.irq_set_type = meson_irq_set_type,
>> +	.irq_eoi = irq_chip_eoi_parent,
>> +	.irq_mask = irq_chip_mask_parent,
>> +	.irq_unmask = irq_chip_unmask_parent,
>> +	.irq_startup = meson_irq_startup,
>> +	.irq_shutdown = meson_irq_shutdown,
>> +	.irq_set_affinity = irq_chip_set_affinity_parent,
>> +};
>> +
>> +static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
>> +			   unsigned int nr_irqs, void *data)
>> +{
>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
>> +	struct meson_data *md = d->host_data;
>> +	irq_hw_number_t hwirq;
>> +	int ret, slot;
>> +
>> +	slot = meson_alloc_irq_slot(md, virq);
>> +	if (slot < 0)
>> +		return slot;
>> +
>> +	hwirq = fwspec->param[0];
>> +	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
>> +
>> +	parent_fwspec.fwnode = d->parent->fwnode;
>> +	parent_fwspec.param_count = 3;
>> +	parent_fwspec.param[0] = 0; /* SPI */
>> +	parent_fwspec.param[1] = md->slots[slot].irq;
>> +	parent_fwspec.param[2] = IRQ_TYPE_NONE;
>> +
>> +	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
>> +	if (ret)
>> +		meson_free_irq_slot(md, virq);
>> +
>> +	return ret;
>> +}
>> +
>> +static void meson_irq_free(struct irq_domain *d, unsigned int virq,
>> +			   unsigned int nr_irqs)
>> +{
>> +	struct meson_data *md = d->host_data;
>> +
>> +	irq_domain_free_irqs_common(d, virq, nr_irqs);
>> +	meson_free_irq_slot(md, virq);
>> +}
>> +
>> +static const struct irq_domain_ops meson_irq_ops = {
>> +	.alloc = meson_irq_alloc,
>> +	.free = meson_irq_free,
>> +	.xlate = irq_domain_xlate_twocell,
>> +};
>> +
>> +static int meson_get_irqs(struct meson_data *md, struct device_node *node)
>> +{
>> +	int ret, i;
>> +	u32 irq;
>> +
>> +	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
>> +		ret = of_property_read_u32_index(node, "interrupts", i,
>> &irq);
>> +		if (ret)
>> +			break;
>> +		md->slots[i].irq = irq;
>> +	}
>> +
>> +	md->num_slots = i;
>> +
>> +	return i ? 0 : -EINVAL;
>> +}
>> +
>> +static const struct regmap_config meson_regmap_config = {
>> +	.reg_bits       = 32,
>> +	.reg_stride     = 4,
>> +	.val_bits       = 32,
>> +	.max_register	= REG_FILTER_SEL,
>> +};
>> +
>> +static int __init meson_gpio_irq_init(struct device_node *node,
>> +				      struct device_node *parent)
>> +{
>> +	struct irq_domain *meson_irq_domain, *parent_domain;
>> +	struct meson_data *md;
>> +	void __iomem *io_base;
>> +	int ret;
>> +
>> +	md = kzalloc(sizeof(*md), GFP_KERNEL);
>> +	if (!md)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&md->lock);
>> +
>> +	io_base = of_iomap(node, 0);
>> +	if (!io_base)
>> +		return -EINVAL;
>> +
>> +	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
>> +	if (IS_ERR(md->regmap))
>> +		return PTR_ERR(md->regmap);
>> +
>> +	/* initialize to IRQ_TYPE_LEVEL_HIGH */
>> +	regmap_write(md->regmap, REG_EDGE_POL, 0);
>> +	/* disable all GPIO interrupt sources */
>> +	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
>> +	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
>> +	/* disable filtering */
>> +	regmap_write(md->regmap, REG_FILTER_SEL, 0);
>> +
>> +	ret = meson_get_irqs(md, node);
>> +	if (ret)
>> +		return ret;
>> +
>> +	parent_domain = irq_find_host(parent);
>> +	if (!parent_domain)
>> +		return -ENXIO;
>> +
>> +	meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
>> +						    NUM_GPIO_HWIRQ, node,
>> +						    &meson_irq_ops, md);
>> +	return meson_irq_domain ? 0 : -EINVAL;
>> +}
>> +
>> +IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc",
>> meson_gpio_irq_init);
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Marc Zyngier]

On 10/06/17 22:57, Heiner Kallweit wrote:
> Add a driver supporting the GPIO interrupt controller on certain
> Amlogic meson SoC's.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v5:
> - changed Kconfig entry based on Neil's suggestion
> - added authors
> - extended explanation why 2 * n hwirqs are used
> v6:
> - change DT property parent-interrupts to interrupts
> v7:
> - no changes
> ---
>  drivers/irqchip/Kconfig          |   5 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 301 insertions(+)
>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 478f8ace..bdc86e14 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>  	help
>  	  Say yes here to add support for the IRQ combiner devices embedded
>  	  in Qualcomm Technologies chips.
> +
> +config MESON_GPIO_INTC
> +	bool
> +	depends on ARCH_MESON
> +	default y
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index b64c59b8..1be482bd 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
> --- /dev/null
> +++ b/drivers/irqchip/irq-meson-gpio.c
> @@ -0,0 +1,295 @@
> +/*
> + * Amlogic Meson GPIO IRQ chip driver
> + *
> + * Copyright (c) 2015 Endless Mobile, Inc.
> + * Author: Carlo Caione <carlo@endlessm.com>
> + * Copyright (c) 2016 BayLibre, SAS.
> + * Author: Jerome Brunet <jbrunet@baylibre.com>
> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation, version 2.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqchip.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/regmap.h>
> +
> +#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 MAX_PARENT_IRQ_NUM	8
> +
> +/* maximum number of GPIO IRQs on supported platforms */
> +#define MAX_NUM_GPIO_IRQ	133

Why aren't these values coming from DT? I bet that a future revision of
the same HW will double these. Or at least, you could match it on the
compatible string.

> +
> +/*
> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
> + * edge. That's due to HW constraints.
> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
> + * one bit to the right.

Please expand on how you expect this to work, specially when a random
driver expects a single interrupt.

> + */
> +#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
> +
> +struct meson_irq_slot {
> +	unsigned int irq;
> +	unsigned int owner;
> +};
> +
> +struct meson_data {
> +	struct regmap *regmap;
> +	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
> +	unsigned int num_slots;
> +	struct mutex lock;
> +};
> +
> +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
> +{
> +	int i, slot = -ENOSPC;
> +
> +	mutex_lock(&md->lock);
> +
> +	for (i = 0; i < md->num_slots && slot < 0; i++)
> +		if (!md->slots[i].owner) {
> +			md->slots[i].owner = virq;

Why do you have to deal with the virq? It'd be more logical to deal with
the hwirq. The usual mechanism to reserve a "slot" is to use a bitmap
indexed by the hwirq. Why is that not working for you?

> +			slot = i;
> +		}
> +
> +	mutex_unlock(&md->lock);
> +
> +	return slot;
> +}
> +
> +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
> +{
> +	int i;
> +
> +	mutex_lock(&md->lock);
> +
> +	for (i = 0; i < md->num_slots; i++)
> +		if (md->slots[i].owner == virq) {
> +			md->slots[i].owner = 0;
> +			break;
> +		}
> +
> +	mutex_unlock(&md->lock);
> +}

These two functions are basically the same...

> +
> +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
> +{
> +	int i, slot = -EINVAL;
> +
> +	mutex_lock(&md->lock);
> +
> +	for (i = 0; i < md->num_slots && slot < 0; i++)
> +		if (md->slots[i].owner == virq)
> +			slot = i;
> +
> +	mutex_unlock(&md->lock);
> +
> +	return slot;
> +}

... and could be expressed in terms of this one.

> +
> +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int hwirq)
> +{
> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
> +	int shift = 8 * (idx % 4);

What's this?

> +
> +	regmap_update_bits(md->regmap, reg, 0xff << shift,
> +			   hwirq << shift);
> +}
> +
> +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
> +{
> +	struct meson_data *md = data->domain->host_data;
> +	int slot = meson_find_irq_slot(md, data->irq);
> +
> +	if (slot >= 0)
> +		meson_set_hwirq(md, slot, hwirq);
> +}
> +
> +static int meson_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> +	struct meson_data *md = data->domain->host_data;
> +	int slot;
> +	unsigned int val = 0;
> +
> +	if (type == IRQ_TYPE_EDGE_BOTH)
> +		return -EINVAL;

So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?

> +
> +	slot = meson_find_irq_slot(md, data->irq);
> +	if (slot < 0)
> +		return slot;

How can this happen?

> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		val |= REG_EDGE_POL_EDGE(slot);
> +
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
> +		val |= REG_EDGE_POL_LOW(slot);
> +
> +	regmap_update_bits(md->regmap, REG_EDGE_POL,
> +			   REG_EDGE_POL_MASK(slot), val);
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		val = IRQ_TYPE_EDGE_RISING;
> +	else
> +		val = IRQ_TYPE_LEVEL_HIGH;

How does this work? Does this HW have some magic falling->rising and
low->high conversion feature? If it doesn't, I cannot see how this can work.

> +
> +	return irq_chip_set_type_parent(data, val);
> +}
> +
> +static unsigned int meson_irq_startup(struct irq_data *data)
> +{
> +	irq_chip_unmask_parent(data);
> +	/*
> +	 * An extra bit was added to allow having the same gpio hwirq twice
> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
> +	 * gpio hwirq.
> +	 */
> +	meson_irq_set_hwirq(data, data->hwirq >> 1);

Again. Do you support EDGE_BOTH or not?

> +
> +	return 0;
> +}
> +
> +static void meson_irq_shutdown(struct irq_data *data)
> +{
> +	meson_irq_set_hwirq(data, 0xff);

What's special about 0xff?

> +	irq_chip_mask_parent(data);
> +}
> +
> +static struct irq_chip meson_irq_chip = {
> +	.name = "meson_gpio_intc",
> +	.irq_set_type = meson_irq_set_type,
> +	.irq_eoi = irq_chip_eoi_parent,
> +	.irq_mask = irq_chip_mask_parent,
> +	.irq_unmask = irq_chip_unmask_parent,
> +	.irq_startup = meson_irq_startup,
> +	.irq_shutdown = meson_irq_shutdown,
> +	.irq_set_affinity = irq_chip_set_affinity_parent,
> +};
> +
> +static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
> +			   unsigned int nr_irqs, void *data)
> +{
> +	struct irq_fwspec parent_fwspec, *fwspec = data;
> +	struct meson_data *md = d->host_data;
> +	irq_hw_number_t hwirq;
> +	int ret, slot;
> +
> +	slot = meson_alloc_irq_slot(md, virq);
> +	if (slot < 0)
> +		return slot;
> +
> +	hwirq = fwspec->param[0];
> +	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
> +
> +	parent_fwspec.fwnode = d->parent->fwnode;
> +	parent_fwspec.param_count = 3;
> +	parent_fwspec.param[0] = 0; /* SPI */
> +	parent_fwspec.param[1] = md->slots[slot].irq;
> +	parent_fwspec.param[2] = IRQ_TYPE_NONE;

Hell no. Look at the GIC DT binding: there is no NONE. It is either
HIGH, or RISING.

> +
> +	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
> +	if (ret)
> +		meson_free_irq_slot(md, virq);
> +
> +	return ret;
> +}
> +
> +static void meson_irq_free(struct irq_domain *d, unsigned int virq,
> +			   unsigned int nr_irqs)
> +{
> +	struct meson_data *md = d->host_data;
> +
> +	irq_domain_free_irqs_common(d, virq, nr_irqs);
> +	meson_free_irq_slot(md, virq);
> +}
> +
> +static const struct irq_domain_ops meson_irq_ops = {
> +	.alloc = meson_irq_alloc,
> +	.free = meson_irq_free,
> +	.xlate = irq_domain_xlate_twocell,
> +};
> +
> +static int meson_get_irqs(struct meson_data *md, struct device_node *node)
> +{
> +	int ret, i;
> +	u32 irq;
> +
> +	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
> +		ret = of_property_read_u32_index(node, "interrupts", i, &irq);
> +		if (ret)
> +			break;
> +		md->slots[i].irq = irq;
> +	}
> +
> +	md->num_slots = i;
> +
> +	return i ? 0 : -EINVAL;
> +}
> +
> +static const struct regmap_config meson_regmap_config = {
> +	.reg_bits       = 32,
> +	.reg_stride     = 4,
> +	.val_bits       = 32,
> +	.max_register	= REG_FILTER_SEL,
> +};
> +
> +static int __init meson_gpio_irq_init(struct device_node *node,
> +				      struct device_node *parent)
> +{
> +	struct irq_domain *meson_irq_domain, *parent_domain;
> +	struct meson_data *md;
> +	void __iomem *io_base;
> +	int ret;
> +
> +	md = kzalloc(sizeof(*md), GFP_KERNEL);
> +	if (!md)
> +		return -ENOMEM;
> +
> +	mutex_init(&md->lock);
> +
> +	io_base = of_iomap(node, 0);
> +	if (!io_base)
> +		return -EINVAL;
> +
> +	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
> +	if (IS_ERR(md->regmap))
> +		return PTR_ERR(md->regmap);
> +
> +	/* initialize to IRQ_TYPE_LEVEL_HIGH */
> +	regmap_write(md->regmap, REG_EDGE_POL, 0);
> +	/* disable all GPIO interrupt sources */
> +	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
> +	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
> +	/* disable filtering */
> +	regmap_write(md->regmap, REG_FILTER_SEL, 0);
> +
> +	ret = meson_get_irqs(md, node);
> +	if (ret)
> +		return ret;
> +
> +	parent_domain = irq_find_host(parent);
> +	if (!parent_domain)
> +		return -ENXIO;

Memory leak on all the return paths.

> +
> +	meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
> +						    NUM_GPIO_HWIRQ, node,
> +						    &meson_irq_ops, md);
> +	return meson_irq_domain ? 0 : -EINVAL;
> +}
> +
> +IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc", meson_gpio_irq_init);
> 

Thanks,

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

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Heiner Kallweit]

Am 13.06.2017 um 10:31 schrieb Marc Zyngier:
> On 10/06/17 22:57, Heiner Kallweit wrote:
>> Add a driver supporting the GPIO interrupt controller on certain
>> Amlogic meson SoC's.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>> v5:
>> - changed Kconfig entry based on Neil's suggestion
>> - added authors
>> - extended explanation why 2 * n hwirqs are used
>> v6:
>> - change DT property parent-interrupts to interrupts
>> v7:
>> - no changes
>> ---
>>  drivers/irqchip/Kconfig          |   5 +
>>  drivers/irqchip/Makefile         |   1 +
>>  drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 301 insertions(+)
>>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
>>
>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>> index 478f8ace..bdc86e14 100644
>> --- a/drivers/irqchip/Kconfig
>> +++ b/drivers/irqchip/Kconfig
>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>>  	help
>>  	  Say yes here to add support for the IRQ combiner devices embedded
>>  	  in Qualcomm Technologies chips.
>> +
>> +config MESON_GPIO_INTC
>> +	bool
>> +	depends on ARCH_MESON
>> +	default y
>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>> index b64c59b8..1be482bd 100644
>> --- a/drivers/irqchip/Makefile
>> +++ b/drivers/irqchip/Makefile
>> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
>> --- /dev/null
>> +++ b/drivers/irqchip/irq-meson-gpio.c
>> @@ -0,0 +1,295 @@
>> +/*
>> + * Amlogic Meson GPIO IRQ chip driver
>> + *
>> + * Copyright (c) 2015 Endless Mobile, Inc.
>> + * Author: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>> + * Copyright (c) 2016 BayLibre, SAS.
>> + * Author: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation, version 2.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irqchip.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/regmap.h>
>> +
>> +#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 MAX_PARENT_IRQ_NUM	8
>> +
>> +/* maximum number of GPIO IRQs on supported platforms */
>> +#define MAX_NUM_GPIO_IRQ	133
> 
> Why aren't these values coming from DT? I bet that a future revision of
> the same HW will double these. Or at least, you could match it on the
> compatible string.
> 
Alternatively this value can be set to 255. The GPIO source is an 8 bit
value with 255 being reserved for "no interrupt source assigned".
This way we cover all chips based on the same IP.
I think what we could gain by introducing an additional DT property
(saving a few bytes in the irqdomain mapping table) isn't worth the effort.

>> +
>> +/*
>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
>> + * edge. That's due to HW constraints.
>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
>> + * one bit to the right.
> 
> Please expand on how you expect this to work, specially when a random
> driver expects a single interrupt.
> 
The gpio interrupt controller in this chip doesn't have native support for
IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio
to two parent interrupts, one for each edge.
There's still no solution to achieve this in a way everybody is happy with.
Therefore this feature isn't part of this patch set.

However, to be prepared to include this feature later, the interface
between pinctrl/gpio and irqchip driver should (IMHO) cater for it already.
Else we may have to touch the irqchip driver later and change the interface
what I would like to avoid.

If a driver just needs one (parent) interrupt, it can request hwirq
(2 * GPIO_HWIRQ) only. There's no issue with that.

>> + */
>> +#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
>> +
>> +struct meson_irq_slot {
>> +	unsigned int irq;
>> +	unsigned int owner;
>> +};
>> +
>> +struct meson_data {
>> +	struct regmap *regmap;
>> +	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
>> +	unsigned int num_slots;
>> +	struct mutex lock;
>> +};
>> +
>> +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
>> +{
>> +	int i, slot = -ENOSPC;
>> +
>> +	mutex_lock(&md->lock);
>> +
>> +	for (i = 0; i < md->num_slots && slot < 0; i++)
>> +		if (!md->slots[i].owner) {
>> +			md->slots[i].owner = virq;
> 
> Why do you have to deal with the virq? It'd be more logical to deal with
> the hwirq. The usual mechanism to reserve a "slot" is to use a bitmap
> indexed by the hwirq. Why is that not working for you?
> 
Using the hwirq as owner should also be possible. Will consider this.

A slot has two members, the owner and a the associated parent irq number.
Of course we could split this into a slot bitmap + an array with parent
irq's indexed by slot number. Would you prefer this?

>> +			slot = i;
>> +		}
>> +
>> +	mutex_unlock(&md->lock);
>> +
>> +	return slot;
>> +}
>> +
>> +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
>> +{
>> +	int i;
>> +
>> +	mutex_lock(&md->lock);
>> +
>> +	for (i = 0; i < md->num_slots; i++)
>> +		if (md->slots[i].owner == virq) {
>> +			md->slots[i].owner = 0;
>> +			break;
>> +		}
>> +
>> +	mutex_unlock(&md->lock);
>> +}
> 
> These two functions are basically the same...
> 
>> +
>> +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
>> +{
>> +	int i, slot = -EINVAL;
>> +
>> +	mutex_lock(&md->lock);
>> +
>> +	for (i = 0; i < md->num_slots && slot < 0; i++)
>> +		if (md->slots[i].owner == virq)
>> +			slot = i;
>> +
>> +	mutex_unlock(&md->lock);
>> +
>> +	return slot;
>> +}
> 
> ... and could be expressed in terms of this one.
> 
>> +
>> +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int hwirq)
>> +{
>> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
>> +	int shift = 8 * (idx % 4);
> 
> What's this?
> 
GPIO source for the eight parent irq's can be configured using two 32-bit registers
with four 8-bit fields each.

>> +
>> +	regmap_update_bits(md->regmap, reg, 0xff << shift,
>> +			   hwirq << shift);
>> +}
>> +
>> +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
>> +{
>> +	struct meson_data *md = data->domain->host_data;
>> +	int slot = meson_find_irq_slot(md, data->irq);
>> +
>> +	if (slot >= 0)
>> +		meson_set_hwirq(md, slot, hwirq);
>> +}
>> +
>> +static int meson_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +	struct meson_data *md = data->domain->host_data;
>> +	int slot;
>> +	unsigned int val = 0;
>> +
>> +	if (type == IRQ_TYPE_EDGE_BOTH)
>> +		return -EINVAL;
> 
> So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?
> 
We reject it in the initial version of the patch set as there's no consensus
yet on some details of the workaround needed for EDGE_BOTH support.

>> +
>> +	slot = meson_find_irq_slot(md, data->irq);
>> +	if (slot < 0)
>> +		return slot;
> 
> How can this happen?
> 
I see no way how this can happen. It was basically added to be on the safe
side and fail nicely in case I miss a scenario which could cause this to fail.
I can remove the check.

>> +
>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>> +		val |= REG_EDGE_POL_EDGE(slot);
>> +
>> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
>> +		val |= REG_EDGE_POL_LOW(slot);
>> +
>> +	regmap_update_bits(md->regmap, REG_EDGE_POL,
>> +			   REG_EDGE_POL_MASK(slot), val);
>> +
>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>> +		val = IRQ_TYPE_EDGE_RISING;
>> +	else
>> +		val = IRQ_TYPE_LEVEL_HIGH;
> 
> How does this work? Does this HW have some magic falling->rising and
> low->high conversion feature? If it doesn't, I cannot see how this can work.
> 
Exactly, HW has a programmable polarity inverter.

>> +
>> +	return irq_chip_set_type_parent(data, val);
>> +}
>> +
>> +static unsigned int meson_irq_startup(struct irq_data *data)
>> +{
>> +	irq_chip_unmask_parent(data);
>> +	/*
>> +	 * An extra bit was added to allow having the same gpio hwirq twice
>> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
>> +	 * gpio hwirq.
>> +	 */
>> +	meson_irq_set_hwirq(data, data->hwirq >> 1);
> 
> Again. Do you support EDGE_BOTH or not?
> 

Not yet ..

>> +
>> +	return 0;
>> +}
>> +
>> +static void meson_irq_shutdown(struct irq_data *data)
>> +{
>> +	meson_irq_set_hwirq(data, 0xff);
> 
> What's special about 0xff?
> 
0xff is the reserved value indicating the no GPIO source is assigned
to the parent irq.

>> +	irq_chip_mask_parent(data);
>> +}
>> +
>> +static struct irq_chip meson_irq_chip = {
>> +	.name = "meson_gpio_intc",
>> +	.irq_set_type = meson_irq_set_type,
>> +	.irq_eoi = irq_chip_eoi_parent,
>> +	.irq_mask = irq_chip_mask_parent,
>> +	.irq_unmask = irq_chip_unmask_parent,
>> +	.irq_startup = meson_irq_startup,
>> +	.irq_shutdown = meson_irq_shutdown,
>> +	.irq_set_affinity = irq_chip_set_affinity_parent,
>> +};
>> +
>> +static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
>> +			   unsigned int nr_irqs, void *data)
>> +{
>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
>> +	struct meson_data *md = d->host_data;
>> +	irq_hw_number_t hwirq;
>> +	int ret, slot;
>> +
>> +	slot = meson_alloc_irq_slot(md, virq);
>> +	if (slot < 0)
>> +		return slot;
>> +
>> +	hwirq = fwspec->param[0];
>> +	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
>> +
>> +	parent_fwspec.fwnode = d->parent->fwnode;
>> +	parent_fwspec.param_count = 3;
>> +	parent_fwspec.param[0] = 0; /* SPI */
>> +	parent_fwspec.param[1] = md->slots[slot].irq;
>> +	parent_fwspec.param[2] = IRQ_TYPE_NONE;
> 
> Hell no. Look at the GIC DT binding: there is no NONE. It is either
> HIGH, or RISING.
> 
OK, will be changed.

>> +
>> +	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
>> +	if (ret)
>> +		meson_free_irq_slot(md, virq);
>> +
>> +	return ret;
>> +}
>> +
>> +static void meson_irq_free(struct irq_domain *d, unsigned int virq,
>> +			   unsigned int nr_irqs)
>> +{
>> +	struct meson_data *md = d->host_data;
>> +
>> +	irq_domain_free_irqs_common(d, virq, nr_irqs);
>> +	meson_free_irq_slot(md, virq);
>> +}
>> +
>> +static const struct irq_domain_ops meson_irq_ops = {
>> +	.alloc = meson_irq_alloc,
>> +	.free = meson_irq_free,
>> +	.xlate = irq_domain_xlate_twocell,
>> +};
>> +
>> +static int meson_get_irqs(struct meson_data *md, struct device_node *node)
>> +{
>> +	int ret, i;
>> +	u32 irq;
>> +
>> +	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
>> +		ret = of_property_read_u32_index(node, "interrupts", i, &irq);
>> +		if (ret)
>> +			break;
>> +		md->slots[i].irq = irq;
>> +	}
>> +
>> +	md->num_slots = i;
>> +
>> +	return i ? 0 : -EINVAL;
>> +}
>> +
>> +static const struct regmap_config meson_regmap_config = {
>> +	.reg_bits       = 32,
>> +	.reg_stride     = 4,
>> +	.val_bits       = 32,
>> +	.max_register	= REG_FILTER_SEL,
>> +};
>> +
>> +static int __init meson_gpio_irq_init(struct device_node *node,
>> +				      struct device_node *parent)
>> +{
>> +	struct irq_domain *meson_irq_domain, *parent_domain;
>> +	struct meson_data *md;
>> +	void __iomem *io_base;
>> +	int ret;
>> +
>> +	md = kzalloc(sizeof(*md), GFP_KERNEL);
>> +	if (!md)
>> +		return -ENOMEM;
>> +
>> +	mutex_init(&md->lock);
>> +
>> +	io_base = of_iomap(node, 0);
>> +	if (!io_base)
>> +		return -EINVAL;
>> +
>> +	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
>> +	if (IS_ERR(md->regmap))
>> +		return PTR_ERR(md->regmap);
>> +
>> +	/* initialize to IRQ_TYPE_LEVEL_HIGH */
>> +	regmap_write(md->regmap, REG_EDGE_POL, 0);
>> +	/* disable all GPIO interrupt sources */
>> +	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
>> +	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
>> +	/* disable filtering */
>> +	regmap_write(md->regmap, REG_FILTER_SEL, 0);
>> +
>> +	ret = meson_get_irqs(md, node);
>> +	if (ret)
>> +		return ret;
>> +
>> +	parent_domain = irq_find_host(parent);
>> +	if (!parent_domain)
>> +		return -ENXIO;
> 
> Memory leak on all the return paths.
> 
Indeed. Most likely copy & paste error when I used other irqchip drivers
as inspiration. E.g. irq-crossbar faces the same issue, it allocates memory
in crossbar_of_init which isn't free'd if irq_domain_add_hierarchy fails.

>> +
>> +	meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0,
>> +						    NUM_GPIO_HWIRQ, node,
>> +						    &meson_irq_ops, md);
>> +	return meson_irq_domain ? 0 : -EINVAL;
>> +}
>> +
>> +IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc", meson_gpio_irq_init);
>>
> 
> Thanks,
> 
> 	M.
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Marc Zyngier]

On 15/06/17 14:10, Heiner Kallweit wrote:
> Am 13.06.2017 um 10:31 schrieb Marc Zyngier:
>> On 10/06/17 22:57, Heiner Kallweit wrote:
>>> Add a driver supporting the GPIO interrupt controller on certain
>>> Amlogic meson SoC's.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> v5:
>>> - changed Kconfig entry based on Neil's suggestion
>>> - added authors
>>> - extended explanation why 2 * n hwirqs are used
>>> v6:
>>> - change DT property parent-interrupts to interrupts
>>> v7:
>>> - no changes
>>> ---
>>>  drivers/irqchip/Kconfig          |   5 +
>>>  drivers/irqchip/Makefile         |   1 +
>>>  drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 301 insertions(+)
>>>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
>>>
>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>> index 478f8ace..bdc86e14 100644
>>> --- a/drivers/irqchip/Kconfig
>>> +++ b/drivers/irqchip/Kconfig
>>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>>>  	help
>>>  	  Say yes here to add support for the IRQ combiner devices embedded
>>>  	  in Qualcomm Technologies chips.
>>> +
>>> +config MESON_GPIO_INTC
>>> +	bool
>>> +	depends on ARCH_MESON
>>> +	default y
>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>> index b64c59b8..1be482bd 100644
>>> --- a/drivers/irqchip/Makefile
>>> +++ b/drivers/irqchip/Makefile
>>> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>>> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
>>> --- /dev/null
>>> +++ b/drivers/irqchip/irq-meson-gpio.c
>>> @@ -0,0 +1,295 @@
>>> +/*
>>> + * Amlogic Meson GPIO IRQ chip driver
>>> + *
>>> + * Copyright (c) 2015 Endless Mobile, Inc.
>>> + * Author: Carlo Caione <carlo@endlessm.com>
>>> + * Copyright (c) 2016 BayLibre, SAS.
>>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1@gmail.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of the GNU General Public License as
>>> + * published by the Free Software Foundation, version 2.
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/init.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/irqchip.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_irq.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/regmap.h>
>>> +
>>> +#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 MAX_PARENT_IRQ_NUM	8
>>> +
>>> +/* maximum number of GPIO IRQs on supported platforms */
>>> +#define MAX_NUM_GPIO_IRQ	133
>>
>> Why aren't these values coming from DT? I bet that a future revision of
>> the same HW will double these. Or at least, you could match it on the
>> compatible string.
>>
> Alternatively this value can be set to 255. The GPIO source is an 8 bit
> value with 255 being reserved for "no interrupt source assigned".

Who is reserving it? The HW? Or is that your own defined convention?

> This way we cover all chips based on the same IP.

Why? Where is that 8bit limit coming from?

> I think what we could gain by introducing an additional DT property
> (saving a few bytes in the irqdomain mapping table) isn't worth the effort.

It is not about saving or wasting memory. It is about making the driver
and its binding able to span multiple generation of the HW without too
much churn. Which is why I'm suggesting that you either define these
properties in DT *or* match the compatible string to obtain these values.

>>> +
>>> +/*
>>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
>>> + * edge. That's due to HW constraints.
>>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
>>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
>>> + * one bit to the right.
>>
>> Please expand on how you expect this to work, specially when a random
>> driver expects a single interrupt.
>>
> The gpio interrupt controller in this chip doesn't have native support for
> IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio
> to two parent interrupts, one for each edge.

No, that's horrible, racy, and impractical. It has been proposed in the
past (for the same HW), and we're not going there again.

> There's still no solution to achieve this in a way everybody is happy with.
> Therefore this feature isn't part of this patch set.
> 
> However, to be prepared to include this feature later, the interface
> between pinctrl/gpio and irqchip driver should (IMHO) cater for it already.
> Else we may have to touch the irqchip driver later and change the interface
> what I would like to avoid.

Don't even think of it, and considered it pre-NAKed.

> 
> If a driver just needs one (parent) interrupt, it can request hwirq
> (2 * GPIO_HWIRQ) only. There's no issue with that.

Other than being utterly useless and confusing?

>>> + */
>>> +#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
>>> +
>>> +struct meson_irq_slot {
>>> +	unsigned int irq;
>>> +	unsigned int owner;
>>> +};
>>> +
>>> +struct meson_data {
>>> +	struct regmap *regmap;
>>> +	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
>>> +	unsigned int num_slots;
>>> +	struct mutex lock;
>>> +};
>>> +
>>> +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
>>> +{
>>> +	int i, slot = -ENOSPC;
>>> +
>>> +	mutex_lock(&md->lock);
>>> +
>>> +	for (i = 0; i < md->num_slots && slot < 0; i++)
>>> +		if (!md->slots[i].owner) {
>>> +			md->slots[i].owner = virq;
>>
>> Why do you have to deal with the virq? It'd be more logical to deal with
>> the hwirq. The usual mechanism to reserve a "slot" is to use a bitmap
>> indexed by the hwirq. Why is that not working for you?
>>
> Using the hwirq as owner should also be possible. Will consider this.
> 
> A slot has two members, the owner and a the associated parent irq number.
> Of course we could split this into a slot bitmap + an array with parent
> irq's indexed by slot number. Would you prefer this?

Again, why do you need to consider the Linux irq number? All you need to
track is whether a hwirq has been allocated or not, since all the
irqchip function are called with an irq_data as a parameter, which
contains both the irq and hwirq values.

>>> +			slot = i;
>>> +		}
>>> +
>>> +	mutex_unlock(&md->lock);
>>> +
>>> +	return slot;
>>> +}
>>> +
>>> +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
>>> +{
>>> +	int i;
>>> +
>>> +	mutex_lock(&md->lock);
>>> +
>>> +	for (i = 0; i < md->num_slots; i++)
>>> +		if (md->slots[i].owner == virq) {
>>> +			md->slots[i].owner = 0;
>>> +			break;
>>> +		}
>>> +
>>> +	mutex_unlock(&md->lock);
>>> +}
>>
>> These two functions are basically the same...
>>
>>> +
>>> +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
>>> +{
>>> +	int i, slot = -EINVAL;
>>> +
>>> +	mutex_lock(&md->lock);
>>> +
>>> +	for (i = 0; i < md->num_slots && slot < 0; i++)
>>> +		if (md->slots[i].owner == virq)
>>> +			slot = i;
>>> +
>>> +	mutex_unlock(&md->lock);
>>> +
>>> +	return slot;
>>> +}
>>
>> ... and could be expressed in terms of this one.
>>
>>> +
>>> +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int hwirq)
>>> +{
>>> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
>>> +	int shift = 8 * (idx % 4);
>>
>> What's this?
>>
> GPIO source for the eight parent irq's can be configured using two 32-bit registers
> with four 8-bit fields each.

Consider moving it to an accessor and document the mapping of the hwirqs
in these registers.

>>> +
>>> +	regmap_update_bits(md->regmap, reg, 0xff << shift,
>>> +			   hwirq << shift);
>>> +}
>>> +
>>> +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
>>> +{
>>> +	struct meson_data *md = data->domain->host_data;
>>> +	int slot = meson_find_irq_slot(md, data->irq);
>>> +
>>> +	if (slot >= 0)
>>> +		meson_set_hwirq(md, slot, hwirq);
>>> +}
>>> +
>>> +static int meson_irq_set_type(struct irq_data *data, unsigned int type)
>>> +{
>>> +	struct meson_data *md = data->domain->host_data;
>>> +	int slot;
>>> +	unsigned int val = 0;
>>> +
>>> +	if (type == IRQ_TYPE_EDGE_BOTH)
>>> +		return -EINVAL;
>>
>> So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?
>>
> We reject it in the initial version of the patch set as there's no consensus
> yet on some details of the workaround needed for EDGE_BOTH support.

There is a consensus: The HW doesn't support this feature.

>>> +
>>> +	slot = meson_find_irq_slot(md, data->irq);
>>> +	if (slot < 0)
>>> +		return slot;
>>
>> How can this happen?
>>
> I see no way how this can happen. It was basically added to be on the safe
> side and fail nicely in case I miss a scenario which could cause this to fail.
> I can remove the check.
> 
>>> +
>>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>>> +		val |= REG_EDGE_POL_EDGE(slot);
>>> +
>>> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
>>> +		val |= REG_EDGE_POL_LOW(slot);
>>> +
>>> +	regmap_update_bits(md->regmap, REG_EDGE_POL,
>>> +			   REG_EDGE_POL_MASK(slot), val);
>>> +
>>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>>> +		val = IRQ_TYPE_EDGE_RISING;
>>> +	else
>>> +		val = IRQ_TYPE_LEVEL_HIGH;
>>
>> How does this work? Does this HW have some magic falling->rising and
>> low->high conversion feature? If it doesn't, I cannot see how this can work.
>>
> Exactly, HW has a programmable polarity inverter.
> 
>>> +
>>> +	return irq_chip_set_type_parent(data, val);
>>> +}
>>> +
>>> +static unsigned int meson_irq_startup(struct irq_data *data)
>>> +{
>>> +	irq_chip_unmask_parent(data);
>>> +	/*
>>> +	 * An extra bit was added to allow having the same gpio hwirq twice
>>> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
>>> +	 * gpio hwirq.
>>> +	 */
>>> +	meson_irq_set_hwirq(data, data->hwirq >> 1);
>>
>> Again. Do you support EDGE_BOTH or not?
>>
> 
> Not yet ..
> 
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void meson_irq_shutdown(struct irq_data *data)
>>> +{
>>> +	meson_irq_set_hwirq(data, 0xff);
>>
>> What's special about 0xff?
>>
> 0xff is the reserved value indicating the no GPIO source is assigned
> to the parent irq.

Then document it, add a #define for this.

>>> +	irq_chip_mask_parent(data);
>>> +}
>>> +
>>> +static struct irq_chip meson_irq_chip = {
>>> +	.name = "meson_gpio_intc",
>>> +	.irq_set_type = meson_irq_set_type,
>>> +	.irq_eoi = irq_chip_eoi_parent,
>>> +	.irq_mask = irq_chip_mask_parent,
>>> +	.irq_unmask = irq_chip_unmask_parent,
>>> +	.irq_startup = meson_irq_startup,
>>> +	.irq_shutdown = meson_irq_shutdown,
>>> +	.irq_set_affinity = irq_chip_set_affinity_parent,
>>> +};
>>> +
>>> +static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
>>> +			   unsigned int nr_irqs, void *data)
>>> +{
>>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
>>> +	struct meson_data *md = d->host_data;
>>> +	irq_hw_number_t hwirq;
>>> +	int ret, slot;
>>> +
>>> +	slot = meson_alloc_irq_slot(md, virq);
>>> +	if (slot < 0)
>>> +		return slot;
>>> +
>>> +	hwirq = fwspec->param[0];
>>> +	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
>>> +
>>> +	parent_fwspec.fwnode = d->parent->fwnode;
>>> +	parent_fwspec.param_count = 3;
>>> +	parent_fwspec.param[0] = 0; /* SPI */
>>> +	parent_fwspec.param[1] = md->slots[slot].irq;
>>> +	parent_fwspec.param[2] = IRQ_TYPE_NONE;
>>
>> Hell no. Look at the GIC DT binding: there is no NONE. It is either
>> HIGH, or RISING.
>>
> OK, will be changed.
> 
>>> +
>>> +	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
>>> +	if (ret)
>>> +		meson_free_irq_slot(md, virq);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static void meson_irq_free(struct irq_domain *d, unsigned int virq,
>>> +			   unsigned int nr_irqs)
>>> +{
>>> +	struct meson_data *md = d->host_data;
>>> +
>>> +	irq_domain_free_irqs_common(d, virq, nr_irqs);
>>> +	meson_free_irq_slot(md, virq);
>>> +}
>>> +
>>> +static const struct irq_domain_ops meson_irq_ops = {
>>> +	.alloc = meson_irq_alloc,
>>> +	.free = meson_irq_free,
>>> +	.xlate = irq_domain_xlate_twocell,
>>> +};
>>> +
>>> +static int meson_get_irqs(struct meson_data *md, struct device_node *node)
>>> +{
>>> +	int ret, i;
>>> +	u32 irq;
>>> +
>>> +	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
>>> +		ret = of_property_read_u32_index(node, "interrupts", i, &irq);
>>> +		if (ret)
>>> +			break;
>>> +		md->slots[i].irq = irq;
>>> +	}
>>> +
>>> +	md->num_slots = i;
>>> +
>>> +	return i ? 0 : -EINVAL;
>>> +}
>>> +
>>> +static const struct regmap_config meson_regmap_config = {
>>> +	.reg_bits       = 32,
>>> +	.reg_stride     = 4,
>>> +	.val_bits       = 32,
>>> +	.max_register	= REG_FILTER_SEL,
>>> +};
>>> +
>>> +static int __init meson_gpio_irq_init(struct device_node *node,
>>> +				      struct device_node *parent)
>>> +{
>>> +	struct irq_domain *meson_irq_domain, *parent_domain;
>>> +	struct meson_data *md;
>>> +	void __iomem *io_base;
>>> +	int ret;
>>> +
>>> +	md = kzalloc(sizeof(*md), GFP_KERNEL);
>>> +	if (!md)
>>> +		return -ENOMEM;
>>> +
>>> +	mutex_init(&md->lock);
>>> +
>>> +	io_base = of_iomap(node, 0);
>>> +	if (!io_base)
>>> +		return -EINVAL;
>>> +
>>> +	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
>>> +	if (IS_ERR(md->regmap))
>>> +		return PTR_ERR(md->regmap);
>>> +
>>> +	/* initialize to IRQ_TYPE_LEVEL_HIGH */
>>> +	regmap_write(md->regmap, REG_EDGE_POL, 0);
>>> +	/* disable all GPIO interrupt sources */
>>> +	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
>>> +	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
>>> +	/* disable filtering */
>>> +	regmap_write(md->regmap, REG_FILTER_SEL, 0);
>>> +
>>> +	ret = meson_get_irqs(md, node);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	parent_domain = irq_find_host(parent);
>>> +	if (!parent_domain)
>>> +		return -ENXIO;
>>
>> Memory leak on all the return paths.
>>
> Indeed. Most likely copy & paste error when I used other irqchip drivers
> as inspiration. E.g. irq-crossbar faces the same issue, it allocates memory
> in crossbar_of_init which isn't free'd if irq_domain_add_hierarchy fails.

Then please submit a patch addressing the defect.

Thanks,

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

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Heiner Kallweit]

Am 15.06.2017 um 15:27 schrieb Marc Zyngier:
> On 15/06/17 14:10, Heiner Kallweit wrote:
>> Am 13.06.2017 um 10:31 schrieb Marc Zyngier:
>>> On 10/06/17 22:57, Heiner Kallweit wrote:
>>>> Add a driver supporting the GPIO interrupt controller on certain
>>>> Amlogic meson SoC's.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> ---
>>>> v5:
>>>> - changed Kconfig entry based on Neil's suggestion
>>>> - added authors
>>>> - extended explanation why 2 * n hwirqs are used
>>>> v6:
>>>> - change DT property parent-interrupts to interrupts
>>>> v7:
>>>> - no changes
>>>> ---
>>>>  drivers/irqchip/Kconfig          |   5 +
>>>>  drivers/irqchip/Makefile         |   1 +
>>>>  drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 301 insertions(+)
>>>>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
>>>>
>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>>> index 478f8ace..bdc86e14 100644
>>>> --- a/drivers/irqchip/Kconfig
>>>> +++ b/drivers/irqchip/Kconfig
>>>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>>>>  	help
>>>>  	  Say yes here to add support for the IRQ combiner devices embedded
>>>>  	  in Qualcomm Technologies chips.
>>>> +
>>>> +config MESON_GPIO_INTC
>>>> +	bool
>>>> +	depends on ARCH_MESON
>>>> +	default y
>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>> index b64c59b8..1be482bd 100644
>>>> --- a/drivers/irqchip/Makefile
>>>> +++ b/drivers/irqchip/Makefile
>>>> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>>>> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
>>>> --- /dev/null
>>>> +++ b/drivers/irqchip/irq-meson-gpio.c
>>>> @@ -0,0 +1,295 @@
>>>> +/*
>>>> + * Amlogic Meson GPIO IRQ chip driver
>>>> + *
>>>> + * Copyright (c) 2015 Endless Mobile, Inc.
>>>> + * Author: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>>>> + * Copyright (c) 2016 BayLibre, SAS.
>>>> + * Author: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> + * modify it under the terms of the GNU General Public License as
>>>> + * published by the Free Software Foundation, version 2.
>>>> + */
>>>> +
>>>> +#include <linux/device.h>
>>>> +#include <linux/init.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/irqchip.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_irq.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/regmap.h>
>>>> +
>>>> +#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 MAX_PARENT_IRQ_NUM	8
>>>> +
>>>> +/* maximum number of GPIO IRQs on supported platforms */
>>>> +#define MAX_NUM_GPIO_IRQ	133
>>>
>>> Why aren't these values coming from DT? I bet that a future revision of
>>> the same HW will double these. Or at least, you could match it on the
>>> compatible string.
>>>
>> Alternatively this value can be set to 255. The GPIO source is an 8 bit
>> value with 255 being reserved for "no interrupt source assigned".
> 
> Who is reserving it? The HW? Or is that your own defined convention?
> 
>> This way we cover all chips based on the same IP.
> 
> Why? Where is that 8bit limit coming from?
> 
The 8 bit limit is in the HW.

>> I think what we could gain by introducing an additional DT property
>> (saving a few bytes in the irqdomain mapping table) isn't worth the effort.
> 
> It is not about saving or wasting memory. It is about making the driver
> and its binding able to span multiple generation of the HW without too
> much churn. Which is why I'm suggesting that you either define these
> properties in DT *or* match the compatible string to obtain these values.
> 
>>>> +
>>>> +/*
>>>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
>>>> + * edge. That's due to HW constraints.
>>>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
>>>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
>>>> + * one bit to the right.
>>>
>>> Please expand on how you expect this to work, specially when a random
>>> driver expects a single interrupt.
>>>
>> The gpio interrupt controller in this chip doesn't have native support for
>> IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio
>> to two parent interrupts, one for each edge.
> 
> No, that's horrible, racy, and impractical. It has been proposed in the
> past (for the same HW), and we're not going there again.
> 
IIRC what has been proposed before is to re-program the polarity of edge
detection withing the ISR. This would match your concern that it is racy.

Here it's about using two parent irq's, one programmed to react on the
rising edge whilst the other is triggered in case of falling edge.
Would you consider this to be racy too?

>> There's still no solution to achieve this in a way everybody is happy with.
>> Therefore this feature isn't part of this patch set.
>>
>> However, to be prepared to include this feature later, the interface
>> between pinctrl/gpio and irqchip driver should (IMHO) cater for it already.
>> Else we may have to touch the irqchip driver later and change the interface
>> what I would like to avoid.
> 
> Don't even think of it, and considered it pre-NAKed.
> 
>>
>> If a driver just needs one (parent) interrupt, it can request hwirq
>> (2 * GPIO_HWIRQ) only. There's no issue with that.
> 
> Other than being utterly useless and confusing?
> 
>>>> + */
>>>> +#define NUM_GPIO_HWIRQ		(2 * MAX_NUM_GPIO_IRQ)
>>>> +
>>>> +struct meson_irq_slot {
>>>> +	unsigned int irq;
>>>> +	unsigned int owner;
>>>> +};
>>>> +
>>>> +struct meson_data {
>>>> +	struct regmap *regmap;
>>>> +	struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM];
>>>> +	unsigned int num_slots;
>>>> +	struct mutex lock;
>>>> +};
>>>> +
>>>> +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq)
>>>> +{
>>>> +	int i, slot = -ENOSPC;
>>>> +
>>>> +	mutex_lock(&md->lock);
>>>> +
>>>> +	for (i = 0; i < md->num_slots && slot < 0; i++)
>>>> +		if (!md->slots[i].owner) {
>>>> +			md->slots[i].owner = virq;
>>>
>>> Why do you have to deal with the virq? It'd be more logical to deal with
>>> the hwirq. The usual mechanism to reserve a "slot" is to use a bitmap
>>> indexed by the hwirq. Why is that not working for you?
>>>
>> Using the hwirq as owner should also be possible. Will consider this.
>>
>> A slot has two members, the owner and a the associated parent irq number.
>> Of course we could split this into a slot bitmap + an array with parent
>> irq's indexed by slot number. Would you prefer this?
> 
> Again, why do you need to consider the Linux irq number? All you need to
> track is whether a hwirq has been allocated or not, since all the
> irqchip function are called with an irq_data as a parameter, which
> contains both the irq and hwirq values.
> 
>>>> +			slot = i;
>>>> +		}
>>>> +
>>>> +	mutex_unlock(&md->lock);
>>>> +
>>>> +	return slot;
>>>> +}
>>>> +
>>>> +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq)
>>>> +{
>>>> +	int i;
>>>> +
>>>> +	mutex_lock(&md->lock);
>>>> +
>>>> +	for (i = 0; i < md->num_slots; i++)
>>>> +		if (md->slots[i].owner == virq) {
>>>> +			md->slots[i].owner = 0;
>>>> +			break;
>>>> +		}
>>>> +
>>>> +	mutex_unlock(&md->lock);
>>>> +}
>>>
>>> These two functions are basically the same...
>>>
>>>> +
>>>> +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq)
>>>> +{
>>>> +	int i, slot = -EINVAL;
>>>> +
>>>> +	mutex_lock(&md->lock);
>>>> +
>>>> +	for (i = 0; i < md->num_slots && slot < 0; i++)
>>>> +		if (md->slots[i].owner == virq)
>>>> +			slot = i;
>>>> +
>>>> +	mutex_unlock(&md->lock);
>>>> +
>>>> +	return slot;
>>>> +}
>>>
>>> ... and could be expressed in terms of this one.
>>>
>>>> +
>>>> +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int hwirq)
>>>> +{
>>>> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
>>>> +	int shift = 8 * (idx % 4);
>>>
>>> What's this?
>>>
>> GPIO source for the eight parent irq's can be configured using two 32-bit registers
>> with four 8-bit fields each.
> 
> Consider moving it to an accessor and document the mapping of the hwirqs
> in these registers.
> 
>>>> +
>>>> +	regmap_update_bits(md->regmap, reg, 0xff << shift,
>>>> +			   hwirq << shift);
>>>> +}
>>>> +
>>>> +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq)
>>>> +{
>>>> +	struct meson_data *md = data->domain->host_data;
>>>> +	int slot = meson_find_irq_slot(md, data->irq);
>>>> +
>>>> +	if (slot >= 0)
>>>> +		meson_set_hwirq(md, slot, hwirq);
>>>> +}
>>>> +
>>>> +static int meson_irq_set_type(struct irq_data *data, unsigned int type)
>>>> +{
>>>> +	struct meson_data *md = data->domain->host_data;
>>>> +	int slot;
>>>> +	unsigned int val = 0;
>>>> +
>>>> +	if (type == IRQ_TYPE_EDGE_BOTH)
>>>> +		return -EINVAL;
>>>
>>> So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?
>>>
>> We reject it in the initial version of the patch set as there's no consensus
>> yet on some details of the workaround needed for EDGE_BOTH support.
> 
> There is a consensus: The HW doesn't support this feature.
> 
Means what? There is no acceptable way to support EDGE_BOTH on this HW?
In this case I could stop here as for me this feature is important.

Would be somewhat a pity as there is a working solution. If it doesn't fit
into the current irq(chip) framework, shouldn't this be considered a gap
in the framework?

>>>> +
>>>> +	slot = meson_find_irq_slot(md, data->irq);
>>>> +	if (slot < 0)
>>>> +		return slot;
>>>
>>> How can this happen?
>>>
>> I see no way how this can happen. It was basically added to be on the safe
>> side and fail nicely in case I miss a scenario which could cause this to fail.
>> I can remove the check.
>>
>>>> +
>>>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>>>> +		val |= REG_EDGE_POL_EDGE(slot);
>>>> +
>>>> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
>>>> +		val |= REG_EDGE_POL_LOW(slot);
>>>> +
>>>> +	regmap_update_bits(md->regmap, REG_EDGE_POL,
>>>> +			   REG_EDGE_POL_MASK(slot), val);
>>>> +
>>>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>>>> +		val = IRQ_TYPE_EDGE_RISING;
>>>> +	else
>>>> +		val = IRQ_TYPE_LEVEL_HIGH;
>>>
>>> How does this work? Does this HW have some magic falling->rising and
>>> low->high conversion feature? If it doesn't, I cannot see how this can work.
>>>
>> Exactly, HW has a programmable polarity inverter.
>>
>>>> +
>>>> +	return irq_chip_set_type_parent(data, val);
>>>> +}
>>>> +
>>>> +static unsigned int meson_irq_startup(struct irq_data *data)
>>>> +{
>>>> +	irq_chip_unmask_parent(data);
>>>> +	/*
>>>> +	 * An extra bit was added to allow having the same gpio hwirq twice
>>>> +	 * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the
>>>> +	 * gpio hwirq.
>>>> +	 */
>>>> +	meson_irq_set_hwirq(data, data->hwirq >> 1);
>>>
>>> Again. Do you support EDGE_BOTH or not?
>>>
>>
>> Not yet ..
>>
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static void meson_irq_shutdown(struct irq_data *data)
>>>> +{
>>>> +	meson_irq_set_hwirq(data, 0xff);
>>>
>>> What's special about 0xff?
>>>
>> 0xff is the reserved value indicating the no GPIO source is assigned
>> to the parent irq.
> 
> Then document it, add a #define for this.
> 
>>>> +	irq_chip_mask_parent(data);
>>>> +}
>>>> +
>>>> +static struct irq_chip meson_irq_chip = {
>>>> +	.name = "meson_gpio_intc",
>>>> +	.irq_set_type = meson_irq_set_type,
>>>> +	.irq_eoi = irq_chip_eoi_parent,
>>>> +	.irq_mask = irq_chip_mask_parent,
>>>> +	.irq_unmask = irq_chip_unmask_parent,
>>>> +	.irq_startup = meson_irq_startup,
>>>> +	.irq_shutdown = meson_irq_shutdown,
>>>> +	.irq_set_affinity = irq_chip_set_affinity_parent,
>>>> +};
>>>> +
>>>> +static int meson_irq_alloc(struct irq_domain *d, unsigned int virq,
>>>> +			   unsigned int nr_irqs, void *data)
>>>> +{
>>>> +	struct irq_fwspec parent_fwspec, *fwspec = data;
>>>> +	struct meson_data *md = d->host_data;
>>>> +	irq_hw_number_t hwirq;
>>>> +	int ret, slot;
>>>> +
>>>> +	slot = meson_alloc_irq_slot(md, virq);
>>>> +	if (slot < 0)
>>>> +		return slot;
>>>> +
>>>> +	hwirq = fwspec->param[0];
>>>> +	irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL);
>>>> +
>>>> +	parent_fwspec.fwnode = d->parent->fwnode;
>>>> +	parent_fwspec.param_count = 3;
>>>> +	parent_fwspec.param[0] = 0; /* SPI */
>>>> +	parent_fwspec.param[1] = md->slots[slot].irq;
>>>> +	parent_fwspec.param[2] = IRQ_TYPE_NONE;
>>>
>>> Hell no. Look at the GIC DT binding: there is no NONE. It is either
>>> HIGH, or RISING.
>>>
>> OK, will be changed.
>>
>>>> +
>>>> +	ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec);
>>>> +	if (ret)
>>>> +		meson_free_irq_slot(md, virq);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>> +static void meson_irq_free(struct irq_domain *d, unsigned int virq,
>>>> +			   unsigned int nr_irqs)
>>>> +{
>>>> +	struct meson_data *md = d->host_data;
>>>> +
>>>> +	irq_domain_free_irqs_common(d, virq, nr_irqs);
>>>> +	meson_free_irq_slot(md, virq);
>>>> +}
>>>> +
>>>> +static const struct irq_domain_ops meson_irq_ops = {
>>>> +	.alloc = meson_irq_alloc,
>>>> +	.free = meson_irq_free,
>>>> +	.xlate = irq_domain_xlate_twocell,
>>>> +};
>>>> +
>>>> +static int meson_get_irqs(struct meson_data *md, struct device_node *node)
>>>> +{
>>>> +	int ret, i;
>>>> +	u32 irq;
>>>> +
>>>> +	for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) {
>>>> +		ret = of_property_read_u32_index(node, "interrupts", i, &irq);
>>>> +		if (ret)
>>>> +			break;
>>>> +		md->slots[i].irq = irq;
>>>> +	}
>>>> +
>>>> +	md->num_slots = i;
>>>> +
>>>> +	return i ? 0 : -EINVAL;
>>>> +}
>>>> +
>>>> +static const struct regmap_config meson_regmap_config = {
>>>> +	.reg_bits       = 32,
>>>> +	.reg_stride     = 4,
>>>> +	.val_bits       = 32,
>>>> +	.max_register	= REG_FILTER_SEL,
>>>> +};
>>>> +
>>>> +static int __init meson_gpio_irq_init(struct device_node *node,
>>>> +				      struct device_node *parent)
>>>> +{
>>>> +	struct irq_domain *meson_irq_domain, *parent_domain;
>>>> +	struct meson_data *md;
>>>> +	void __iomem *io_base;
>>>> +	int ret;
>>>> +
>>>> +	md = kzalloc(sizeof(*md), GFP_KERNEL);
>>>> +	if (!md)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	mutex_init(&md->lock);
>>>> +
>>>> +	io_base = of_iomap(node, 0);
>>>> +	if (!io_base)
>>>> +		return -EINVAL;
>>>> +
>>>> +	md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config);
>>>> +	if (IS_ERR(md->regmap))
>>>> +		return PTR_ERR(md->regmap);
>>>> +
>>>> +	/* initialize to IRQ_TYPE_LEVEL_HIGH */
>>>> +	regmap_write(md->regmap, REG_EDGE_POL, 0);
>>>> +	/* disable all GPIO interrupt sources */
>>>> +	regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff);
>>>> +	regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff);
>>>> +	/* disable filtering */
>>>> +	regmap_write(md->regmap, REG_FILTER_SEL, 0);
>>>> +
>>>> +	ret = meson_get_irqs(md, node);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	parent_domain = irq_find_host(parent);
>>>> +	if (!parent_domain)
>>>> +		return -ENXIO;
>>>
>>> Memory leak on all the return paths.
>>>
>> Indeed. Most likely copy & paste error when I used other irqchip drivers
>> as inspiration. E.g. irq-crossbar faces the same issue, it allocates memory
>> in crossbar_of_init which isn't free'd if irq_domain_add_hierarchy fails.
> 
> Then please submit a patch addressing the defect.
> 
> Thanks,
> 
> 	M.
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Marc Zyngier]

On 15/06/17 16:24, Heiner Kallweit wrote:
> Am 15.06.2017 um 15:27 schrieb Marc Zyngier:
>> On 15/06/17 14:10, Heiner Kallweit wrote:
>>> Am 13.06.2017 um 10:31 schrieb Marc Zyngier:
>>>> On 10/06/17 22:57, Heiner Kallweit wrote:
>>>>> Add a driver supporting the GPIO interrupt controller on certain
>>>>> Amlogic meson SoC's.
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>> ---
>>>>> v5:
>>>>> - changed Kconfig entry based on Neil's suggestion
>>>>> - added authors
>>>>> - extended explanation why 2 * n hwirqs are used
>>>>> v6:
>>>>> - change DT property parent-interrupts to interrupts
>>>>> v7:
>>>>> - no changes
>>>>> ---
>>>>>  drivers/irqchip/Kconfig          |   5 +
>>>>>  drivers/irqchip/Makefile         |   1 +
>>>>>  drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
>>>>>  3 files changed, 301 insertions(+)
>>>>>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
>>>>>
>>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>>>> index 478f8ace..bdc86e14 100644
>>>>> --- a/drivers/irqchip/Kconfig
>>>>> +++ b/drivers/irqchip/Kconfig
>>>>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>>>>>  	help
>>>>>  	  Say yes here to add support for the IRQ combiner devices embedded
>>>>>  	  in Qualcomm Technologies chips.
>>>>> +
>>>>> +config MESON_GPIO_INTC
>>>>> +	bool
>>>>> +	depends on ARCH_MESON
>>>>> +	default y
>>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>>> index b64c59b8..1be482bd 100644
>>>>> --- a/drivers/irqchip/Makefile
>>>>> +++ b/drivers/irqchip/Makefile
>>>>> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>>>>> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
>>>>> --- /dev/null
>>>>> +++ b/drivers/irqchip/irq-meson-gpio.c
>>>>> @@ -0,0 +1,295 @@
>>>>> +/*
>>>>> + * Amlogic Meson GPIO IRQ chip driver
>>>>> + *
>>>>> + * Copyright (c) 2015 Endless Mobile, Inc.
>>>>> + * Author: Carlo Caione <carlo@endlessm.com>
>>>>> + * Copyright (c) 2016 BayLibre, SAS.
>>>>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>>>>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1@gmail.com>
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or
>>>>> + * modify it under the terms of the GNU General Public License as
>>>>> + * published by the Free Software Foundation, version 2.
>>>>> + */
>>>>> +
>>>>> +#include <linux/device.h>
>>>>> +#include <linux/init.h>
>>>>> +#include <linux/interrupt.h>
>>>>> +#include <linux/irqchip.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/of_irq.h>
>>>>> +#include <linux/of_address.h>
>>>>> +#include <linux/regmap.h>
>>>>> +
>>>>> +#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 MAX_PARENT_IRQ_NUM	8
>>>>> +
>>>>> +/* maximum number of GPIO IRQs on supported platforms */
>>>>> +#define MAX_NUM_GPIO_IRQ	133
>>>>
>>>> Why aren't these values coming from DT? I bet that a future revision of
>>>> the same HW will double these. Or at least, you could match it on the
>>>> compatible string.
>>>>
>>> Alternatively this value can be set to 255. The GPIO source is an 8 bit
>>> value with 255 being reserved for "no interrupt source assigned".
>>
>> Who is reserving it? The HW? Or is that your own defined convention?
>>
>>> This way we cover all chips based on the same IP.
>>
>> Why? Where is that 8bit limit coming from?
>>
> The 8 bit limit is in the HW.
> 
>>> I think what we could gain by introducing an additional DT property
>>> (saving a few bytes in the irqdomain mapping table) isn't worth the effort.
>>
>> It is not about saving or wasting memory. It is about making the driver
>> and its binding able to span multiple generation of the HW without too
>> much churn. Which is why I'm suggesting that you either define these
>> properties in DT *or* match the compatible string to obtain these values.
>>
>>>>> +
>>>>> +/*
>>>>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
>>>>> + * edge. That's due to HW constraints.
>>>>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
>>>>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
>>>>> + * one bit to the right.
>>>>
>>>> Please expand on how you expect this to work, specially when a random
>>>> driver expects a single interrupt.
>>>>
>>> The gpio interrupt controller in this chip doesn't have native support for
>>> IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio
>>> to two parent interrupts, one for each edge.
>>
>> No, that's horrible, racy, and impractical. It has been proposed in the
>> past (for the same HW), and we're not going there again.
>>
> IIRC what has been proposed before is to re-program the polarity of edge
> detection withing the ISR. This would match your concern that it is racy.
> 
> Here it's about using two parent irq's, one programmed to react on the
> rising edge whilst the other is triggered in case of falling edge.
> Would you consider this to be racy too?

How do you reconcile two interrupts to make look like a single one for a
random, pre-existing driver?

[...]

>>>> So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?
>>>>
>>> We reject it in the initial version of the patch set as there's no consensus
>>> yet on some details of the workaround needed for EDGE_BOTH support.
>>
>> There is a consensus: The HW doesn't support this feature.
>>
> Means what? There is no acceptable way to support EDGE_BOTH on this HW?
> In this case I could stop here as for me this feature is important.

Answer my question above, which I asked in my initial review: How do you
make two interrupts appear as one for a driver that wants to get
signaled on each edge, using the existing API.

> 
> Would be somewhat a pity as there is a working solution. If it doesn't fit
> into the current irq(chip) framework, shouldn't this be considered a gap
> in the framework?

Again, this is not about the framework, which we can almost twist as we
see fit. It is about the API that a driver expects. At the moment, you
haven't proposed anything.

Thanks,

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

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Heiner Kallweit]

Am 15.06.2017 um 18:04 schrieb Marc Zyngier:
> On 15/06/17 16:24, Heiner Kallweit wrote:
>> Am 15.06.2017 um 15:27 schrieb Marc Zyngier:
>>> On 15/06/17 14:10, Heiner Kallweit wrote:
>>>> Am 13.06.2017 um 10:31 schrieb Marc Zyngier:
>>>>> On 10/06/17 22:57, Heiner Kallweit wrote:
>>>>>> Add a driver supporting the GPIO interrupt controller on certain
>>>>>> Amlogic meson SoC's.
>>>>>>
>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>> ---
>>>>>> v5:
>>>>>> - changed Kconfig entry based on Neil's suggestion
>>>>>> - added authors
>>>>>> - extended explanation why 2 * n hwirqs are used
>>>>>> v6:
>>>>>> - change DT property parent-interrupts to interrupts
>>>>>> v7:
>>>>>> - no changes
>>>>>> ---
>>>>>>  drivers/irqchip/Kconfig          |   5 +
>>>>>>  drivers/irqchip/Makefile         |   1 +
>>>>>>  drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
>>>>>>  3 files changed, 301 insertions(+)
>>>>>>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
>>>>>>
>>>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>>>>> index 478f8ace..bdc86e14 100644
>>>>>> --- a/drivers/irqchip/Kconfig
>>>>>> +++ b/drivers/irqchip/Kconfig
>>>>>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>>>>>>  	help
>>>>>>  	  Say yes here to add support for the IRQ combiner devices embedded
>>>>>>  	  in Qualcomm Technologies chips.
>>>>>> +
>>>>>> +config MESON_GPIO_INTC
>>>>>> +	bool
>>>>>> +	depends on ARCH_MESON
>>>>>> +	default y
>>>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>>>> index b64c59b8..1be482bd 100644
>>>>>> --- a/drivers/irqchip/Makefile
>>>>>> +++ b/drivers/irqchip/Makefile
>>>>>> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>>>>>> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/irqchip/irq-meson-gpio.c
>>>>>> @@ -0,0 +1,295 @@
>>>>>> +/*
>>>>>> + * Amlogic Meson GPIO IRQ chip driver
>>>>>> + *
>>>>>> + * Copyright (c) 2015 Endless Mobile, Inc.
>>>>>> + * Author: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>>>>>> + * Copyright (c) 2016 BayLibre, SAS.
>>>>>> + * Author: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>>>>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>> + *
>>>>>> + * This program is free software; you can redistribute it and/or
>>>>>> + * modify it under the terms of the GNU General Public License as
>>>>>> + * published by the Free Software Foundation, version 2.
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/device.h>
>>>>>> +#include <linux/init.h>
>>>>>> +#include <linux/interrupt.h>
>>>>>> +#include <linux/irqchip.h>
>>>>>> +#include <linux/of.h>
>>>>>> +#include <linux/of_irq.h>
>>>>>> +#include <linux/of_address.h>
>>>>>> +#include <linux/regmap.h>
>>>>>> +
>>>>>> +#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 MAX_PARENT_IRQ_NUM	8
>>>>>> +
>>>>>> +/* maximum number of GPIO IRQs on supported platforms */
>>>>>> +#define MAX_NUM_GPIO_IRQ	133
>>>>>
>>>>> Why aren't these values coming from DT? I bet that a future revision of
>>>>> the same HW will double these. Or at least, you could match it on the
>>>>> compatible string.
>>>>>
>>>> Alternatively this value can be set to 255. The GPIO source is an 8 bit
>>>> value with 255 being reserved for "no interrupt source assigned".
>>>
>>> Who is reserving it? The HW? Or is that your own defined convention?
>>>
>>>> This way we cover all chips based on the same IP.
>>>
>>> Why? Where is that 8bit limit coming from?
>>>
>> The 8 bit limit is in the HW.
>>
>>>> I think what we could gain by introducing an additional DT property
>>>> (saving a few bytes in the irqdomain mapping table) isn't worth the effort.
>>>
>>> It is not about saving or wasting memory. It is about making the driver
>>> and its binding able to span multiple generation of the HW without too
>>> much churn. Which is why I'm suggesting that you either define these
>>> properties in DT *or* match the compatible string to obtain these values.
>>>
>>>>>> +
>>>>>> +/*
>>>>>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
>>>>>> + * edge. That's due to HW constraints.
>>>>>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
>>>>>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
>>>>>> + * one bit to the right.
>>>>>
>>>>> Please expand on how you expect this to work, specially when a random
>>>>> driver expects a single interrupt.
>>>>>
>>>> The gpio interrupt controller in this chip doesn't have native support for
>>>> IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio
>>>> to two parent interrupts, one for each edge.
>>>
>>> No, that's horrible, racy, and impractical. It has been proposed in the
>>> past (for the same HW), and we're not going there again.
>>>
>> IIRC what has been proposed before is to re-program the polarity of edge
>> detection withing the ISR. This would match your concern that it is racy.
>>
>> Here it's about using two parent irq's, one programmed to react on the
>> rising edge whilst the other is triggered in case of falling edge.
>> Would you consider this to be racy too?
> 
> How do you reconcile two interrupts to make look like a single one for a
> random, pre-existing driver?
> 
> [...]
> 
>>>>> So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?
>>>>>
>>>> We reject it in the initial version of the patch set as there's no consensus
>>>> yet on some details of the workaround needed for EDGE_BOTH support.
>>>
>>> There is a consensus: The HW doesn't support this feature.
>>>
>> Means what? There is no acceptable way to support EDGE_BOTH on this HW?
>> In this case I could stop here as for me this feature is important.
> 
> Answer my question above, which I asked in my initial review: How do you
> make two interrupts appear as one for a driver that wants to get
> signaled on each edge, using the existing API.
> 
Please see the pinctrl/gpio part of the patch set.

The GPIO controller has an own IRQ domain. When requesting an interrupt
in request_resources if needed two parent irq's are allocated, (was removed
in the current initial version of the patch set) both calling the same ISR
via a chained interrupt.

Works perfectly fine here.

>>
>> Would be somewhat a pity as there is a working solution. If it doesn't fit
>> into the current irq(chip) framework, shouldn't this be considered a gap
>> in the framework?
> 
> Again, this is not about the framework, which we can almost twist as we
> see fit. It is about the API that a driver expects. At the moment, you
> haven't proposed anything.
> 
> Thanks,
> 
> 	M.
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Marc Zyngier]

On 15/06/17 17:37, Heiner Kallweit wrote:
> Am 15.06.2017 um 18:04 schrieb Marc Zyngier:
>> On 15/06/17 16:24, Heiner Kallweit wrote:
>>> Am 15.06.2017 um 15:27 schrieb Marc Zyngier:
>>>> On 15/06/17 14:10, Heiner Kallweit wrote:
>>>>> Am 13.06.2017 um 10:31 schrieb Marc Zyngier:
>>>>>> On 10/06/17 22:57, Heiner Kallweit wrote:
>>>>>>> Add a driver supporting the GPIO interrupt controller on certain
>>>>>>> Amlogic meson SoC's.
>>>>>>>
>>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>>>> ---
>>>>>>> v5:
>>>>>>> - changed Kconfig entry based on Neil's suggestion
>>>>>>> - added authors
>>>>>>> - extended explanation why 2 * n hwirqs are used
>>>>>>> v6:
>>>>>>> - change DT property parent-interrupts to interrupts
>>>>>>> v7:
>>>>>>> - no changes
>>>>>>> ---
>>>>>>>  drivers/irqchip/Kconfig          |   5 +
>>>>>>>  drivers/irqchip/Makefile         |   1 +
>>>>>>>  drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
>>>>>>>  3 files changed, 301 insertions(+)
>>>>>>>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
>>>>>>>
>>>>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>>>>>> index 478f8ace..bdc86e14 100644
>>>>>>> --- a/drivers/irqchip/Kconfig
>>>>>>> +++ b/drivers/irqchip/Kconfig
>>>>>>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>>>>>>>  	help
>>>>>>>  	  Say yes here to add support for the IRQ combiner devices embedded
>>>>>>>  	  in Qualcomm Technologies chips.
>>>>>>> +
>>>>>>> +config MESON_GPIO_INTC
>>>>>>> +	bool
>>>>>>> +	depends on ARCH_MESON
>>>>>>> +	default y
>>>>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>>>>> index b64c59b8..1be482bd 100644
>>>>>>> --- a/drivers/irqchip/Makefile
>>>>>>> +++ b/drivers/irqchip/Makefile
>>>>>>> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>>>>>>> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
>>>>>>> --- /dev/null
>>>>>>> +++ b/drivers/irqchip/irq-meson-gpio.c
>>>>>>> @@ -0,0 +1,295 @@
>>>>>>> +/*
>>>>>>> + * Amlogic Meson GPIO IRQ chip driver
>>>>>>> + *
>>>>>>> + * Copyright (c) 2015 Endless Mobile, Inc.
>>>>>>> + * Author: Carlo Caione <carlo@endlessm.com>
>>>>>>> + * Copyright (c) 2016 BayLibre, SAS.
>>>>>>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>>>>>>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1@gmail.com>
>>>>>>> + *
>>>>>>> + * This program is free software; you can redistribute it and/or
>>>>>>> + * modify it under the terms of the GNU General Public License as
>>>>>>> + * published by the Free Software Foundation, version 2.
>>>>>>> + */
>>>>>>> +
>>>>>>> +#include <linux/device.h>
>>>>>>> +#include <linux/init.h>
>>>>>>> +#include <linux/interrupt.h>
>>>>>>> +#include <linux/irqchip.h>
>>>>>>> +#include <linux/of.h>
>>>>>>> +#include <linux/of_irq.h>
>>>>>>> +#include <linux/of_address.h>
>>>>>>> +#include <linux/regmap.h>
>>>>>>> +
>>>>>>> +#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 MAX_PARENT_IRQ_NUM	8
>>>>>>> +
>>>>>>> +/* maximum number of GPIO IRQs on supported platforms */
>>>>>>> +#define MAX_NUM_GPIO_IRQ	133
>>>>>>
>>>>>> Why aren't these values coming from DT? I bet that a future revision of
>>>>>> the same HW will double these. Or at least, you could match it on the
>>>>>> compatible string.
>>>>>>
>>>>> Alternatively this value can be set to 255. The GPIO source is an 8 bit
>>>>> value with 255 being reserved for "no interrupt source assigned".
>>>>
>>>> Who is reserving it? The HW? Or is that your own defined convention?
>>>>
>>>>> This way we cover all chips based on the same IP.
>>>>
>>>> Why? Where is that 8bit limit coming from?
>>>>
>>> The 8 bit limit is in the HW.
>>>
>>>>> I think what we could gain by introducing an additional DT property
>>>>> (saving a few bytes in the irqdomain mapping table) isn't worth the effort.
>>>>
>>>> It is not about saving or wasting memory. It is about making the driver
>>>> and its binding able to span multiple generation of the HW without too
>>>> much churn. Which is why I'm suggesting that you either define these
>>>> properties in DT *or* match the compatible string to obtain these values.
>>>>
>>>>>>> +
>>>>>>> +/*
>>>>>>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
>>>>>>> + * edge. That's due to HW constraints.
>>>>>>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
>>>>>>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
>>>>>>> + * one bit to the right.
>>>>>>
>>>>>> Please expand on how you expect this to work, specially when a random
>>>>>> driver expects a single interrupt.
>>>>>>
>>>>> The gpio interrupt controller in this chip doesn't have native support for
>>>>> IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio
>>>>> to two parent interrupts, one for each edge.
>>>>
>>>> No, that's horrible, racy, and impractical. It has been proposed in the
>>>> past (for the same HW), and we're not going there again.
>>>>
>>> IIRC what has been proposed before is to re-program the polarity of edge
>>> detection withing the ISR. This would match your concern that it is racy.
>>>
>>> Here it's about using two parent irq's, one programmed to react on the
>>> rising edge whilst the other is triggered in case of falling edge.
>>> Would you consider this to be racy too?
>>
>> How do you reconcile two interrupts to make look like a single one for a
>> random, pre-existing driver?
>>
>> [...]
>>
>>>>>> So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?
>>>>>>
>>>>> We reject it in the initial version of the patch set as there's no consensus
>>>>> yet on some details of the workaround needed for EDGE_BOTH support.
>>>>
>>>> There is a consensus: The HW doesn't support this feature.
>>>>
>>> Means what? There is no acceptable way to support EDGE_BOTH on this HW?
>>> In this case I could stop here as for me this feature is important.
>>
>> Answer my question above, which I asked in my initial review: How do you
>> make two interrupts appear as one for a driver that wants to get
>> signaled on each edge, using the existing API.
>>
> Please see the pinctrl/gpio part of the patch set.
> 
> The GPIO controller has an own IRQ domain. When requesting an interrupt
> in request_resources if needed two parent irq's are allocated, (was removed
> in the current initial version of the patch set) both calling the same ISR
> via a chained interrupt.
> 
> Works perfectly fine here.

Are you referring to the horror that performs interrupt allocations from
within the irq_set_type callback? No way. That's beyond disgusting. And
potentially broken, as the locks that are being taken were never
designed to nest that way.

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

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Heiner Kallweit]

Am 15.06.2017 um 18:58 schrieb Marc Zyngier:
> On 15/06/17 17:37, Heiner Kallweit wrote:
>> Am 15.06.2017 um 18:04 schrieb Marc Zyngier:
>>> On 15/06/17 16:24, Heiner Kallweit wrote:
>>>> Am 15.06.2017 um 15:27 schrieb Marc Zyngier:
>>>>> On 15/06/17 14:10, Heiner Kallweit wrote:
>>>>>> Am 13.06.2017 um 10:31 schrieb Marc Zyngier:
>>>>>>> On 10/06/17 22:57, Heiner Kallweit wrote:
>>>>>>>> Add a driver supporting the GPIO interrupt controller on certain
>>>>>>>> Amlogic meson SoC's.
>>>>>>>>
>>>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>>>>> ---
>>>>>>>> v5:
>>>>>>>> - changed Kconfig entry based on Neil's suggestion
>>>>>>>> - added authors
>>>>>>>> - extended explanation why 2 * n hwirqs are used
>>>>>>>> v6:
>>>>>>>> - change DT property parent-interrupts to interrupts
>>>>>>>> v7:
>>>>>>>> - no changes
>>>>>>>> ---
>>>>>>>>  drivers/irqchip/Kconfig          |   5 +
>>>>>>>>  drivers/irqchip/Makefile         |   1 +
>>>>>>>>  drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
>>>>>>>>  3 files changed, 301 insertions(+)
>>>>>>>>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
>>>>>>>>
>>>>>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>>>>>>> index 478f8ace..bdc86e14 100644
>>>>>>>> --- a/drivers/irqchip/Kconfig
>>>>>>>> +++ b/drivers/irqchip/Kconfig
>>>>>>>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>>>>>>>>  	help
>>>>>>>>  	  Say yes here to add support for the IRQ combiner devices embedded
>>>>>>>>  	  in Qualcomm Technologies chips.
>>>>>>>> +
>>>>>>>> +config MESON_GPIO_INTC
>>>>>>>> +	bool
>>>>>>>> +	depends on ARCH_MESON
>>>>>>>> +	default y
>>>>>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>>>>>> index b64c59b8..1be482bd 100644
>>>>>>>> --- a/drivers/irqchip/Makefile
>>>>>>>> +++ b/drivers/irqchip/Makefile
>>>>>>>> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>>>>>>>> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/irqchip/irq-meson-gpio.c
>>>>>>>> @@ -0,0 +1,295 @@
>>>>>>>> +/*
>>>>>>>> + * Amlogic Meson GPIO IRQ chip driver
>>>>>>>> + *
>>>>>>>> + * Copyright (c) 2015 Endless Mobile, Inc.
>>>>>>>> + * Author: Carlo Caione <carlo@endlessm.com>
>>>>>>>> + * Copyright (c) 2016 BayLibre, SAS.
>>>>>>>> + * Author: Jerome Brunet <jbrunet@baylibre.com>
>>>>>>>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1@gmail.com>
>>>>>>>> + *
>>>>>>>> + * This program is free software; you can redistribute it and/or
>>>>>>>> + * modify it under the terms of the GNU General Public License as
>>>>>>>> + * published by the Free Software Foundation, version 2.
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> +#include <linux/device.h>
>>>>>>>> +#include <linux/init.h>
>>>>>>>> +#include <linux/interrupt.h>
>>>>>>>> +#include <linux/irqchip.h>
>>>>>>>> +#include <linux/of.h>
>>>>>>>> +#include <linux/of_irq.h>
>>>>>>>> +#include <linux/of_address.h>
>>>>>>>> +#include <linux/regmap.h>
>>>>>>>> +
>>>>>>>> +#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 MAX_PARENT_IRQ_NUM	8
>>>>>>>> +
>>>>>>>> +/* maximum number of GPIO IRQs on supported platforms */
>>>>>>>> +#define MAX_NUM_GPIO_IRQ	133
>>>>>>>
>>>>>>> Why aren't these values coming from DT? I bet that a future revision of
>>>>>>> the same HW will double these. Or at least, you could match it on the
>>>>>>> compatible string.
>>>>>>>
>>>>>> Alternatively this value can be set to 255. The GPIO source is an 8 bit
>>>>>> value with 255 being reserved for "no interrupt source assigned".
>>>>>
>>>>> Who is reserving it? The HW? Or is that your own defined convention?
>>>>>
>>>>>> This way we cover all chips based on the same IP.
>>>>>
>>>>> Why? Where is that 8bit limit coming from?
>>>>>
>>>> The 8 bit limit is in the HW.
>>>>
>>>>>> I think what we could gain by introducing an additional DT property
>>>>>> (saving a few bytes in the irqdomain mapping table) isn't worth the effort.
>>>>>
>>>>> It is not about saving or wasting memory. It is about making the driver
>>>>> and its binding able to span multiple generation of the HW without too
>>>>> much churn. Which is why I'm suggesting that you either define these
>>>>> properties in DT *or* match the compatible string to obtain these values.
>>>>>
>>>>>>>> +
>>>>>>>> +/*
>>>>>>>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
>>>>>>>> + * edge. That's due to HW constraints.
>>>>>>>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
>>>>>>>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
>>>>>>>> + * one bit to the right.
>>>>>>>
>>>>>>> Please expand on how you expect this to work, specially when a random
>>>>>>> driver expects a single interrupt.
>>>>>>>
>>>>>> The gpio interrupt controller in this chip doesn't have native support for
>>>>>> IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio
>>>>>> to two parent interrupts, one for each edge.
>>>>>
>>>>> No, that's horrible, racy, and impractical. It has been proposed in the
>>>>> past (for the same HW), and we're not going there again.
>>>>>
>>>> IIRC what has been proposed before is to re-program the polarity of edge
>>>> detection withing the ISR. This would match your concern that it is racy.
>>>>
>>>> Here it's about using two parent irq's, one programmed to react on the
>>>> rising edge whilst the other is triggered in case of falling edge.
>>>> Would you consider this to be racy too?
>>>
>>> How do you reconcile two interrupts to make look like a single one for a
>>> random, pre-existing driver?
>>>
>>> [...]
>>>
>>>>>>> So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?
>>>>>>>
>>>>>> We reject it in the initial version of the patch set as there's no consensus
>>>>>> yet on some details of the workaround needed for EDGE_BOTH support.
>>>>>
>>>>> There is a consensus: The HW doesn't support this feature.
>>>>>
>>>> Means what? There is no acceptable way to support EDGE_BOTH on this HW?
>>>> In this case I could stop here as for me this feature is important.
>>>
>>> Answer my question above, which I asked in my initial review: How do you
>>> make two interrupts appear as one for a driver that wants to get
>>> signaled on each edge, using the existing API.
>>>
>> Please see the pinctrl/gpio part of the patch set.
>>
>> The GPIO controller has an own IRQ domain. When requesting an interrupt
>> in request_resources if needed two parent irq's are allocated, (was removed
>> in the current initial version of the patch set) both calling the same ISR
>> via a chained interrupt.
>>
>> Works perfectly fine here.
> 
> Are you referring to the horror that performs interrupt allocations from
> within the irq_set_type callback? No way. That's beyond disgusting. And
> potentially broken, as the locks that are being taken were never
> designed to nest that way.
> 
No, this horror was the first attempt. In v7 of the patch set this is
done from the request_resources callback.

Rgds, Heiner

> 	M.
> 

Re: [PATCH v7 1/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Marc Zyngier]

On 15/06/17 20:03, Heiner Kallweit wrote:
> Am 15.06.2017 um 18:58 schrieb Marc Zyngier:
>> On 15/06/17 17:37, Heiner Kallweit wrote:
>>> Am 15.06.2017 um 18:04 schrieb Marc Zyngier:
>>>> On 15/06/17 16:24, Heiner Kallweit wrote:
>>>>> Am 15.06.2017 um 15:27 schrieb Marc Zyngier:
>>>>>> On 15/06/17 14:10, Heiner Kallweit wrote:
>>>>>>> Am 13.06.2017 um 10:31 schrieb Marc Zyngier:
>>>>>>>> On 10/06/17 22:57, Heiner Kallweit wrote:
>>>>>>>>> Add a driver supporting the GPIO interrupt controller on certain
>>>>>>>>> Amlogic meson SoC's.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>>>>> ---
>>>>>>>>> v5:
>>>>>>>>> - changed Kconfig entry based on Neil's suggestion
>>>>>>>>> - added authors
>>>>>>>>> - extended explanation why 2 * n hwirqs are used
>>>>>>>>> v6:
>>>>>>>>> - change DT property parent-interrupts to interrupts
>>>>>>>>> v7:
>>>>>>>>> - no changes
>>>>>>>>> ---
>>>>>>>>>  drivers/irqchip/Kconfig          |   5 +
>>>>>>>>>  drivers/irqchip/Makefile         |   1 +
>>>>>>>>>  drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++
>>>>>>>>>  3 files changed, 301 insertions(+)
>>>>>>>>>  create mode 100644 drivers/irqchip/irq-meson-gpio.c
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
>>>>>>>>> index 478f8ace..bdc86e14 100644
>>>>>>>>> --- a/drivers/irqchip/Kconfig
>>>>>>>>> +++ b/drivers/irqchip/Kconfig
>>>>>>>>> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER
>>>>>>>>>  	help
>>>>>>>>>  	  Say yes here to add support for the IRQ combiner devices embedded
>>>>>>>>>  	  in Qualcomm Technologies chips.
>>>>>>>>> +
>>>>>>>>> +config MESON_GPIO_INTC
>>>>>>>>> +	bool
>>>>>>>>> +	depends on ARCH_MESON
>>>>>>>>> +	default y
>>>>>>>>> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
>>>>>>>>> index b64c59b8..1be482bd 100644
>>>>>>>>> --- a/drivers/irqchip/Makefile
>>>>>>>>> +++ b/drivers/irqchip/Makefile
>>>>>>>>> @@ -76,3 +76,4 @@ 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_QCOM_IRQ_COMBINER)		+= qcom-irq-combiner.o
>>>>>>>>> +obj-$(CONFIG_MESON_GPIO_INTC)		+= 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 00000000..925d00c2
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/drivers/irqchip/irq-meson-gpio.c
>>>>>>>>> @@ -0,0 +1,295 @@
>>>>>>>>> +/*
>>>>>>>>> + * Amlogic Meson GPIO IRQ chip driver
>>>>>>>>> + *
>>>>>>>>> + * Copyright (c) 2015 Endless Mobile, Inc.
>>>>>>>>> + * Author: Carlo Caione <carlo-6IF/jdPJHihWk0Htik3J/w@public.gmane.org>
>>>>>>>>> + * Copyright (c) 2016 BayLibre, SAS.
>>>>>>>>> + * Author: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>>>>>>>>> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>>>>>>>>> + *
>>>>>>>>> + * This program is free software; you can redistribute it and/or
>>>>>>>>> + * modify it under the terms of the GNU General Public License as
>>>>>>>>> + * published by the Free Software Foundation, version 2.
>>>>>>>>> + */
>>>>>>>>> +
>>>>>>>>> +#include <linux/device.h>
>>>>>>>>> +#include <linux/init.h>
>>>>>>>>> +#include <linux/interrupt.h>
>>>>>>>>> +#include <linux/irqchip.h>
>>>>>>>>> +#include <linux/of.h>
>>>>>>>>> +#include <linux/of_irq.h>
>>>>>>>>> +#include <linux/of_address.h>
>>>>>>>>> +#include <linux/regmap.h>
>>>>>>>>> +
>>>>>>>>> +#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 MAX_PARENT_IRQ_NUM	8
>>>>>>>>> +
>>>>>>>>> +/* maximum number of GPIO IRQs on supported platforms */
>>>>>>>>> +#define MAX_NUM_GPIO_IRQ	133
>>>>>>>>
>>>>>>>> Why aren't these values coming from DT? I bet that a future revision of
>>>>>>>> the same HW will double these. Or at least, you could match it on the
>>>>>>>> compatible string.
>>>>>>>>
>>>>>>> Alternatively this value can be set to 255. The GPIO source is an 8 bit
>>>>>>> value with 255 being reserved for "no interrupt source assigned".
>>>>>>
>>>>>> Who is reserving it? The HW? Or is that your own defined convention?
>>>>>>
>>>>>>> This way we cover all chips based on the same IP.
>>>>>>
>>>>>> Why? Where is that 8bit limit coming from?
>>>>>>
>>>>> The 8 bit limit is in the HW.
>>>>>
>>>>>>> I think what we could gain by introducing an additional DT property
>>>>>>> (saving a few bytes in the irqdomain mapping table) isn't worth the effort.
>>>>>>
>>>>>> It is not about saving or wasting memory. It is about making the driver
>>>>>> and its binding able to span multiple generation of the HW without too
>>>>>> much churn. Which is why I'm suggesting that you either define these
>>>>>> properties in DT *or* match the compatible string to obtain these values.
>>>>>>
>>>>>>>>> +
>>>>>>>>> +/*
>>>>>>>>> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each
>>>>>>>>> + * edge. That's due to HW constraints.
>>>>>>>>> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one
>>>>>>>>> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq
>>>>>>>>> + * one bit to the right.
>>>>>>>>
>>>>>>>> Please expand on how you expect this to work, specially when a random
>>>>>>>> driver expects a single interrupt.
>>>>>>>>
>>>>>>> The gpio interrupt controller in this chip doesn't have native support for
>>>>>>> IRQ_TYPE_EDGE_BOTH. As a workaround we would need to assign the same gpio
>>>>>>> to two parent interrupts, one for each edge.
>>>>>>
>>>>>> No, that's horrible, racy, and impractical. It has been proposed in the
>>>>>> past (for the same HW), and we're not going there again.
>>>>>>
>>>>> IIRC what has been proposed before is to re-program the polarity of edge
>>>>> detection withing the ISR. This would match your concern that it is racy.
>>>>>
>>>>> Here it's about using two parent irq's, one programmed to react on the
>>>>> rising edge whilst the other is triggered in case of falling edge.
>>>>> Would you consider this to be racy too?
>>>>
>>>> How do you reconcile two interrupts to make look like a single one for a
>>>> random, pre-existing driver?
>>>>
>>>> [...]
>>>>
>>>>>>>> So you reject EDGE_BOTH? So what's the deal with the beginning of the patch?
>>>>>>>>
>>>>>>> We reject it in the initial version of the patch set as there's no consensus
>>>>>>> yet on some details of the workaround needed for EDGE_BOTH support.
>>>>>>
>>>>>> There is a consensus: The HW doesn't support this feature.
>>>>>>
>>>>> Means what? There is no acceptable way to support EDGE_BOTH on this HW?
>>>>> In this case I could stop here as for me this feature is important.
>>>>
>>>> Answer my question above, which I asked in my initial review: How do you
>>>> make two interrupts appear as one for a driver that wants to get
>>>> signaled on each edge, using the existing API.
>>>>
>>> Please see the pinctrl/gpio part of the patch set.
>>>
>>> The GPIO controller has an own IRQ domain. When requesting an interrupt
>>> in request_resources if needed two parent irq's are allocated, (was removed
>>> in the current initial version of the patch set) both calling the same ISR
>>> via a chained interrupt.
>>>
>>> Works perfectly fine here.
>>
>> Are you referring to the horror that performs interrupt allocations from
>> within the irq_set_type callback? No way. That's beyond disgusting. And
>> potentially broken, as the locks that are being taken were never
>> designed to nest that way.
>>
> No, this horror was the first attempt. In v7 of the patch set this is
> done from the request_resources callback.

Which makes a lot more sense, thanks.

It remains that none of that code supports EDGE_BOTH, so please remove
all traces of it in your next submission. With the right level of
abstraction, you'll be able to add it as a subsequent series, and it
will be easier to review once the basics are out of the way.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 2/9] dt-bindings: add Amlogic Meson GPIO interrupt-controller DT binding documentation

[Heiner Kallweit]

Add dt binding documentation for Amlogic meson GPIO interrupt controller.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Rob Herring <robh@kernel.org>
---
v5:
- added Reviewed-by
v6:
- rename parent-interrupts to interrupts
v7:
- no changes
---
 .../amlogic,meson-gpio-intc.txt                    | 26 ++++++++++++++++++++++
 1 file changed, 26 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 00000000..4c9bb323
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
@@ -0,0 +1,26 @@
+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. The actual number of interrupt exposed depends on the
+SoC.
+
+Required properties:
+
+- compatible : should be "amlogic,meson-gpio-intc".
+- reg : Specifies base physical address and size of the registers.
+- interrupt-controller : Identifies the node as an interrupt controller.
+- #interrupt-cells : should be 2.
+- interrupts : list of GIC interrupts which can be used with the
+	       GPIO IRQ multiplexer
+
+Example:
+
+gpio_intc: interrupt-controller@9880 {
+	compatible = "amlogic,meson-gpio-intc";
+	reg = <0x0 0x09880 0x0 0x10>;
+	interrupt-controller;
+	#interrupt-cells = <2>;
+	interrupts = <64 65 66 67 68 69 70 71>;
+};
-- 
2.13.1

Re: [PATCH v7 2/9] dt-bindings: add Amlogic Meson GPIO interrupt-controller DT binding documentation

[Marc Zyngier]

On 10/06/17 22:57, Heiner Kallweit wrote:
> Add dt binding documentation for Amlogic meson GPIO interrupt controller.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
> Acked-by: Rob Herring <robh@kernel.org>
> ---
> v5:
> - added Reviewed-by
> v6:
> - rename parent-interrupts to interrupts
> v7:
> - no changes
> ---
>  .../amlogic,meson-gpio-intc.txt                    | 26 ++++++++++++++++++++++
>  1 file changed, 26 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 00000000..4c9bb323
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
> @@ -0,0 +1,26 @@
> +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. The actual number of interrupt exposed depends on the
> +SoC.
> +
> +Required properties:
> +
> +- compatible : should be "amlogic,meson-gpio-intc".
> +- reg : Specifies base physical address and size of the registers.
> +- interrupt-controller : Identifies the node as an interrupt controller.
> +- #interrupt-cells : should be 2.
> +- interrupts : list of GIC interrupts which can be used with the
> +	       GPIO IRQ multiplexer
> +
> +Example:
> +
> +gpio_intc: interrupt-controller@9880 {
> +	compatible = "amlogic,meson-gpio-intc";
> +	reg = <0x0 0x09880 0x0 0x10>;
> +	interrupt-controller;
> +	#interrupt-cells = <2>;
> +	interrupts = <64 65 66 67 68 69 70 71>;

What does it mean to have single-cell interrupt specifiers like this,
when the interrupt-parent is a GIC, which has #interrupt-cells set to 3?
Either you use actual interrupt specifiers, or you use another identifier.

Thanks,

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

Re: [PATCH v7 2/9] dt-bindings: add Amlogic Meson GPIO interrupt-controller DT binding documentation

[Heiner Kallweit]

Am 13.06.2017 um 10:53 schrieb Marc Zyngier:
> On 10/06/17 22:57, Heiner Kallweit wrote:
>> Add dt binding documentation for Amlogic meson GPIO interrupt controller.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Reviewed-by: Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>> v5:
>> - added Reviewed-by
>> v6:
>> - rename parent-interrupts to interrupts
>> v7:
>> - no changes
>> ---
>>  .../amlogic,meson-gpio-intc.txt                    | 26 ++++++++++++++++++++++
>>  1 file changed, 26 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 00000000..4c9bb323
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
>> @@ -0,0 +1,26 @@
>> +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. The actual number of interrupt exposed depends on the
>> +SoC.
>> +
>> +Required properties:
>> +
>> +- compatible : should be "amlogic,meson-gpio-intc".
>> +- reg : Specifies base physical address and size of the registers.
>> +- interrupt-controller : Identifies the node as an interrupt controller.
>> +- #interrupt-cells : should be 2.
>> +- interrupts : list of GIC interrupts which can be used with the
>> +	       GPIO IRQ multiplexer
>> +
>> +Example:
>> +
>> +gpio_intc: interrupt-controller@9880 {
>> +	compatible = "amlogic,meson-gpio-intc";
>> +	reg = <0x0 0x09880 0x0 0x10>;
>> +	interrupt-controller;
>> +	#interrupt-cells = <2>;
>> +	interrupts = <64 65 66 67 68 69 70 71>;
> 
> What does it mean to have single-cell interrupt specifiers like this,
> when the interrupt-parent is a GIC, which has #interrupt-cells set to 3?
> Either you use actual interrupt specifiers, or you use another identifier.
> 
The following approaches have been used already:

- standard interrupts property (3-cell): was rejected because the chip doesn't
  actually generate the interrupts but just provides a programmable IRQ routing

- property "parent-interrupts" (1-cell): Rob suggested to use "interrupts" instead

I'd appreciate if you could come to an agreement with Rob, whatever the outcome
is is fine with me.

Rgds, Heiner

> Thanks,
> 
> 	M.
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 2/9] dt-bindings: add Amlogic Meson GPIO interrupt-controller DT binding documentation

[Rob Herring]

On Thu, Jun 15, 2017 at 3:34 AM, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> Am 13.06.2017 um 10:53 schrieb Marc Zyngier:
>> On 10/06/17 22:57, Heiner Kallweit wrote:
>>> Add dt binding documentation for Amlogic meson GPIO interrupt controller.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
>>> Acked-by: Rob Herring <robh@kernel.org>
>>> ---
>>> v5:
>>> - added Reviewed-by
>>> v6:
>>> - rename parent-interrupts to interrupts
>>> v7:
>>> - no changes
>>> ---
>>>  .../amlogic,meson-gpio-intc.txt                    | 26 ++++++++++++++++++++++
>>>  1 file changed, 26 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 00000000..4c9bb323
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/amlogic,meson-gpio-intc.txt
>>> @@ -0,0 +1,26 @@
>>> +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. The actual number of interrupt exposed depends on the
>>> +SoC.
>>> +
>>> +Required properties:
>>> +
>>> +- compatible : should be "amlogic,meson-gpio-intc".
>>> +- reg : Specifies base physical address and size of the registers.
>>> +- interrupt-controller : Identifies the node as an interrupt controller.
>>> +- #interrupt-cells : should be 2.
>>> +- interrupts : list of GIC interrupts which can be used with the
>>> +           GPIO IRQ multiplexer
>>> +
>>> +Example:
>>> +
>>> +gpio_intc: interrupt-controller@9880 {
>>> +    compatible = "amlogic,meson-gpio-intc";
>>> +    reg = <0x0 0x09880 0x0 0x10>;
>>> +    interrupt-controller;
>>> +    #interrupt-cells = <2>;
>>> +    interrupts = <64 65 66 67 68 69 70 71>;
>>
>> What does it mean to have single-cell interrupt specifiers like this,
>> when the interrupt-parent is a GIC, which has #interrupt-cells set to 3?
>> Either you use actual interrupt specifiers, or you use another identifier.
>>
> The following approaches have been used already:
>
> - standard interrupts property (3-cell): was rejected because the chip doesn't
>   actually generate the interrupts but just provides a programmable IRQ routing

So what? That is describing any chained interrupt controller. The
interrupts binding describes interrupt connections, not just interrupt
source endpoints.

> - property "parent-interrupts" (1-cell): Rob suggested to use "interrupts" instead
>
> I'd appreciate if you could come to an agreement with Rob, whatever the outcome
> is is fine with me.

If Marc said not to use standard interrupt binding I can't find it.

Rob

[PATCH v7 3/9] ARM: dts: meson: add GPIO interrupt-controller support

[Heiner Kallweit]

Add support for the GPIO interupt controller of certain Amlogic Meson
Soc's.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
v5:
- split ARM and ARM64 into separate patches
- added Reviewed-by
v6:
- change parent-interrupts to interrupts
v7:
- no changes
---
 arch/arm/boot/dts/meson8.dtsi  | 8 ++++++++
 arch/arm/boot/dts/meson8b.dtsi | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index 69930773..6c3a68af 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -126,6 +126,14 @@
 };
 
 &cbus {
+	gpio_intc: interrupt-controller@9880 {
+		compatible = "amlogic,meson-gpio-intc";
+		reg = <0x9880 0x10>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupts = <64 65 66 67 68 79 70 71>;
+	};
+
 	pinctrl_cbus: pinctrl@9880 {
 		compatible = "amlogic,meson8-cbus-pinctrl";
 		reg = <0x9880 0x10>;
diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index d9f116a4..64d02932 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -112,6 +112,14 @@
 };
 
 &cbus {
+	gpio_intc: interrupt-controller@9880 {
+		compatible = "amlogic,meson-gpio-intc";
+		reg = <0x9880 0x10>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		interrupts = <64 65 66 67 68 79 70 71>;
+	};
+
 	clkc: clock-controller@4000 {
 		#clock-cells = <1>;
 		compatible = "amlogic,meson8b-clkc";
-- 
2.13.1

[PATCH v7 4/9] ARM64: dts: meson: add GPIO interrupt-controller support

[Heiner Kallweit]

Add support for the GPIO interupt controller of certain Amlogic Meson
Soc's.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
v5:
- split ARM and ARM64 into separate patches
- added Reviewed-by
v6:
- change parent-interrupts to interrupts
v7:
- no changes
---
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index 436b8750..87c7403e 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -312,6 +312,14 @@
 				status = "disabled";
 			};
 
+			gpio_intc: interrupt-controller@9880 {
+				compatible = "amlogic,meson-gpio-intc";
+				reg = <0x0 0x09880 0x0 0x10>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
+				interrupts = <64 65 66 67 68 79 70 71>;
+			};
+
 			watchdog@98d0 {
 				compatible = "amlogic,meson-gx-wdt", "amlogic,meson-gxbb-wdt";
 				reg = <0x0 0x098d0 0x0 0x10>;
-- 
2.13.1

[PATCH v7 5/9] gpiolib: export gpiochip_irq_reqres and gpiochip_irq_relres

[Heiner Kallweit]

Customized versions of the request_resources and release_resources
callbacks usually just extend what is available in gpiochip_irq_reqres
and gpiochip_irq_relres. Therefore export them to make them available
to gpio drivers.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/gpio/gpiolib.c      | 6 ++++--
 include/linux/gpio/driver.h | 3 +++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 62ffb4e2..4d666abb 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1643,7 +1643,7 @@ static const struct irq_domain_ops gpiochip_domain_ops = {
 	.xlate	= irq_domain_xlate_twocell,
 };
 
-static int gpiochip_irq_reqres(struct irq_data *d)
+int gpiochip_irq_reqres(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
 
@@ -1659,14 +1659,16 @@ static int gpiochip_irq_reqres(struct irq_data *d)
 	}
 	return 0;
 }
+EXPORT_SYMBOL_GPL(gpiochip_irq_reqres);
 
-static void gpiochip_irq_relres(struct irq_data *d)
+void gpiochip_irq_relres(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(d);
 
 	gpiochip_unlock_as_irq(chip, d->hwirq);
 	module_put(chip->gpiodev->owner);
 }
+EXPORT_SYMBOL_GPL(gpiochip_irq_relres);
 
 static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
 {
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index af20369e..82bc27de 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -262,6 +262,9 @@ int gpiochip_irqchip_add_key(struct gpio_chip *gpiochip,
 			     bool nested,
 			     struct lock_class_key *lock_key);
 
+int gpiochip_irq_reqres(struct irq_data *d);
+void gpiochip_irq_relres(struct irq_data *d);
+
 #ifdef CONFIG_LOCKDEP
 
 /*
-- 
2.13.1

Re: [PATCH v7 5/9] gpiolib: export gpiochip_irq_reqres and gpiochip_irq_relres

[Linus Walleij]

On Sat, Jun 10, 2017 at 11:57 PM, Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Customized versions of the request_resources and release_resources
> callbacks usually just extend what is available in gpiochip_irq_reqres
> and gpiochip_irq_relres. Therefore export them to make them available
> to gpio drivers.
>
> Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

I'm not happy with this patch.

The gpiolib IRQchip abstraction was created to pull common
code into the gpiolib library so I get less mess of GPIO IRQ drivers
spread out all over the place.

This does the reverse and start to spread it all over the place again.

Can you think about a way to try to centralize what you are trying to
do into the gpiolib with a flag or special registration call or
similar?

I guess I don't understand the context, I guess this is part of the
problem Jerome is discussing with me in another thread.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 6/9] pinctrl: meson: add support for GPIO interrupts

[Heiner Kallweit]

Add support for GPIO interrupts and make use of the just introduced
irqchip driver handling the GPIO interrupt-controller.

Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
v5:
- changed Kconfig entry based on Neil's suggestion
- extended comments
- fixed indentation
v6:
- no changes
v7:
- remove IRQ_TYPE_EDGE_BOTH support for now as it requires a
  somehwat hacky workaround
- smaller refactorings
---
 drivers/pinctrl/Kconfig               |   1 +
 drivers/pinctrl/meson/pinctrl-meson.c | 176 +++++++++++++++++++++++++++++++++-
 2 files changed, 176 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 3c3c9d94..8bb99d5a 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -167,6 +167,7 @@ config PINCTRL_MESON
 	select PINCONF
 	select GENERIC_PINCONF
 	select GPIOLIB
+	select GPIOLIB_IRQCHIP
 	select OF_GPIO
 	select REGMAP_MMIO
 
diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
index 66ed70c1..454048d2 100644
--- a/drivers/pinctrl/meson/pinctrl-meson.c
+++ b/drivers/pinctrl/meson/pinctrl-meson.c
@@ -62,6 +62,8 @@
 #include "../pinctrl-utils.h"
 #include "pinctrl-meson.h"
 
+static struct irq_domain *meson_pinctrl_irq_domain;
+
 /**
  * meson_get_bank() - find the bank containing a given pin
  *
@@ -497,6 +499,160 @@ static int meson_gpio_get(struct gpio_chip *chip, unsigned gpio)
 	return !!(val & BIT(bit));
 }
 
+static struct meson_pinctrl *meson_gpio_data_to_pc(struct irq_data *data)
+{
+	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
+
+	return gpiochip_get_data(chip);
+}
+
+static int meson_gpio_bank_hwirq(struct meson_bank *bank, unsigned int offset)
+{
+	int hwirq;
+
+	if (bank->irq_first < 0)
+		/* this bank cannot generate irqs */
+		return -ENOENT;
+
+	hwirq = offset - bank->first + bank->irq_first;
+
+	if (hwirq > bank->irq_last)
+		return  -EINVAL;
+
+	return hwirq;
+}
+
+static int meson_gpio_to_hwirq(struct irq_data *data)
+{
+	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
+	unsigned int offset = data->hwirq + pc->data->pin_base;
+	struct meson_bank *bank;
+	int hwirq, ret;
+
+	ret = meson_get_bank(pc, offset, &bank);
+	if (ret)
+		return ret;
+
+	hwirq = meson_gpio_bank_hwirq(bank, offset);
+	if (hwirq < 0)
+		dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
+
+	return hwirq;
+}
+
+static void meson_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct irq_data *gpio_irq_data = irq_desc_get_handler_data(desc);
+
+	chained_irq_enter(chip, desc);
+
+	if (gpio_irq_data)
+		generic_handle_irq(gpio_irq_data->irq);
+
+	chained_irq_exit(chip, desc);
+}
+
+static void meson_gpio_irq_unmask(struct irq_data *data) {}
+static void meson_gpio_irq_mask(struct irq_data *data) {}
+
+static unsigned int meson_gpio_irq_startup(struct irq_data *data)
+{
+	int hwirq = meson_gpio_to_hwirq(data);
+	int irq = irq_find_mapping(meson_pinctrl_irq_domain, 2 * hwirq);
+
+	irq_set_chained_handler_and_data(irq, meson_gpio_irq_handler, data);
+
+	return 0;
+}
+
+static void meson_gpio_irq_shutdown(struct irq_data *data)
+{
+	int hwirq = meson_gpio_to_hwirq(data);
+	int irq = irq_find_mapping(meson_pinctrl_irq_domain, 2 * hwirq);
+
+	irq_set_chained_handler_and_data(irq, handle_bad_irq, NULL);
+}
+
+static int meson_gpio_create_irq(struct irq_data *data, int hwirq)
+{
+	struct irq_fwspec fwspec;
+
+	fwspec.fwnode = meson_pinctrl_irq_domain->fwnode;
+	fwspec.param_count = 2;
+	fwspec.param[0] = hwirq;
+	fwspec.param[1] = IRQ_TYPE_NONE;
+
+	return irq_create_fwspec_mapping(&fwspec);
+}
+
+static void meson_gpio_delete_irq(int hwirq)
+{
+	int irq = irq_find_mapping(meson_pinctrl_irq_domain, hwirq);
+
+	irq_dispose_mapping(irq);
+}
+
+static int meson_gpio_irq_reqres(struct irq_data *data)
+{
+	int irq, hwirq, ret;
+
+	hwirq = meson_gpio_to_hwirq(data);
+	if (hwirq < 0)
+		return hwirq;
+
+	ret = gpiochip_irq_reqres(data);
+	if (ret)
+		return ret;
+	/*
+	 * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts,
+	 * one for each edge. That's due to HW constraints.
+	 * Future extension:
+	 * Use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have
+	 * one GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by
+	 * shifting hwirq one bit to the right.
+	 * In preperation of this extension use format 2 * GPIO_HWIRQ already.
+	 */
+	irq = meson_gpio_create_irq(data, 2 * hwirq);
+	if (!irq) {
+		gpiochip_irq_relres(data);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void meson_gpio_irq_relres(struct irq_data *data)
+{
+	int hwirq = meson_gpio_to_hwirq(data);
+
+	meson_gpio_delete_irq(2 * hwirq);
+	gpiochip_irq_relres(data);
+}
+
+static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	int hwirq = meson_gpio_to_hwirq(data);
+	int irq = irq_find_mapping(meson_pinctrl_irq_domain, 2 * hwirq);
+
+	/* not supported due to hardware constraints */
+	if (type == IRQ_TYPE_EDGE_BOTH)
+		return -EINVAL;
+
+        return irq_set_irq_type(irq, type);
+}
+
+static struct irq_chip meson_gpio_irq_chip = {
+        .name = "GPIO",
+        .irq_set_type = meson_gpio_irq_set_type,
+        .irq_mask = meson_gpio_irq_mask,
+        .irq_unmask = meson_gpio_irq_unmask,
+        .irq_startup = meson_gpio_irq_startup,
+        .irq_shutdown = meson_gpio_irq_shutdown,
+        .irq_request_resources = meson_gpio_irq_reqres,
+        .irq_release_resources = meson_gpio_irq_relres,
+};
+
 static const struct of_device_id meson_pinctrl_dt_match[] = {
 	{
 		.compatible = "amlogic,meson8-cbus-pinctrl",
@@ -558,7 +714,8 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc)
 		return ret;
 	}
 
-	return 0;
+	return gpiochip_irqchip_add(&pc->chip, &meson_gpio_irq_chip, 0,
+				    handle_simple_irq, IRQ_TYPE_NONE);
 }
 
 static struct regmap_config meson_regmap_config = {
@@ -637,6 +794,23 @@ static int meson_pinctrl_parse_dt(struct meson_pinctrl *pc,
 		return PTR_ERR(pc->reg_gpio);
 	}
 
+	if (!meson_pinctrl_irq_domain) {
+		np = of_find_compatible_node(NULL, NULL, "amlogic,meson-gpio-intc");
+		if (!np) {
+			dev_err(pc->dev, "interrupt controller DT node not found\n");
+			return -EINVAL;
+		}
+
+		meson_pinctrl_irq_domain = irq_find_host(np);
+		if (!meson_pinctrl_irq_domain) {
+			dev_err(pc->dev, "interrupt controller not found\n");
+			of_node_put(np);
+			return -EINVAL;
+		}
+
+		of_node_put(np);
+	}
+
 	return 0;
 }
 
-- 
2.13.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 8/9] ARM: dts: meson: mark gpio controllers as interrupt controllers

[Heiner Kallweit]

Make the GPIO controllers act as interrupt controllers.

Signed-off-by: Heiner Kallweit <hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Reviewed-by: Neil Armstrong <narmstrong-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
---
v5:
- separate ARM / ARM64 DT changes
- add Reviewed-by
v6:
- no changes
v7:
- no changes
---
 arch/arm/boot/dts/meson8.dtsi  | 4 ++++
 arch/arm/boot/dts/meson8b.dtsi | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/meson8.dtsi b/arch/arm/boot/dts/meson8.dtsi
index 6c3a68af..952dad3d 100644
--- a/arch/arm/boot/dts/meson8.dtsi
+++ b/arch/arm/boot/dts/meson8.dtsi
@@ -106,6 +106,8 @@
 			reg-names = "mux", "pull", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_aobus 0 120 16>;
 		};
 
@@ -149,6 +151,8 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_cbus 0 0 120>;
 		};
 
diff --git a/arch/arm/boot/dts/meson8b.dtsi b/arch/arm/boot/dts/meson8b.dtsi
index 64d02932..570a347e 100644
--- a/arch/arm/boot/dts/meson8b.dtsi
+++ b/arch/arm/boot/dts/meson8b.dtsi
@@ -99,6 +99,8 @@
 			reg-names = "mux", "pull", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_aobus 0 130 16>;
 		};
 
@@ -174,6 +176,8 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_cbus 0 0 130>;
 		};
 	};
-- 
2.13.1


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7 7/9] pinctrl: meson: update DT binding documentation

[Heiner Kallweit]

The GPIO controllers act as interrupt controllers now,
therefore update the related DT binding documentation.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
Acked-by: Rob Herring <robh@kernel.org>
---
v5:
- separate documentation update from actual DT update
- add Reviewed-by
v6:
- add Acked-by
v7:
- no changes
---
 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 2392557e..49a8118a 100644
--- a/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/meson,pinctrl.txt
@@ -24,6 +24,8 @@ Required properties for sub-nodes are:
    when it is missing the "pull" registers are used instead
  - gpio-controller: identifies the node as a gpio controller
  - #gpio-cells: must be 2
+ - interrupt-controller: identifies the node as interrupt controller
+ - #interrupt-cells: must be 2
 
 === Other sub-nodes ===
 
@@ -62,6 +64,8 @@ pinctrl-bindings.txt
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
                };
 
 		nand {
-- 
2.13.1

[PATCH v7 9/9] ARM64: dts: meson: mark gpio controllers as interrupt controllers

[Heiner Kallweit]

Make the GPIO controllers act as interrupt controllers.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Reviewed-by: Neil Armstrong <narmstrong@baylibre.com>
---
v5:
- separate ARM / ARM64 DT changes
- add Reviewed-by
v6:
- no changes
v7:
- no changes
---
 arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi | 4 ++++
 arch/arm64/boot/dts/amlogic/meson-gxl.dtsi  | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
index dbd300ff..8cd5821a 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxbb.dtsi
@@ -111,6 +111,8 @@
 			reg-names = "mux", "pull", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_aobus 0 0 14>;
 		};
 
@@ -370,6 +372,8 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_periphs 0 14 120>;
 		};
 
diff --git a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
index 4dfc22b0..237f09f7 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gxl.dtsi
@@ -80,6 +80,8 @@
 			reg-names = "mux", "pull", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_aobus 0 0 14>;
 		};
 
@@ -258,6 +260,8 @@
 			reg-names = "mux", "pull", "pull-enable", "gpio";
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-controller;
+			#interrupt-cells = <2>;
 			gpio-ranges = <&pinctrl_periphs 0 10 101>;
 		};
 
-- 
2.13.1