[v2,6/6] pintrl: meson: add support for GPIO interrupts

Add support for GPIO interrupts on Amlogic Meson SoC's.

There's a limit of 8 parent interupts which can be used in total.
Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
one for each edge.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- make the GPIO IRQ controller a separate driver
- several smaller improvements
---
 drivers/pinctrl/Kconfig                   |   1 +
 drivers/pinctrl/meson/Makefile            |   2 +-
 drivers/pinctrl/meson/pinctrl-meson-irq.c | 367 ++++++++++++++++++++++++++++++
 drivers/pinctrl/meson/pinctrl-meson.c     |   8 +-
 drivers/pinctrl/meson/pinctrl-meson.h     |   1 +
 5 files changed, 377 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pinctrl/meson/pinctrl-meson-irq.c

On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
> Add support for GPIO interrupts on Amlogic Meson SoC's.
> 
> There's a limit of 8 parent interupts which can be used in total.
> Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
> one for each edge.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - make the GPIO IRQ controller a separate driver
> - several smaller improvements
> ---
>  drivers/pinctrl/Kconfig                   |   1 +
>  drivers/pinctrl/meson/Makefile            |   2 +-
>  drivers/pinctrl/meson/pinctrl-meson-irq.c | 367 ++++++++++++++++++++++++++++++

Hi Heiner,

This code has nothing to do with GPIOs on pinmux handling here, what we figured with Jerome
is that this must be an independent IRQ Controller in drivers/irqchip.

Please move it and make independent, you should be able to request irqs without any links
to the pinmux/gpio since physically the GPIO lines input are always connected to this
irq controller, and the pinmux has no impact on the interrupt management here.
From the GPIO-IRQ Controller perspective, the GPIOs are only a number and the pinmux code
is only here to make the translation to a specific GPIO to a GPIO-IRQ number.

For more chance to have it upstreamed in the right way, we should :
1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype, forums, ...
2) Push an independent IRQ controller that matches the capacity of the HW
3) Push a link from the pinctrl driver to have the to_gpio_irq mapping done in the right way

Jerome spent quite a lot of time and had a chat with the IRQ subsystem maintainers to have s
clear image of how this should be implemented, and it would be a good point to actually
have a chat with them to elaborate find a strong solution.

I'm sorry to say that pushing this code without really understanding how and why will lead to
nothing expect frustration from everybody.

Neil

>  drivers/pinctrl/meson/pinctrl-meson.c     |   8 +-
>  drivers/pinctrl/meson/pinctrl-meson.h     |   1 +
>  5 files changed, 377 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/pinctrl/meson/pinctrl-meson-irq.c
> 
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 37af5e30..f8f401a0 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -153,6 +153,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/Makefile b/drivers/pinctrl/meson/Makefile
> index 27c5b512..827e416d 100644
> --- a/drivers/pinctrl/meson/Makefile
> +++ b/drivers/pinctrl/meson/Makefile
> @@ -1,3 +1,3 @@
>  obj-y	+= pinctrl-meson8.o pinctrl-meson8b.o
>  obj-y	+= pinctrl-meson-gxbb.o pinctrl-meson-gxl.o
> -obj-y	+= pinctrl-meson.o
> +obj-y	+= pinctrl-meson.o pinctrl-meson-irq.o
> diff --git a/drivers/pinctrl/meson/pinctrl-meson-irq.c b/drivers/pinctrl/meson/pinctrl-meson-irq.c
> new file mode 100644
> index 00000000..c5f403f3
> --- /dev/null
> +++ b/drivers/pinctrl/meson/pinctrl-meson-irq.c
> @@ -0,0 +1,367 @@
> +/*
> + * Amlogic Meson GPIO IRQ driver
> + *
> + * Copyright 2017 Heiner Kallweit <hkallweit1@gmail.com>
> + * Based on a first version by Jerome Brunet <jbrunet@baylibre.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/gpio.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include "pinctrl-meson.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 MESON_GPIO_MAX_PARENT_IRQ_NUM	8
> +
> +struct meson_gpio_irq_slot {
> +	int irq;
> +	int owner;
> +};
> +
> +static struct regmap *meson_gpio_irq_regmap;
> +static struct meson_gpio_irq_slot
> +		meson_gpio_irq_slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
> +static int meson_gpio_num_irq_slots;
> +static DEFINE_MUTEX(meson_gpio_irq_slot_mutex);
> +static DECLARE_BITMAP(meson_gpio_irq_locked, 256);
> +
> +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_to_hwirq(struct meson_bank *bank, unsigned int offset)
> +{
> +	int hwirq;
> +
> +	if (bank->irq_first < 0)
> +		/* this bank cannot generate irqs */
> +		return 0;
> +
> +	hwirq = offset - bank->first + bank->irq_first;
> +
> +	if (hwirq > bank->irq_last)
> +		/* this pin cannot generate irqs */
> +		return 0;
> +
> +	return hwirq;
> +}
> +
> +static int meson_gpio_to_irq(struct meson_pinctrl *pc, unsigned int offset)
> +{
> +	struct meson_bank *bank;
> +	int hwirq;
> +
> +	offset += pc->data->pin_base;
> +
> +	bank = meson_pinctrl_get_bank(pc, offset);
> +	if (IS_ERR(bank))
> +		return PTR_ERR(bank);
> +
> +	hwirq = meson_gpio_to_hwirq(bank, offset);
> +	if (!hwirq)
> +		dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
> +
> +	return hwirq;
> +}
> +
> +static int meson_gpio_data_to_hwirq(struct irq_data *data)
> +{
> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
> +	unsigned gpio = irqd_to_hwirq(data);
> +
> +	return meson_gpio_to_irq(pc, gpio);
> +}
> +
> +static irqreturn_t meson_gpio_irq_handler(int irq, void *data)
> +{
> +	struct irq_data *gpio_irqdata = data;
> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
> +	int hwirq = meson_gpio_data_to_hwirq(gpio_irqdata);
> +
> +	/*
> +	 * For some strange reason spurious interrupts created by the chip when
> +	 * the interrupt source registers are written cause a deadlock here.
> +	 * generic_handle_irq calls handle_simple_irq which tries to get
> +	 * spinlock desc->lock. This interrupt handler is called whilst
> +	 * __setup_irq holds desc->lock.
> +	 * The deadlock means that both are running on the same CPU what should
> +	 * not happen as __setup_irq called raw_spin_lock_irqsave thus disabling
> +	 * interrupts on this CPU.
> +	 * Work around this by ignoring interrupts in code protected by
> +	 * chip_bus_lock (__setup_irq/__free_irq for the respective GPIO hwirq).
> +	 */
> +	if (test_bit(hwirq, meson_gpio_irq_locked))
> +		dev_dbg(pc->dev, "spurious interrupt detected!\n");
> +	else
> +		generic_handle_irq(gpio_irqdata->irq);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int meson_gpio_alloc_irq_slot(struct irq_data *data, int num_slots,
> +				     int *slots)
> +{
> +	int i, cnt = 0;
> +
> +	mutex_lock(&meson_gpio_irq_slot_mutex);
> +
> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
> +		if (!meson_gpio_irq_slots[i].owner) {
> +			meson_gpio_irq_slots[i].owner = data->irq;
> +			slots[cnt++] = i;
> +			if (cnt == num_slots)
> +				break;
> +		}
> +
> +	if (cnt < num_slots)
> +		for (i = 0; i < cnt; i++)
> +			meson_gpio_irq_slots[i].owner = 0;
> +
> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
> +
> +	return cnt == num_slots ? 0 : -ENOSPC;
> +}
> +
> +static void meson_gpio_free_irq_slot(struct irq_data *data)
> +{
> +	int i;
> +
> +	mutex_lock(&meson_gpio_irq_slot_mutex);
> +
> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
> +		if (meson_gpio_irq_slots[i].owner == data->irq) {
> +			free_irq(meson_gpio_irq_slots[i].irq, data);
> +			meson_gpio_irq_slots[i].owner = 0;
> +		}
> +
> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
> +}
> +
> +static int meson_gpio_find_irq_slot(struct irq_data *data, int *slots)
> +{
> +	int i, cnt = 0;
> +
> +	mutex_lock(&meson_gpio_irq_slot_mutex);
> +
> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
> +		if (meson_gpio_irq_slots[i].owner == data->irq)
> +			slots[cnt++] = i;
> +
> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
> +
> +	return cnt ?: -EINVAL;
> +}
> +
> +static void meson_gpio_set_hwirq(int idx, int hwirq)
> +{
> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
> +	int shift = 8 * (idx % 4);
> +
> +	regmap_update_bits(meson_gpio_irq_regmap, reg, 0xff << shift,
> +			   hwirq << shift);
> +}
> +
> +static void meson_gpio_irq_set_hwirq(struct irq_data *data, int hwirq)
> +{
> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
> +	int i, cnt, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
> +
> +	cnt = meson_gpio_find_irq_slot(data, slots);
> +	if (cnt < 0) {
> +		dev_err(pc->dev, "didn't find gpio irq slot\n");
> +		return;
> +	}
> +
> +	for (i = 0; i < cnt; i++)
> +		meson_gpio_set_hwirq(slots[i], hwirq);
> +}
> +
> +static void meson_gpio_irq_unmask(struct irq_data *data)
> +{
> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
> +	unsigned gpio = irqd_to_hwirq(data);
> +	int hwirq = meson_gpio_to_irq(pc, gpio);
> +
> +	meson_gpio_irq_set_hwirq(data, hwirq);
> +}
> +
> +static void meson_gpio_irq_mask(struct irq_data *data)
> +{
> +	meson_gpio_irq_set_hwirq(data, 0xff);
> +}
> +
> +static void meson_gpio_irq_shutdown(struct irq_data *data)
> +{
> +	meson_gpio_irq_mask(data);
> +	meson_gpio_free_irq_slot(data);
> +}
> +
> +static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> +	int i, ret, irq, num_slots, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
> +	unsigned int val = 0;
> +
> +	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
> +	ret = meson_gpio_alloc_irq_slot(data, num_slots, slots);
> +	if (ret)
> +		return ret;
> +
> +	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
> +		val |= REG_EDGE_POL_EDGE(slots[0]);
> +
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
> +		val |= REG_EDGE_POL_LOW(slots[0]);
> +
> +	regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
> +			   REG_EDGE_POL_MASK(slots[0]), val);
> +
> +	/*
> +	 * The chip can create an interrupt for either rising or falling edge
> +	 * only. Therefore use two interrupts in case of IRQ_TYPE_EDGE_BOTH,
> +	 * first for falling edge and second one for rising edge.
> +	 */
> +	if (num_slots > 1) {
> +		val = REG_EDGE_POL_EDGE(slots[1]);
> +		regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
> +				   REG_EDGE_POL_MASK(slots[1]), val);
> +	}
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		val = IRQ_TYPE_EDGE_RISING;
> +	else
> +		val = IRQ_TYPE_LEVEL_HIGH;
> +
> +	for (i = 0; i < num_slots; i++) {
> +		irq = meson_gpio_irq_slots[slots[i]].irq;
> +		ret = irq_set_irq_type(irq, val);
> +		if (ret)
> +			break;
> +		ret = request_irq(irq, meson_gpio_irq_handler, 0,
> +				  "GPIO parent", data);
> +		if (ret)
> +			break;
> +	}
> +
> +	if (ret)
> +		while (--i >= 0)
> +			free_irq(meson_gpio_irq_slots[slots[i]].irq, data);
> +
> +	return ret;
> +}
> +
> +static void meson_gpio_irq_bus_lock(struct irq_data *data)
> +{
> +	int hwirq = meson_gpio_data_to_hwirq(data);
> +
> +	set_bit(hwirq, meson_gpio_irq_locked);
> +}
> +
> +static void meson_gpio_irq_bus_sync_unlock(struct irq_data *data)
> +{
> +	int hwirq = meson_gpio_data_to_hwirq(data);
> +
> +	clear_bit(hwirq, meson_gpio_irq_locked);
> +}
> +
> +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_shutdown = meson_gpio_irq_shutdown,
> +	.irq_bus_lock = meson_gpio_irq_bus_lock,
> +	.irq_bus_sync_unlock = meson_gpio_irq_bus_sync_unlock,
> +};
> +
> +static int meson_gpio_get_irqs(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	int irq, i;
> +
> +	for (i = 0; i < MESON_GPIO_MAX_PARENT_IRQ_NUM; i++) {
> +		irq = irq_of_parse_and_map(np, i);
> +		if (!irq)
> +			break;
> +		meson_gpio_irq_slots[i].irq = irq;
> +	}
> +
> +	meson_gpio_num_irq_slots = i;
> +
> +	return i ? 0 : -EINVAL;
> +}
> +
> +static const struct regmap_config meson_gpio_regmap_config = {
> +	.reg_bits       = 32,
> +	.reg_stride     = 4,
> +	.val_bits       = 32,
> +	.max_register	= REG_FILTER_SEL,
> +};
> +
> +static int meson_gpio_irq_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	void __iomem *io_base;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	io_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	meson_gpio_irq_regmap = devm_regmap_init_mmio(&pdev->dev, io_base,
> +						&meson_gpio_regmap_config);
> +	if (IS_ERR(meson_gpio_irq_regmap))
> +		return PTR_ERR(meson_gpio_irq_regmap);
> +
> +	/* initialize to IRQ_TYPE_LEVEL_HIGH */
> +	regmap_write(meson_gpio_irq_regmap, REG_EDGE_POL, 0);
> +	/* disable all GPIO interrupt sources */
> +	regmap_write(meson_gpio_irq_regmap, REG_PIN_03_SEL, 0xffffffff);
> +	regmap_write(meson_gpio_irq_regmap, REG_PIN_47_SEL, 0xffffffff);
> +	/* disable filtering */
> +	regmap_write(meson_gpio_irq_regmap, REG_FILTER_SEL, 0);
> +
> +	ret = meson_gpio_get_irqs(pdev);
> +	if (ret)
> +		return ret;
> +
> +	meson_pinctrl_irq_chip = &meson_gpio_irq_chip;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id meson_gpio_irq_dt_match[] = {
> +	{ .compatible = "amlogic,meson-gpio-interrupt" },
> +	{ },
> +};
> +
> +static struct platform_driver meson_gpio_irq_driver = {
> +	.probe		= meson_gpio_irq_probe,
> +	.driver = {
> +		.name	= "meson-gpio-interrupt",
> +		.of_match_table = meson_gpio_irq_dt_match,
> +	},
> +};
> +builtin_platform_driver(meson_gpio_irq_driver);
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
> index 39ad9861..c587f6f0 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"
>  
> +struct irq_chip *meson_pinctrl_irq_chip;
> +
>  /**
>   * meson_pinctrl_get_bank() - find the bank containing a given pin
>   *
> @@ -551,7 +553,8 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc)
>  		return ret;
>  	}
>  
> -	return 0;
> +	return gpiochip_irqchip_add(&pc->chip, meson_pinctrl_irq_chip, 0,
> +				    handle_simple_irq, IRQ_TYPE_NONE);
>  }
>  
>  static struct regmap_config meson_regmap_config = {
> @@ -640,6 +643,9 @@ static int meson_pinctrl_probe(struct platform_device *pdev)
>  	struct meson_pinctrl *pc;
>  	int ret;
>  
> +	if (!meson_pinctrl_irq_chip)
> +		return -EPROBE_DEFER;
> +
>  	pc = devm_kzalloc(dev, sizeof(struct meson_pinctrl), GFP_KERNEL);
>  	if (!pc)
>  		return -ENOMEM;
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
> index 40b56aff..16aab328 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.h
> +++ b/drivers/pinctrl/meson/pinctrl-meson.h
> @@ -176,6 +176,7 @@ extern struct meson_pinctrl_data meson_gxbb_periphs_pinctrl_data;
>  extern struct meson_pinctrl_data meson_gxbb_aobus_pinctrl_data;
>  extern struct meson_pinctrl_data meson_gxl_periphs_pinctrl_data;
>  extern struct meson_pinctrl_data meson_gxl_aobus_pinctrl_data;
> +extern struct irq_chip *meson_pinctrl_irq_chip;
>  
>  struct meson_bank *meson_pinctrl_get_bank(const struct meson_pinctrl *pc,
>  					  unsigned int pin);
> 

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

Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
> On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
>> Add support for GPIO interrupts on Amlogic Meson SoC's.
>>
>> There's a limit of 8 parent interupts which can be used in total.
>> Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
>> one for each edge.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v2:
>> - make the GPIO IRQ controller a separate driver
>> - several smaller improvements
>> ---
>>  drivers/pinctrl/Kconfig                   |   1 +
>>  drivers/pinctrl/meson/Makefile            |   2 +-
>>  drivers/pinctrl/meson/pinctrl-meson-irq.c | 367 ++++++++++++++++++++++++++++++
> 
> Hi Heiner,
> 
> This code has nothing to do with GPIOs on pinmux handling here, what we figured with Jerome
> is that this must be an independent IRQ Controller in drivers/irqchip.
> 
I'm not convinced and would like to hear more opinions on that. I see it like this:
The driver implements an irqchip, right. But it's not an interrupt controller.
Due to using GPIOLIB_IRQCHIP the gpio controller now has an own irq domain (one for ao
and one for periphs GPIO domain). Therefore the gpio-controller now also acts as
interrupt-controller. And both gpio (and interrupt) controllers just use the irqchip
exposed by the new driver.
Last but not least the irqchip can be used with GPIOs only.

In the irqchip implementation we need the SoC-specific mapping from GPIO number
to internal GPIO IRQ number. Having to export this from drivers/pinctrl/meson for use
under drivers/irqchip most likely would also cause objections.

So far my impression is that the very specific way GPIO IRQ's are handled on Meson
doesn't fit perfectly into the current IRQ subsystem. Therefore the discussion
about Jerome's version didn't result in the IRQ maintainers stating: do it this way ..
Having said that most likely every possible approach is going to raise some concerns.

> Please move it and make independent, you should be able to request irqs without any links
> to the pinmux/gpio since physically the GPIO lines input are always connected to this
> irq controller, and the pinmux has no impact on the interrupt management here.
>>From the GPIO-IRQ Controller perspective, the GPIOs are only a number and the pinmux code
> is only here to make the translation to a specific GPIO to a GPIO-IRQ number.
> 
> For more chance to have it upstreamed in the right way, we should :
> 1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype, forums, ...
> 2) Push an independent IRQ controller that matches the capacity of the HW
> 3) Push a link from the pinctrl driver to have the to_gpio_irq mapping done in the right way
> 
> Jerome spent quite a lot of time and had a chat with the IRQ subsystem maintainers to have s
> clear image of how this should be implemented, and it would be a good point to actually
> have a chat with them to elaborate find a strong solution.
> 
I know and I really appreciate Jerome's work and his discussion with the IRQ maintainers.
My current attempt was inspired by his work.
However the discussion last year ended w/o result and the topic of GPIO IRQs has been dead
since then. And I think discussing approaches works best based on a concrete piece of code.
Therefore I submitted my version as discussion basis. I didn't expect that everybody would
be totally happy with it and it would go to mainline unchanged.

> I'm sorry to say that pushing this code without really understanding how and why will lead to
> nothing expect frustration from everybody.
> 
I can only speak for myself: I'm not frustrated and I can live with critical review comments.

Regards, Heiner

> Neil
> 
>>  drivers/pinctrl/meson/pinctrl-meson.c     |   8 +-
>>  drivers/pinctrl/meson/pinctrl-meson.h     |   1 +
>>  5 files changed, 377 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/pinctrl/meson/pinctrl-meson-irq.c
>>
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>> index 37af5e30..f8f401a0 100644
>> --- a/drivers/pinctrl/Kconfig
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -153,6 +153,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/Makefile b/drivers/pinctrl/meson/Makefile
>> index 27c5b512..827e416d 100644
>> --- a/drivers/pinctrl/meson/Makefile
>> +++ b/drivers/pinctrl/meson/Makefile
>> @@ -1,3 +1,3 @@
>>  obj-y	+= pinctrl-meson8.o pinctrl-meson8b.o
>>  obj-y	+= pinctrl-meson-gxbb.o pinctrl-meson-gxl.o
>> -obj-y	+= pinctrl-meson.o
>> +obj-y	+= pinctrl-meson.o pinctrl-meson-irq.o
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-irq.c b/drivers/pinctrl/meson/pinctrl-meson-irq.c
>> new file mode 100644
>> index 00000000..c5f403f3
>> --- /dev/null
>> +++ b/drivers/pinctrl/meson/pinctrl-meson-irq.c
>> @@ -0,0 +1,367 @@
>> +/*
>> + * Amlogic Meson GPIO IRQ driver
>> + *
>> + * Copyright 2017 Heiner Kallweit <hkallweit1@gmail.com>
>> + * Based on a first version by Jerome Brunet <jbrunet@baylibre.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/gpio.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include "pinctrl-meson.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 MESON_GPIO_MAX_PARENT_IRQ_NUM	8
>> +
>> +struct meson_gpio_irq_slot {
>> +	int irq;
>> +	int owner;
>> +};
>> +
>> +static struct regmap *meson_gpio_irq_regmap;
>> +static struct meson_gpio_irq_slot
>> +		meson_gpio_irq_slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
>> +static int meson_gpio_num_irq_slots;
>> +static DEFINE_MUTEX(meson_gpio_irq_slot_mutex);
>> +static DECLARE_BITMAP(meson_gpio_irq_locked, 256);
>> +
>> +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_to_hwirq(struct meson_bank *bank, unsigned int offset)
>> +{
>> +	int hwirq;
>> +
>> +	if (bank->irq_first < 0)
>> +		/* this bank cannot generate irqs */
>> +		return 0;
>> +
>> +	hwirq = offset - bank->first + bank->irq_first;
>> +
>> +	if (hwirq > bank->irq_last)
>> +		/* this pin cannot generate irqs */
>> +		return 0;
>> +
>> +	return hwirq;
>> +}
>> +
>> +static int meson_gpio_to_irq(struct meson_pinctrl *pc, unsigned int offset)
>> +{
>> +	struct meson_bank *bank;
>> +	int hwirq;
>> +
>> +	offset += pc->data->pin_base;
>> +
>> +	bank = meson_pinctrl_get_bank(pc, offset);
>> +	if (IS_ERR(bank))
>> +		return PTR_ERR(bank);
>> +
>> +	hwirq = meson_gpio_to_hwirq(bank, offset);
>> +	if (!hwirq)
>> +		dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
>> +
>> +	return hwirq;
>> +}
>> +
>> +static int meson_gpio_data_to_hwirq(struct irq_data *data)
>> +{
>> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
>> +	unsigned gpio = irqd_to_hwirq(data);
>> +
>> +	return meson_gpio_to_irq(pc, gpio);
>> +}
>> +
>> +static irqreturn_t meson_gpio_irq_handler(int irq, void *data)
>> +{
>> +	struct irq_data *gpio_irqdata = data;
>> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
>> +	int hwirq = meson_gpio_data_to_hwirq(gpio_irqdata);
>> +
>> +	/*
>> +	 * For some strange reason spurious interrupts created by the chip when
>> +	 * the interrupt source registers are written cause a deadlock here.
>> +	 * generic_handle_irq calls handle_simple_irq which tries to get
>> +	 * spinlock desc->lock. This interrupt handler is called whilst
>> +	 * __setup_irq holds desc->lock.
>> +	 * The deadlock means that both are running on the same CPU what should
>> +	 * not happen as __setup_irq called raw_spin_lock_irqsave thus disabling
>> +	 * interrupts on this CPU.
>> +	 * Work around this by ignoring interrupts in code protected by
>> +	 * chip_bus_lock (__setup_irq/__free_irq for the respective GPIO hwirq).
>> +	 */
>> +	if (test_bit(hwirq, meson_gpio_irq_locked))
>> +		dev_dbg(pc->dev, "spurious interrupt detected!\n");
>> +	else
>> +		generic_handle_irq(gpio_irqdata->irq);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int meson_gpio_alloc_irq_slot(struct irq_data *data, int num_slots,
>> +				     int *slots)
>> +{
>> +	int i, cnt = 0;
>> +
>> +	mutex_lock(&meson_gpio_irq_slot_mutex);
>> +
>> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
>> +		if (!meson_gpio_irq_slots[i].owner) {
>> +			meson_gpio_irq_slots[i].owner = data->irq;
>> +			slots[cnt++] = i;
>> +			if (cnt == num_slots)
>> +				break;
>> +		}
>> +
>> +	if (cnt < num_slots)
>> +		for (i = 0; i < cnt; i++)
>> +			meson_gpio_irq_slots[i].owner = 0;
>> +
>> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
>> +
>> +	return cnt == num_slots ? 0 : -ENOSPC;
>> +}
>> +
>> +static void meson_gpio_free_irq_slot(struct irq_data *data)
>> +{
>> +	int i;
>> +
>> +	mutex_lock(&meson_gpio_irq_slot_mutex);
>> +
>> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
>> +		if (meson_gpio_irq_slots[i].owner == data->irq) {
>> +			free_irq(meson_gpio_irq_slots[i].irq, data);
>> +			meson_gpio_irq_slots[i].owner = 0;
>> +		}
>> +
>> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
>> +}
>> +
>> +static int meson_gpio_find_irq_slot(struct irq_data *data, int *slots)
>> +{
>> +	int i, cnt = 0;
>> +
>> +	mutex_lock(&meson_gpio_irq_slot_mutex);
>> +
>> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
>> +		if (meson_gpio_irq_slots[i].owner == data->irq)
>> +			slots[cnt++] = i;
>> +
>> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
>> +
>> +	return cnt ?: -EINVAL;
>> +}
>> +
>> +static void meson_gpio_set_hwirq(int idx, int hwirq)
>> +{
>> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
>> +	int shift = 8 * (idx % 4);
>> +
>> +	regmap_update_bits(meson_gpio_irq_regmap, reg, 0xff << shift,
>> +			   hwirq << shift);
>> +}
>> +
>> +static void meson_gpio_irq_set_hwirq(struct irq_data *data, int hwirq)
>> +{
>> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
>> +	int i, cnt, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
>> +
>> +	cnt = meson_gpio_find_irq_slot(data, slots);
>> +	if (cnt < 0) {
>> +		dev_err(pc->dev, "didn't find gpio irq slot\n");
>> +		return;
>> +	}
>> +
>> +	for (i = 0; i < cnt; i++)
>> +		meson_gpio_set_hwirq(slots[i], hwirq);
>> +}
>> +
>> +static void meson_gpio_irq_unmask(struct irq_data *data)
>> +{
>> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
>> +	unsigned gpio = irqd_to_hwirq(data);
>> +	int hwirq = meson_gpio_to_irq(pc, gpio);
>> +
>> +	meson_gpio_irq_set_hwirq(data, hwirq);
>> +}
>> +
>> +static void meson_gpio_irq_mask(struct irq_data *data)
>> +{
>> +	meson_gpio_irq_set_hwirq(data, 0xff);
>> +}
>> +
>> +static void meson_gpio_irq_shutdown(struct irq_data *data)
>> +{
>> +	meson_gpio_irq_mask(data);
>> +	meson_gpio_free_irq_slot(data);
>> +}
>> +
>> +static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +	int i, ret, irq, num_slots, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
>> +	unsigned int val = 0;
>> +
>> +	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
>> +	ret = meson_gpio_alloc_irq_slot(data, num_slots, slots);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
>> +		val |= REG_EDGE_POL_EDGE(slots[0]);
>> +
>> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
>> +		val |= REG_EDGE_POL_LOW(slots[0]);
>> +
>> +	regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
>> +			   REG_EDGE_POL_MASK(slots[0]), val);
>> +
>> +	/*
>> +	 * The chip can create an interrupt for either rising or falling edge
>> +	 * only. Therefore use two interrupts in case of IRQ_TYPE_EDGE_BOTH,
>> +	 * first for falling edge and second one for rising edge.
>> +	 */
>> +	if (num_slots > 1) {
>> +		val = REG_EDGE_POL_EDGE(slots[1]);
>> +		regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
>> +				   REG_EDGE_POL_MASK(slots[1]), val);
>> +	}
>> +
>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>> +		val = IRQ_TYPE_EDGE_RISING;
>> +	else
>> +		val = IRQ_TYPE_LEVEL_HIGH;
>> +
>> +	for (i = 0; i < num_slots; i++) {
>> +		irq = meson_gpio_irq_slots[slots[i]].irq;
>> +		ret = irq_set_irq_type(irq, val);
>> +		if (ret)
>> +			break;
>> +		ret = request_irq(irq, meson_gpio_irq_handler, 0,
>> +				  "GPIO parent", data);
>> +		if (ret)
>> +			break;
>> +	}
>> +
>> +	if (ret)
>> +		while (--i >= 0)
>> +			free_irq(meson_gpio_irq_slots[slots[i]].irq, data);
>> +
>> +	return ret;
>> +}
>> +
>> +static void meson_gpio_irq_bus_lock(struct irq_data *data)
>> +{
>> +	int hwirq = meson_gpio_data_to_hwirq(data);
>> +
>> +	set_bit(hwirq, meson_gpio_irq_locked);
>> +}
>> +
>> +static void meson_gpio_irq_bus_sync_unlock(struct irq_data *data)
>> +{
>> +	int hwirq = meson_gpio_data_to_hwirq(data);
>> +
>> +	clear_bit(hwirq, meson_gpio_irq_locked);
>> +}
>> +
>> +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_shutdown = meson_gpio_irq_shutdown,
>> +	.irq_bus_lock = meson_gpio_irq_bus_lock,
>> +	.irq_bus_sync_unlock = meson_gpio_irq_bus_sync_unlock,
>> +};
>> +
>> +static int meson_gpio_get_irqs(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	int irq, i;
>> +
>> +	for (i = 0; i < MESON_GPIO_MAX_PARENT_IRQ_NUM; i++) {
>> +		irq = irq_of_parse_and_map(np, i);
>> +		if (!irq)
>> +			break;
>> +		meson_gpio_irq_slots[i].irq = irq;
>> +	}
>> +
>> +	meson_gpio_num_irq_slots = i;
>> +
>> +	return i ? 0 : -EINVAL;
>> +}
>> +
>> +static const struct regmap_config meson_gpio_regmap_config = {
>> +	.reg_bits       = 32,
>> +	.reg_stride     = 4,
>> +	.val_bits       = 32,
>> +	.max_register	= REG_FILTER_SEL,
>> +};
>> +
>> +static int meson_gpio_irq_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	void __iomem *io_base;
>> +	int ret;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	io_base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(io_base))
>> +		return PTR_ERR(io_base);
>> +
>> +	meson_gpio_irq_regmap = devm_regmap_init_mmio(&pdev->dev, io_base,
>> +						&meson_gpio_regmap_config);
>> +	if (IS_ERR(meson_gpio_irq_regmap))
>> +		return PTR_ERR(meson_gpio_irq_regmap);
>> +
>> +	/* initialize to IRQ_TYPE_LEVEL_HIGH */
>> +	regmap_write(meson_gpio_irq_regmap, REG_EDGE_POL, 0);
>> +	/* disable all GPIO interrupt sources */
>> +	regmap_write(meson_gpio_irq_regmap, REG_PIN_03_SEL, 0xffffffff);
>> +	regmap_write(meson_gpio_irq_regmap, REG_PIN_47_SEL, 0xffffffff);
>> +	/* disable filtering */
>> +	regmap_write(meson_gpio_irq_regmap, REG_FILTER_SEL, 0);
>> +
>> +	ret = meson_gpio_get_irqs(pdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	meson_pinctrl_irq_chip = &meson_gpio_irq_chip;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id meson_gpio_irq_dt_match[] = {
>> +	{ .compatible = "amlogic,meson-gpio-interrupt" },
>> +	{ },
>> +};
>> +
>> +static struct platform_driver meson_gpio_irq_driver = {
>> +	.probe		= meson_gpio_irq_probe,
>> +	.driver = {
>> +		.name	= "meson-gpio-interrupt",
>> +		.of_match_table = meson_gpio_irq_dt_match,
>> +	},
>> +};
>> +builtin_platform_driver(meson_gpio_irq_driver);
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c b/drivers/pinctrl/meson/pinctrl-meson.c
>> index 39ad9861..c587f6f0 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"
>>  
>> +struct irq_chip *meson_pinctrl_irq_chip;
>> +
>>  /**
>>   * meson_pinctrl_get_bank() - find the bank containing a given pin
>>   *
>> @@ -551,7 +553,8 @@ static int meson_gpiolib_register(struct meson_pinctrl *pc)
>>  		return ret;
>>  	}
>>  
>> -	return 0;
>> +	return gpiochip_irqchip_add(&pc->chip, meson_pinctrl_irq_chip, 0,
>> +				    handle_simple_irq, IRQ_TYPE_NONE);
>>  }
>>  
>>  static struct regmap_config meson_regmap_config = {
>> @@ -640,6 +643,9 @@ static int meson_pinctrl_probe(struct platform_device *pdev)
>>  	struct meson_pinctrl *pc;
>>  	int ret;
>>  
>> +	if (!meson_pinctrl_irq_chip)
>> +		return -EPROBE_DEFER;
>> +
>>  	pc = devm_kzalloc(dev, sizeof(struct meson_pinctrl), GFP_KERNEL);
>>  	if (!pc)
>>  		return -ENOMEM;
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h b/drivers/pinctrl/meson/pinctrl-meson.h
>> index 40b56aff..16aab328 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson.h
>> +++ b/drivers/pinctrl/meson/pinctrl-meson.h
>> @@ -176,6 +176,7 @@ extern struct meson_pinctrl_data meson_gxbb_periphs_pinctrl_data;
>>  extern struct meson_pinctrl_data meson_gxbb_aobus_pinctrl_data;
>>  extern struct meson_pinctrl_data meson_gxl_periphs_pinctrl_data;
>>  extern struct meson_pinctrl_data meson_gxl_aobus_pinctrl_data;
>> +extern struct irq_chip *meson_pinctrl_irq_chip;
>>  
>>  struct meson_bank *meson_pinctrl_get_bank(const struct meson_pinctrl *pc,
>>  					  unsigned int pin);
>>
> 
> 

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

Hi Heiner,

On 05/15/2017 09:00 PM, Heiner Kallweit wrote:
> Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
>> On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
>>> Add support for GPIO interrupts on Amlogic Meson SoC's.
>>>
>>> There's a limit of 8 parent interupts which can be used in total.
>>> Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
>>> one for each edge.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> v2:
>>> - make the GPIO IRQ controller a separate driver
>>> - several smaller improvements
>>> ---
>>>  drivers/pinctrl/Kconfig                   |   1 +
>>>  drivers/pinctrl/meson/Makefile            |   2 +-
>>>  drivers/pinctrl/meson/pinctrl-meson-irq.c | 367 ++++++++++++++++++++++++++++++
>>
>> Hi Heiner,
>>
>> This code has nothing to do with GPIOs on pinmux handling here, what we figured with Jerome
>> is that this must be an independent IRQ Controller in drivers/irqchip.
>>
> I'm not convinced and would like to hear more opinions on that. I see it like this:
> The driver implements an irqchip, right. But it's not an interrupt controller.
> Due to using GPIOLIB_IRQCHIP the gpio controller now has an own irq domain (one for ao
> and one for periphs GPIO domain). Therefore the gpio-controller now also acts as
> interrupt-controller. And both gpio (and interrupt) controllers just use the irqchip
> exposed by the new driver.
> Last but not least the irqchip can be used with GPIOs only.

In fact it's an interrupt controller since it can mask/unmask and control a singal that
can wake up an interrupt for the ARM Core.

Please look at the STM32 EXTI code, the design is quite similar except they don't have a
dynamic management of links, but fixed ones.
They have a proper independant IRQCHIP driver and a link from the pinctrl driver, and this
should be the right design.
They have a flaw since they do the mapping from the gpio_to_irq, and Linus won't allow
this anymore.

> 
> In the irqchip implementation we need the SoC-specific mapping from GPIO number
> to internal GPIO IRQ number. Having to export this from drivers/pinctrl/meson for use
> under drivers/irqchip most likely would also cause objections.

You won't need, this interrupt controller will take the number either from DT either
from a mapping creating from the pinctrl driver. The link will only be through the
irq subsystem.

> 
> So far my impression is that the very specific way GPIO IRQ's are handled on Meson
> doesn't fit perfectly into the current IRQ subsystem. Therefore the discussion
> about Jerome's version didn't result in the IRQ maintainers stating: do it this way ..
> Having said that most likely every possible approach is going to raise some concerns.

It doesn't fit exactly, but the subsystem can certainly be used to achieve it either by
using all it's capacity or by eventually discussing with the maintainers to adapt it.

Jerome has some hints hot to achieve the pinctrl part with everyone "happy".

> 
>> Please move it and make independent, you should be able to request irqs without any links
>> to the pinmux/gpio since physically the GPIO lines input are always connected to this
>> irq controller, and the pinmux has no impact on the interrupt management here.
>> >From the GPIO-IRQ Controller perspective, the GPIOs are only a number and the pinmux code
>> is only here to make the translation to a specific GPIO to a GPIO-IRQ number.
>>
>> For more chance to have it upstreamed in the right way, we should :
>> 1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype, forums, ...
>> 2) Push an independent IRQ controller that matches the capacity of the HW
>> 3) Push a link from the pinctrl driver to have the to_gpio_irq mapping done in the right way
>>
>> Jerome spent quite a lot of time and had a chat with the IRQ subsystem maintainers to have s
>> clear image of how this should be implemented, and it would be a good point to actually
>> have a chat with them to elaborate find a strong solution.
>>
> I know and I really appreciate Jerome's work and his discussion with the IRQ maintainers.
> My current attempt was inspired by his work.
> However the discussion last year ended w/o result and the topic of GPIO IRQs has been dead
> since then. And I think discussing approaches works best based on a concrete piece of code.
> Therefore I submitted my version as discussion basis. I didn't expect that everybody would
> be totally happy with it and it would go to mainline unchanged.

Sure, thanks for the work,

> 
>> I'm sorry to say that pushing this code without really understanding how and why will lead to
>> nothing expect frustration from everybody.
>>
> I can only speak for myself: I'm not frustrated and I can live with critical review comments.

Great ! Anyway thanks for your work and I hope this will lead to mainline !

> Regards, Heiner
> 
>> Neil

Neil

[...]
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Am 16.05.2017 um 09:54 schrieb Neil Armstrong:
> Hi Heiner,
> 
> On 05/15/2017 09:00 PM, Heiner Kallweit wrote:
>> Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
>>> On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
>>>> Add support for GPIO interrupts on Amlogic Meson SoC's.
>>>>
>>>> There's a limit of 8 parent interupts which can be used in total.
>>>> Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
>>>> one for each edge.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> v2:
>>>> - make the GPIO IRQ controller a separate driver
>>>> - several smaller improvements
>>>> ---
>>>>  drivers/pinctrl/Kconfig                   |   1 +
>>>>  drivers/pinctrl/meson/Makefile            |   2 +-
>>>>  drivers/pinctrl/meson/pinctrl-meson-irq.c | 367 ++++++++++++++++++++++++++++++
>>>
>>> Hi Heiner,
>>>
>>> This code has nothing to do with GPIOs on pinmux handling here, what we figured with Jerome
>>> is that this must be an independent IRQ Controller in drivers/irqchip.
>>>
>> I'm not convinced and would like to hear more opinions on that. I see it like this:
>> The driver implements an irqchip, right. But it's not an interrupt controller.
>> Due to using GPIOLIB_IRQCHIP the gpio controller now has an own irq domain (one for ao
>> and one for periphs GPIO domain). Therefore the gpio-controller now also acts as
>> interrupt-controller. And both gpio (and interrupt) controllers just use the irqchip
>> exposed by the new driver.
>> Last but not least the irqchip can be used with GPIOs only.
> 
> In fact it's an interrupt controller since it can mask/unmask and control a singal that
> can wake up an interrupt for the ARM Core.
> 
> Please look at the STM32 EXTI code, the design is quite similar except they don't have a
> dynamic management of links, but fixed ones.
> They have a proper independant IRQCHIP driver and a link from the pinctrl driver, and this
> should be the right design.
> They have a flaw since they do the mapping from the gpio_to_irq, and Linus won't allow
> this anymore.
> 
At first I involve Rob as he also provided feedback regarding the DT part.

I had a look at the STM32 EXTI code and it looks very similar to Jerome's version.
Actually I'd assume that the first Meson driver attempt was modeled after STM32 EXTI.

As you just mentioned there are at least two issues:
1. The mapping can be requested via gpio_to_irq but it doesn't have to. A driver could
   also request a GPIO IRQ via DT.
2. Missing is the handling of IRQ_TYPE_EDGE_BOTH which requires the allocation of two
   parent interrupts.

When looking at the drivers under drivers/irqchip basically all of them implement not
only an irqchip but also an IRQ domain. Providing an IRQ domain seems to me to be
the main criteria whether something should be considered an interrupt controller
(please correct me if this understanding is wrong).

The STM32 EXTI drivers seems to me to be unnecessarily complex due to not using
GPIOLIB_IRQCHIP. This GPIO framework extension can hide a significant part of the
GPIO IRQ complexity.

Coming back to my version:
The new driver just implements an irqchip, no IRQ domain. This irqchip then is
provided to the IRQ domains bound to the respective gpio controllers (for AO and PERIPHS
GPIO domain) - thanks to GPIOLIB_IRQCHIP handling of the IRQ domains comes for free.

Adding the interrupt-controller property to the gpio-controller nodes is one thing
still missing in my patch series. Then a driver can request a GPIO IRQ also via DT, e.g.:

interrupt-parent = <&gpio>;
interrupts = <GPIOZ_0 IRQ_TYPE_EDGE_BOTH>;

interrupt-parent = <&gpio_ao>;
interrupts = <GPIOAO_0 IRQ_TYPE_EDGE_BOTH>;

Advantage of having an IRQ domain per GPIO controller is being able to to use the GPIO
defines as is. Or in other words:
GPIOAO_0 and GPIOZ_0 both have the value 0. This works here due to having one IRQ domain
per GPIO controller (both using the same new irqchip).

And all I had to do is implementing one irq_chip and changing one line in the existing
pinctrl driver.
Whilst I have to admit that e.g. calling request_irq for a parent irq from the irq_set_type
callback may be something people find ugly and hacky.

Compared to this implementation the STM32 EXTI drivers needs significantly more data
structures, e.g. irqchip + irq domain in the irqchip driver, then hierarchical IRQ domain
handling + further irqchip and irq domain + irq domain ops in the pinctrl driver.

Last but not least coming back to the initial talk with Rob about where to best place the
DT docu for this irqchip implementation:
If acceptable I'd prefer to do it like .e.g in bindings/pinctrl/allwinner,sunxi-pinctrl.txt
or bindings/pinctrl/atmel,at91-pio4-pinctrl.txt.
There the interrupt-controller properties are documented as part of the pinctrl driver and
not separately under bindings/interrupt-controller.

Maybe this explains a little bit better why I chose this approach.

Rgds, Heiner

>>
>> In the irqchip implementation we need the SoC-specific mapping from GPIO number
>> to internal GPIO IRQ number. Having to export this from drivers/pinctrl/meson for use
>> under drivers/irqchip most likely would also cause objections.
> 
> You won't need, this interrupt controller will take the number either from DT either
> from a mapping creating from the pinctrl driver. The link will only be through the
> irq subsystem.
> 
>>
>> So far my impression is that the very specific way GPIO IRQ's are handled on Meson
>> doesn't fit perfectly into the current IRQ subsystem. Therefore the discussion
>> about Jerome's version didn't result in the IRQ maintainers stating: do it this way ..
>> Having said that most likely every possible approach is going to raise some concerns.
> 
> It doesn't fit exactly, but the subsystem can certainly be used to achieve it either by
> using all it's capacity or by eventually discussing with the maintainers to adapt it.
> 
> Jerome has some hints hot to achieve the pinctrl part with everyone "happy".
> 
>>
>>> Please move it and make independent, you should be able to request irqs without any links
>>> to the pinmux/gpio since physically the GPIO lines input are always connected to this
>>> irq controller, and the pinmux has no impact on the interrupt management here.
>>> >From the GPIO-IRQ Controller perspective, the GPIOs are only a number and the pinmux code
>>> is only here to make the translation to a specific GPIO to a GPIO-IRQ number.
>>>
>>> For more chance to have it upstreamed in the right way, we should :
>>> 1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype, forums, ...
>>> 2) Push an independent IRQ controller that matches the capacity of the HW
>>> 3) Push a link from the pinctrl driver to have the to_gpio_irq mapping done in the right way
>>>
>>> Jerome spent quite a lot of time and had a chat with the IRQ subsystem maintainers to have s
>>> clear image of how this should be implemented, and it would be a good point to actually
>>> have a chat with them to elaborate find a strong solution.
>>>
>> I know and I really appreciate Jerome's work and his discussion with the IRQ maintainers.
>> My current attempt was inspired by his work.
>> However the discussion last year ended w/o result and the topic of GPIO IRQs has been dead
>> since then. And I think discussing approaches works best based on a concrete piece of code.
>> Therefore I submitted my version as discussion basis. I didn't expect that everybody would
>> be totally happy with it and it would go to mainline unchanged.
> 
> Sure, thanks for the work,
> 
>>
>>> I'm sorry to say that pushing this code without really understanding how and why will lead to
>>> nothing expect frustration from everybody.
>>>
>> I can only speak for myself: I'm not frustrated and I can live with critical review comments.
> 
> Great ! Anyway thanks for your work and I hope this will lead to mainline !
> 
>> Regards, Heiner
>>
>>> Neil
> 
> Neil
> 
> [...]
> 

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

On Fri, 2017-05-12 at 21:14 +0200, Heiner Kallweit wrote:
> Add support for GPIO interrupts on Amlogic Meson SoC's.
> 
> There's a limit of 8 parent interupts which can be used in total.
> Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
> one for each edge.
> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - make the GPIO IRQ controller a separate driver
> - several smaller improvements
> ---
>  drivers/pinctrl/Kconfig                   |   1 +
>  drivers/pinctrl/meson/Makefile            |   2 +-
>  drivers/pinctrl/meson/pinctrl-meson-irq.c | 367
> ++++++++++++++++++++++++++++++
>  drivers/pinctrl/meson/pinctrl-meson.c     |   8 +-
>  drivers/pinctrl/meson/pinctrl-meson.h     |   1 +
>  5 files changed, 377 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/pinctrl/meson/pinctrl-meson-irq.c
> 
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index 37af5e30..f8f401a0 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -153,6 +153,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/Makefile b/drivers/pinctrl/meson/Makefile
> index 27c5b512..827e416d 100644
> --- a/drivers/pinctrl/meson/Makefile
> +++ b/drivers/pinctrl/meson/Makefile
> @@ -1,3 +1,3 @@
>  obj-y	+= pinctrl-meson8.o pinctrl-meson8b.o
>  obj-y	+= pinctrl-meson-gxbb.o pinctrl-meson-gxl.o
> -obj-y	+= pinctrl-meson.o
> +obj-y	+= pinctrl-meson.o pinctrl-meson-irq.o
> diff --git a/drivers/pinctrl/meson/pinctrl-meson-irq.c
> b/drivers/pinctrl/meson/pinctrl-meson-irq.c
> new file mode 100644
> index 00000000..c5f403f3
> --- /dev/null
> +++ b/drivers/pinctrl/meson/pinctrl-meson-irq.c
> @@ -0,0 +1,367 @@
> +/*
> + * Amlogic Meson GPIO IRQ driver
> + *
> + * Copyright 2017 Heiner Kallweit <hkallweit1@gmail.com>
> + * Based on a first version by Jerome Brunet <jbrunet@baylibre.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/gpio.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include "pinctrl-meson.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 MESON_GPIO_MAX_PARENT_IRQ_NUM	8
> +
> +struct meson_gpio_irq_slot {
> +	int irq;
> +	int owner;
> +};
> +
> +static struct regmap *meson_gpio_irq_regmap;
> +static struct meson_gpio_irq_slot
> +		meson_gpio_irq_slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
> +static int meson_gpio_num_irq_slots;
> +static DEFINE_MUTEX(meson_gpio_irq_slot_mutex);
> +static DECLARE_BITMAP(meson_gpio_irq_locked, 256);
> +
> +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_to_hwirq(struct meson_bank *bank, unsigned int offset)
> +{
> +	int hwirq;
> +
> +	if (bank->irq_first < 0)
> +		/* this bank cannot generate irqs */
> +		return 0;
> +
> +	hwirq = offset - bank->first + bank->irq_first;
> +
> +	if (hwirq > bank->irq_last)
> +		/* this pin cannot generate irqs */
> +		return 0;
> +
> +	return hwirq;
> +}
> +
> +static int meson_gpio_to_irq(struct meson_pinctrl *pc, unsigned int offset)
> +{
> +	struct meson_bank *bank;
> +	int hwirq;
> +
> +	offset += pc->data->pin_base;
> +
> +	bank = meson_pinctrl_get_bank(pc, offset);
> +	if (IS_ERR(bank))
> +		return PTR_ERR(bank);
> +
> +	hwirq = meson_gpio_to_hwirq(bank, offset);
> +	if (!hwirq)
> +		dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
> +
> +	return hwirq;
> +}
> +
> +static int meson_gpio_data_to_hwirq(struct irq_data *data)
> +{
> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
> +	unsigned gpio = irqd_to_hwirq(data);
> +
> +	return meson_gpio_to_irq(pc, gpio);
> +}
> +
> +static irqreturn_t meson_gpio_irq_handler(int irq, void *data)
> +{
> +	struct irq_data *gpio_irqdata = data;
> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
> +	int hwirq = meson_gpio_data_to_hwirq(gpio_irqdata);
> +
> +	/*
> +	 * For some strange reason spurious interrupts created by the chip
> when
> +	 * the interrupt source registers are written cause a deadlock here.
> +	 * generic_handle_irq calls handle_simple_irq which tries to get
> +	 * spinlock desc->lock. This interrupt handler is called whilst
> +	 * __setup_irq holds desc->lock.
> +	 * The deadlock means that both are running on the same CPU what
> should
> +	 * not happen as __setup_irq called raw_spin_lock_irqsave thus
> disabling
> +	 * interrupts on this CPU.
> +	 * Work around this by ignoring interrupts in code protected by
> +	 * chip_bus_lock (__setup_irq/__free_irq for the respective GPIO
> hwirq).
> +	 */

The explanation for this seems a bit weak. Even your comment say it does not
make sense. I've never seen this 'spurious irq' during my test.

It be nice to have the beginning of an explanation before introducing such hack.

> +	if (test_bit(hwirq, meson_gpio_irq_locked))
> +		dev_dbg(pc->dev, "spurious interrupt detected!\n");
> +	else
> +		generic_handle_irq(gpio_irqdata->irq);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int meson_gpio_alloc_irq_slot(struct irq_data *data, int num_slots,
> +				     int *slots)
> +{
> +	int i, cnt = 0;
> +
> +	mutex_lock(&meson_gpio_irq_slot_mutex);
> +
> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
> +		if (!meson_gpio_irq_slots[i].owner) {
> +			meson_gpio_irq_slots[i].owner = data->irq;
> +			slots[cnt++] = i;
> +			if (cnt == num_slots)
> +				break;
> +		}
> +
> +	if (cnt < num_slots)
> +		for (i = 0; i < cnt; i++)
> +			meson_gpio_irq_slots[i].owner = 0;
> +
> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
> +
> +	return cnt == num_slots ? 0 : -ENOSPC;
> +}
> +
> +static void meson_gpio_free_irq_slot(struct irq_data *data)
> +{
> +	int i;
> +
> +	mutex_lock(&meson_gpio_irq_slot_mutex);
> +
> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
> +		if (meson_gpio_irq_slots[i].owner == data->irq) {
> +			free_irq(meson_gpio_irq_slots[i].irq, data);
> +			meson_gpio_irq_slots[i].owner = 0;
> +		}
> +
> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
> +}
> +
> +static int meson_gpio_find_irq_slot(struct irq_data *data, int *slots)
> +{
> +	int i, cnt = 0;
> +
> +	mutex_lock(&meson_gpio_irq_slot_mutex);
> +
> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
> +		if (meson_gpio_irq_slots[i].owner == data->irq)
> +			slots[cnt++] = i;
> +
> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
> +
> +	return cnt ?: -EINVAL;
> +}
> +
> +static void meson_gpio_set_hwirq(int idx, int hwirq)
> +{
> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
> +	int shift = 8 * (idx % 4);
> +
> +	regmap_update_bits(meson_gpio_irq_regmap, reg, 0xff << shift,
> +			   hwirq << shift);
> +}
> +
> +static void meson_gpio_irq_set_hwirq(struct irq_data *data, int hwirq)
> +{
> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
> +	int i, cnt, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
> +
> +	cnt = meson_gpio_find_irq_slot(data, slots);
> +	if (cnt < 0) {
> +		dev_err(pc->dev, "didn't find gpio irq slot\n");
> +		return;
> +	}
> +
> +	for (i = 0; i < cnt; i++)
> +		meson_gpio_set_hwirq(slots[i], hwirq);
> +}
> +
> +static void meson_gpio_irq_unmask(struct irq_data *data)
> +{
> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
> +	unsigned gpio = irqd_to_hwirq(data);
> +	int hwirq = meson_gpio_to_irq(pc, gpio);
> +
> +	meson_gpio_irq_set_hwirq(data, hwirq);
> +}
> +
> +static void meson_gpio_irq_mask(struct irq_data *data)
> +{
> +	meson_gpio_irq_set_hwirq(data, 0xff);
> +}
> +
> +static void meson_gpio_irq_shutdown(struct irq_data *data)
> +{
> +	meson_gpio_irq_mask(data);
> +	meson_gpio_free_irq_slot(data);
> +}
> +
> +static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> +	int i, ret, irq, num_slots, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
> +	unsigned int val = 0;
> +
> +	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
> +	ret = meson_gpio_alloc_irq_slot(data, num_slots, slots);
> +	if (ret)
> +		return ret;
> +
> +	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
> +		val |= REG_EDGE_POL_EDGE(slots[0]);
> +
> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
> +		val |= REG_EDGE_POL_LOW(slots[0]);
> +
> +	regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
> +			   REG_EDGE_POL_MASK(slots[0]), val);
> +
> +	/*
> +	 * The chip can create an interrupt for either rising or falling edge
> +	 * only. Therefore use two interrupts in case of IRQ_TYPE_EDGE_BOTH,
> +	 * first for falling edge and second one for rising edge.
> +	 */
> +	if (num_slots > 1) {
> +		val = REG_EDGE_POL_EDGE(slots[1]);
> +		regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
> +				   REG_EDGE_POL_MASK(slots[1]), val);
> +	}
> +
> +	if (type & IRQ_TYPE_EDGE_BOTH)
> +		val = IRQ_TYPE_EDGE_RISING;
> +	else
> +		val = IRQ_TYPE_LEVEL_HIGH;
> +
> +	for (i = 0; i < num_slots; i++) {
> +		irq = meson_gpio_irq_slots[slots[i]].irq;
> +		ret = irq_set_irq_type(irq, val);
> +		if (ret)
> +			break;
> +		ret = request_irq(irq, meson_gpio_irq_handler, 0,
> +				  "GPIO parent", data);
It seems weird to use request_irq here. With the eventual reuqeste_irq in the
consumer driver, wouldn't you end with to virq allocated for what is only on irq
line really ?

Isn't it showing that you should use domains and mappings ?


> +		if (ret)
> +			break;
> +	}
> +
> +	if (ret)
> +		while (--i >= 0)
> +			free_irq(meson_gpio_irq_slots[slots[i]].irq, data);
> +
> +	return ret;
> +}
> +
> +static void meson_gpio_irq_bus_lock(struct irq_data *data)
> +{
> +	int hwirq = meson_gpio_data_to_hwirq(data);
> +
> +	set_bit(hwirq, meson_gpio_irq_locked);
> +}
> +
> +static void meson_gpio_irq_bus_sync_unlock(struct irq_data *data)
> +{
> +	int hwirq = meson_gpio_data_to_hwirq(data);
> +
> +	clear_bit(hwirq, meson_gpio_irq_locked);
> +}
> +
> +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_shutdown = meson_gpio_irq_shutdown,
> +	.irq_bus_lock = meson_gpio_irq_bus_lock,
> +	.irq_bus_sync_unlock = meson_gpio_irq_bus_sync_unlock,
> +};
> +
> +static int meson_gpio_get_irqs(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	int irq, i;
> +
> +	for (i = 0; i < MESON_GPIO_MAX_PARENT_IRQ_NUM; i++) {
> +		irq = irq_of_parse_and_map(np, i);
> +		if (!irq)
> +			break;
> +		meson_gpio_irq_slots[i].irq = irq;
> +	}
> +
> +	meson_gpio_num_irq_slots = i;
> +
> +	return i ? 0 : -EINVAL;
> +}
> +
> +static const struct regmap_config meson_gpio_regmap_config = {
> +	.reg_bits       = 32,
> +	.reg_stride     = 4,
> +	.val_bits       = 32,
> +	.max_register	= REG_FILTER_SEL,
> +};
> +
> +static int meson_gpio_irq_probe(struct platform_device *pdev)
> +{
> +	struct resource *res;
> +	void __iomem *io_base;
> +	int ret;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	io_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	meson_gpio_irq_regmap = devm_regmap_init_mmio(&pdev->dev, io_base,
> +						&meson_gpio_regmap_config);
> +	if (IS_ERR(meson_gpio_irq_regmap))
> +		return PTR_ERR(meson_gpio_irq_regmap);
> +
> +	/* initialize to IRQ_TYPE_LEVEL_HIGH */
> +	regmap_write(meson_gpio_irq_regmap, REG_EDGE_POL, 0);
> +	/* disable all GPIO interrupt sources */
> +	regmap_write(meson_gpio_irq_regmap, REG_PIN_03_SEL, 0xffffffff);
> +	regmap_write(meson_gpio_irq_regmap, REG_PIN_47_SEL, 0xffffffff);
> +	/* disable filtering */
> +	regmap_write(meson_gpio_irq_regmap, REG_FILTER_SEL, 0);
> +
> +	ret = meson_gpio_get_irqs(pdev);
> +	if (ret)
> +		return ret;
> +
> +	meson_pinctrl_irq_chip = &meson_gpio_irq_chip;
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id meson_gpio_irq_dt_match[] = {
> +	{ .compatible = "amlogic,meson-gpio-interrupt" },
> +	{ },
> +};
> +
> +static struct platform_driver meson_gpio_irq_driver = {
> +	.probe		= meson_gpio_irq_probe,
> +	.driver = {
> +		.name	= "meson-gpio-interrupt",
> +		.of_match_table = meson_gpio_irq_dt_match,
> +	},
> +};
> +builtin_platform_driver(meson_gpio_irq_driver);
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c
> b/drivers/pinctrl/meson/pinctrl-meson.c
> index 39ad9861..c587f6f0 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"
>  
> +struct irq_chip *meson_pinctrl_irq_chip;
> +
>  /**
>   * meson_pinctrl_get_bank() - find the bank containing a given pin
>   *
> @@ -551,7 +553,8 @@ static int meson_gpiolib_register(struct meson_pinctrl
> *pc)
>  		return ret;
>  	}
>  
> -	return 0;
> +	return gpiochip_irqchip_add(&pc->chip, meson_pinctrl_irq_chip, 0,
> +				    handle_simple_irq, IRQ_TYPE_NONE);
>  }
>  
>  static struct regmap_config meson_regmap_config = {
> @@ -640,6 +643,9 @@ static int meson_pinctrl_probe(struct platform_device
> *pdev)
>  	struct meson_pinctrl *pc;
>  	int ret;
>  
> +	if (!meson_pinctrl_irq_chip)
> +		return -EPROBE_DEFER;
> +
>  	pc = devm_kzalloc(dev, sizeof(struct meson_pinctrl), GFP_KERNEL);
>  	if (!pc)
>  		return -ENOMEM;
> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h
> b/drivers/pinctrl/meson/pinctrl-meson.h
> index 40b56aff..16aab328 100644
> --- a/drivers/pinctrl/meson/pinctrl-meson.h
> +++ b/drivers/pinctrl/meson/pinctrl-meson.h
> @@ -176,6 +176,7 @@ extern struct meson_pinctrl_data
> meson_gxbb_periphs_pinctrl_data;
>  extern struct meson_pinctrl_data meson_gxbb_aobus_pinctrl_data;
>  extern struct meson_pinctrl_data meson_gxl_periphs_pinctrl_data;
>  extern struct meson_pinctrl_data meson_gxl_aobus_pinctrl_data;
> +extern struct irq_chip *meson_pinctrl_irq_chip;
>  
>  struct meson_bank *meson_pinctrl_get_bank(const struct meson_pinctrl *pc,
>  					  unsigned int pin);
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, 2017-05-16 at 20:31 +0200, Heiner Kallweit wrote:
> Am 16.05.2017 um 09:54 schrieb Neil Armstrong:
> > Hi Heiner,
> > 
> > On 05/15/2017 09:00 PM, Heiner Kallweit wrote:
> > > Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
> > > > On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
> > > > > Add support for GPIO interrupts on Amlogic Meson SoC's.
> > > > > 
> > > > > There's a limit of 8 parent interupts which can be used in total.
> > > > > Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
> > > > > one for each edge.
> > > > > 
> > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> > > > > ---
> > > > > v2:
> > > > > - make the GPIO IRQ controller a separate driver
> > > > > - several smaller improvements
> > > > > ---
> > > > >  drivers/pinctrl/Kconfig                   |   1 +
> > > > >  drivers/pinctrl/meson/Makefile            |   2 +-
> > > > >  drivers/pinctrl/meson/pinctrl-meson-irq.c | 367
> > > > > ++++++++++++++++++++++++++++++
> > > > 
> > > > Hi Heiner,
> > > > 
> > > > This code has nothing to do with GPIOs on pinmux handling here, what we
> > > > figured with Jerome
> > > > is that this must be an independent IRQ Controller in drivers/irqchip.
> > > > 
> > > 
> > > I'm not convinced and would like to hear more opinions on that. I see it
> > > like this:
> > > The driver implements an irqchip, right. But it's not an interrupt
> > > controller.
> > > Due to using GPIOLIB_IRQCHIP the gpio controller now has an own irq domain
> > > (one for ao
> > > and one for periphs GPIO domain). Therefore the gpio-controller now also
> > > acts as
> > > interrupt-controller. And both gpio (and interrupt) controllers just use
> > > the irqchip
> > > exposed by the new driver.
> > > Last but not least the irqchip can be used with GPIOs only.
> > 
> > In fact it's an interrupt controller since it can mask/unmask and control a
> > singal that
> > can wake up an interrupt for the ARM Core.
> > 
> > Please look at the STM32 EXTI code, the design is quite similar except they
> > don't have a
> > dynamic management of links, but fixed ones.
> > They have a proper independant IRQCHIP driver and a link from the pinctrl
> > driver, and this
> > should be the right design.
> > They have a flaw since they do the mapping from the gpio_to_irq, and Linus
> > won't allow
> > this anymore.
> > 
> 
> At first I involve Rob as he also provided feedback regarding the DT part.
> 
> I had a look at the STM32 EXTI code and it looks very similar to Jerome's
> version.
> Actually I'd assume that the first Meson driver attempt was modeled after
> STM32 EXTI.
Well, no. EXTI has been merged when I already submitted the RFC for this driver,
but that's not the point I suppose.

> 
> As you just mentioned there are at least two issues:
> 1. The mapping can be requested via gpio_to_irq but it doesn't have to. A
> driver could
Maybe you missed it in our previous discussion with Linus but, no you can't
create mapping in gpio_to_irq. This callback is fast and creating mappings might
sleep because of the irq_domain mutex

https://patchwork.ozlabs.org/patch/684208/

>    also request a GPIO IRQ via DT.
> 2. Missing is the handling of IRQ_TYPE_EDGE_BOTH which requires the allocation
> of two
>    parent interrupts.
> 
> When looking at the drivers under drivers/irqchip basically all of them
> implement not
> only an irqchip but also an IRQ domain. Providing an IRQ domain seems to me to
> be
> the main criteria whether something should be considered an interrupt
> controller
> (please correct me if this understanding is wrong).
> 
> The STM32 EXTI drivers seems to me to be unnecessarily complex due to not
> using
> GPIOLIB_IRQCHIP. This GPIO framework extension can hide a significant part of
> the
> GPIO IRQ complexity.
I can only speculate regarding the design choice of STM EXTI, but I suppose the
EXTI controller is independent of the pinmux/gpio subsystem HW wise. It's only
weakly linked to the gpios, in the way that you have (or can create) a "map"
between the gpio and the irq line number provided.

That is also the case for the amlogic gpio-irq. I suppose that's why Neil
suggested to look at EXTI. That's also why I commented that this should be first
implemented as an independent controller of the gpio subsys  (so in
drivers/irqchip)

You may find it more complex, which is arguable, but it is a more accurate
representation of the HW.

> 
> Coming back to my version:
> The new driver just implements an irqchip, no IRQ domain.
>  This irqchip then is
> provided to the IRQ domains bound to the respective gpio controllers (for AO
> and PERIPHS
> GPIO domain) - thanks to GPIOLIB_IRQCHIP handling of the IRQ domains comes for
> free.
> 
> Adding the interrupt-controller property to the gpio-controller nodes is one
> thing
> still missing in my patch series. Then a driver can request a GPIO IRQ also
> via DT, e.g.:
> 
> interrupt-parent = <&gpio>;
> interrupts = <GPIOZ_0 IRQ_TYPE_EDGE_BOTH>;
> 
> interrupt-parent = <&gpio_ao>;
> interrupts = <GPIOAO_0 IRQ_TYPE_EDGE_BOTH>;
> 

Which is also possible in the series, where the controller is gpio-irq device
itself. This is what the HW provide, so it should be possible. It would be nice
to have your way working as well, which I think is possible (PSB)

> Advantage of having an IRQ domain per GPIO controller is being able to to use
> the GPIO
> defines as is. Or in other words:
> GPIOAO_0 and GPIOZ_0 both have the value 0. This works here due to having one
> IRQ domain
> per GPIO controller (both using the same new irqchip).
> 

I think you are onto something. As I told you previously, the problem was to
create the mappings in pinctrl w/o allocating the parent. I struggled with that
back in November and I had no time to really get back to it since.

Creating domains in each gpio controller, w/ all the mapping created at startup,
  stacked to the domain provided by the driver/irqchip would solve the problem.

We would just have to call find_mapping in gpio_to_irq, which is fine and
allocate the parent irq in the request_ressource callback.

> And all I had to do is implementing one irq_chip and changing one line in the
> existing
> pinctrl driver.
> Whilst I have to admit that e.g. calling request_irq for a parent irq from the
> irq_set_type
> callback may be something people find ugly and hacky.
> 
The fact is that the HW does not directly support IRQ_EDGE_BOTH. Yes, it would
be nice to find a way to support it anyway. There two possibilities for it:
* Change the change the edge type depending on the next expected edge: Marc
already stated that he is firmly against that. It is indeed not very robust
* Allocate and deallocate additional parent irq to get secondary irq in
set_type: This indeed looks hacky, I'd like to get the view of irq maintainers
on this.


> Compared to this implementation the STM32 EXTI drivers needs significantly
> more data
> structures, e.g. irqchip + irq domain in the irqchip driver, then hierarchical
> IRQ domain
> handling + further irqchip and irq domain + irq domain ops in the pinctrl
> driver.

Within reasonable limits, having more or less data, code lines does not make a
driver better or worse. 

> 
> Last but not least coming back to the initial talk with Rob about where to
> best place the
> DT docu for this irqchip implementation:
> If acceptable I'd prefer to do it like .e.g in
> bindings/pinctrl/allwinner,sunxi-pinctrl.txt
> or bindings/pinctrl/atmel,at91-pio4-pinctrl.txt.
> There the interrupt-controller properties are documented as part of the
> pinctrl driver and
> not separately under bindings/interrupt-controller.
> 
> Maybe this explains a little bit better why I chose this approach.
> 
> Rgds, Heiner
> 
> > > 
> > > In the irqchip implementation we need the SoC-specific mapping from GPIO
> > > number
> > > to internal GPIO IRQ number. Having to export this from
> > > drivers/pinctrl/meson for use
> > > under drivers/irqchip most likely would also cause objections.
> > 
> > You won't need, this interrupt controller will take the number either from
> > DT either
> > from a mapping creating from the pinctrl driver. The link will only be
> > through the
> > irq subsystem.
> > 
> > > 
> > > So far my impression is that the very specific way GPIO IRQ's are handled
> > > on Meson
> > > doesn't fit perfectly into the current IRQ subsystem. Therefore the
> > > discussion
> > > about Jerome's version didn't result in the IRQ maintainers stating: do it
> > > this way ..
> > > Having said that most likely every possible approach is going to raise
> > > some concerns.
> > 
> > It doesn't fit exactly, but the subsystem can certainly be used to achieve
> > it either by
> > using all it's capacity or by eventually discussing with the maintainers to
> > adapt it.
> > 
> > Jerome has some hints hot to achieve the pinctrl part with everyone "happy".
> > 
> > > 
> > > > Please move it and make independent, you should be able to request irqs
> > > > without any links
> > > > to the pinmux/gpio since physically the GPIO lines input are always
> > > > connected to this
> > > > irq controller, and the pinmux has no impact on the interrupt management
> > > > here.
> > > > > From the GPIO-IRQ Controller perspective, the GPIOs are only a number
> > > > > and the pinmux code
> > > > 
> > > > is only here to make the translation to a specific GPIO to a GPIO-IRQ
> > > > number.
> > > > 
> > > > For more chance to have it upstreamed in the right way, we should :
> > > > 1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype,
> > > > forums, ...
> > > > 2) Push an independent IRQ controller that matches the capacity of the
> > > > HW
> > > > 3) Push a link from the pinctrl driver to have the to_gpio_irq mapping
> > > > done in the right way
> > > > 
> > > > Jerome spent quite a lot of time and had a chat with the IRQ subsystem
> > > > maintainers to have s
> > > > clear image of how this should be implemented, and it would be a good
> > > > point to actually
> > > > have a chat with them to elaborate find a strong solution.
> > > > 
> > > 
> > > I know and I really appreciate Jerome's work and his discussion with the
> > > IRQ maintainers.
> > > My current attempt was inspired by his work.
> > > However the discussion last year ended w/o result and the topic of GPIO
> > > IRQs has been dead
> > > since then. And I think discussing approaches works best based on a
> > > concrete piece of code.
> > > Therefore I submitted my version as discussion basis. I didn't expect that
> > > everybody would
> > > be totally happy with it and it would go to mainline unchanged.
> > 
> > Sure, thanks for the work,
> > 
> > > 
> > > > I'm sorry to say that pushing this code without really understanding how
> > > > and why will lead to
> > > > nothing expect frustration from everybody.
> > > > 
> > > 
> > > I can only speak for myself: I'm not frustrated and I can live with
> > > critical review comments.
> > 
> > Great ! Anyway thanks for your work and I hope this will lead to mainline !
> > 
> > > Regards, Heiner
> > > 
> > > > Neil
> > 
> > Neil
> > 
> > [...]
> > 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Am 17.05.2017 um 01:07 schrieb Jerome Brunet:
> On Fri, 2017-05-12 at 21:14 +0200, Heiner Kallweit wrote:
>> Add support for GPIO interrupts on Amlogic Meson SoC's.
>>
>> There's a limit of 8 parent interupts which can be used in total.
>> Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
>> one for each edge.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v2:
>> - make the GPIO IRQ controller a separate driver
>> - several smaller improvements
>> ---
>>  drivers/pinctrl/Kconfig                   |   1 +
>>  drivers/pinctrl/meson/Makefile            |   2 +-
>>  drivers/pinctrl/meson/pinctrl-meson-irq.c | 367
>> ++++++++++++++++++++++++++++++
>>  drivers/pinctrl/meson/pinctrl-meson.c     |   8 +-
>>  drivers/pinctrl/meson/pinctrl-meson.h     |   1 +
>>  5 files changed, 377 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/pinctrl/meson/pinctrl-meson-irq.c
>>
>> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
>> index 37af5e30..f8f401a0 100644
>> --- a/drivers/pinctrl/Kconfig
>> +++ b/drivers/pinctrl/Kconfig
>> @@ -153,6 +153,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/Makefile b/drivers/pinctrl/meson/Makefile
>> index 27c5b512..827e416d 100644
>> --- a/drivers/pinctrl/meson/Makefile
>> +++ b/drivers/pinctrl/meson/Makefile
>> @@ -1,3 +1,3 @@
>>  obj-y	+= pinctrl-meson8.o pinctrl-meson8b.o
>>  obj-y	+= pinctrl-meson-gxbb.o pinctrl-meson-gxl.o
>> -obj-y	+= pinctrl-meson.o
>> +obj-y	+= pinctrl-meson.o pinctrl-meson-irq.o
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson-irq.c
>> b/drivers/pinctrl/meson/pinctrl-meson-irq.c
>> new file mode 100644
>> index 00000000..c5f403f3
>> --- /dev/null
>> +++ b/drivers/pinctrl/meson/pinctrl-meson-irq.c
>> @@ -0,0 +1,367 @@
>> +/*
>> + * Amlogic Meson GPIO IRQ driver
>> + *
>> + * Copyright 2017 Heiner Kallweit <hkallweit1@gmail.com>
>> + * Based on a first version by Jerome Brunet <jbrunet@baylibre.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/gpio.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_address.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include "pinctrl-meson.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 MESON_GPIO_MAX_PARENT_IRQ_NUM	8
>> +
>> +struct meson_gpio_irq_slot {
>> +	int irq;
>> +	int owner;
>> +};
>> +
>> +static struct regmap *meson_gpio_irq_regmap;
>> +static struct meson_gpio_irq_slot
>> +		meson_gpio_irq_slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
>> +static int meson_gpio_num_irq_slots;
>> +static DEFINE_MUTEX(meson_gpio_irq_slot_mutex);
>> +static DECLARE_BITMAP(meson_gpio_irq_locked, 256);
>> +
>> +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_to_hwirq(struct meson_bank *bank, unsigned int offset)
>> +{
>> +	int hwirq;
>> +
>> +	if (bank->irq_first < 0)
>> +		/* this bank cannot generate irqs */
>> +		return 0;
>> +
>> +	hwirq = offset - bank->first + bank->irq_first;
>> +
>> +	if (hwirq > bank->irq_last)
>> +		/* this pin cannot generate irqs */
>> +		return 0;
>> +
>> +	return hwirq;
>> +}
>> +
>> +static int meson_gpio_to_irq(struct meson_pinctrl *pc, unsigned int offset)
>> +{
>> +	struct meson_bank *bank;
>> +	int hwirq;
>> +
>> +	offset += pc->data->pin_base;
>> +
>> +	bank = meson_pinctrl_get_bank(pc, offset);
>> +	if (IS_ERR(bank))
>> +		return PTR_ERR(bank);
>> +
>> +	hwirq = meson_gpio_to_hwirq(bank, offset);
>> +	if (!hwirq)
>> +		dev_dbg(pc->dev, "no interrupt for pin %u\n", offset);
>> +
>> +	return hwirq;
>> +}
>> +
>> +static int meson_gpio_data_to_hwirq(struct irq_data *data)
>> +{
>> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
>> +	unsigned gpio = irqd_to_hwirq(data);
>> +
>> +	return meson_gpio_to_irq(pc, gpio);
>> +}
>> +
>> +static irqreturn_t meson_gpio_irq_handler(int irq, void *data)
>> +{
>> +	struct irq_data *gpio_irqdata = data;
>> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
>> +	int hwirq = meson_gpio_data_to_hwirq(gpio_irqdata);
>> +
>> +	/*
>> +	 * For some strange reason spurious interrupts created by the chip
>> when
>> +	 * the interrupt source registers are written cause a deadlock here.
>> +	 * generic_handle_irq calls handle_simple_irq which tries to get
>> +	 * spinlock desc->lock. This interrupt handler is called whilst
>> +	 * __setup_irq holds desc->lock.
>> +	 * The deadlock means that both are running on the same CPU what
>> should
>> +	 * not happen as __setup_irq called raw_spin_lock_irqsave thus
>> disabling
>> +	 * interrupts on this CPU.
>> +	 * Work around this by ignoring interrupts in code protected by
>> +	 * chip_bus_lock (__setup_irq/__free_irq for the respective GPIO
>> hwirq).
>> +	 */
> 
> The explanation for this seems a bit weak. Even your comment say it does not
> make sense. I've never seen this 'spurious irq' during my test.
> 
> It be nice to have the beginning of an explanation before introducing such hack.
> 
In v3 of the patch series I switched from request_irq to chained irq handling.
This also fixed the spurious irq issue and the hacky workaround code was removed.

>> +	if (test_bit(hwirq, meson_gpio_irq_locked))
>> +		dev_dbg(pc->dev, "spurious interrupt detected!\n");
>> +	else
>> +		generic_handle_irq(gpio_irqdata->irq);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int meson_gpio_alloc_irq_slot(struct irq_data *data, int num_slots,
>> +				     int *slots)
>> +{
>> +	int i, cnt = 0;
>> +
>> +	mutex_lock(&meson_gpio_irq_slot_mutex);
>> +
>> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
>> +		if (!meson_gpio_irq_slots[i].owner) {
>> +			meson_gpio_irq_slots[i].owner = data->irq;
>> +			slots[cnt++] = i;
>> +			if (cnt == num_slots)
>> +				break;
>> +		}
>> +
>> +	if (cnt < num_slots)
>> +		for (i = 0; i < cnt; i++)
>> +			meson_gpio_irq_slots[i].owner = 0;
>> +
>> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
>> +
>> +	return cnt == num_slots ? 0 : -ENOSPC;
>> +}
>> +
>> +static void meson_gpio_free_irq_slot(struct irq_data *data)
>> +{
>> +	int i;
>> +
>> +	mutex_lock(&meson_gpio_irq_slot_mutex);
>> +
>> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
>> +		if (meson_gpio_irq_slots[i].owner == data->irq) {
>> +			free_irq(meson_gpio_irq_slots[i].irq, data);
>> +			meson_gpio_irq_slots[i].owner = 0;
>> +		}
>> +
>> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
>> +}
>> +
>> +static int meson_gpio_find_irq_slot(struct irq_data *data, int *slots)
>> +{
>> +	int i, cnt = 0;
>> +
>> +	mutex_lock(&meson_gpio_irq_slot_mutex);
>> +
>> +	for (i = 0; i < meson_gpio_num_irq_slots; i++)
>> +		if (meson_gpio_irq_slots[i].owner == data->irq)
>> +			slots[cnt++] = i;
>> +
>> +	mutex_unlock(&meson_gpio_irq_slot_mutex);
>> +
>> +	return cnt ?: -EINVAL;
>> +}
>> +
>> +static void meson_gpio_set_hwirq(int idx, int hwirq)
>> +{
>> +	int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL;
>> +	int shift = 8 * (idx % 4);
>> +
>> +	regmap_update_bits(meson_gpio_irq_regmap, reg, 0xff << shift,
>> +			   hwirq << shift);
>> +}
>> +
>> +static void meson_gpio_irq_set_hwirq(struct irq_data *data, int hwirq)
>> +{
>> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
>> +	int i, cnt, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
>> +
>> +	cnt = meson_gpio_find_irq_slot(data, slots);
>> +	if (cnt < 0) {
>> +		dev_err(pc->dev, "didn't find gpio irq slot\n");
>> +		return;
>> +	}
>> +
>> +	for (i = 0; i < cnt; i++)
>> +		meson_gpio_set_hwirq(slots[i], hwirq);
>> +}
>> +
>> +static void meson_gpio_irq_unmask(struct irq_data *data)
>> +{
>> +	struct meson_pinctrl *pc = meson_gpio_data_to_pc(data);
>> +	unsigned gpio = irqd_to_hwirq(data);
>> +	int hwirq = meson_gpio_to_irq(pc, gpio);
>> +
>> +	meson_gpio_irq_set_hwirq(data, hwirq);
>> +}
>> +
>> +static void meson_gpio_irq_mask(struct irq_data *data)
>> +{
>> +	meson_gpio_irq_set_hwirq(data, 0xff);
>> +}
>> +
>> +static void meson_gpio_irq_shutdown(struct irq_data *data)
>> +{
>> +	meson_gpio_irq_mask(data);
>> +	meson_gpio_free_irq_slot(data);
>> +}
>> +
>> +static int meson_gpio_irq_set_type(struct irq_data *data, unsigned int type)
>> +{
>> +	int i, ret, irq, num_slots, slots[MESON_GPIO_MAX_PARENT_IRQ_NUM];
>> +	unsigned int val = 0;
>> +
>> +	num_slots = (type == IRQ_TYPE_EDGE_BOTH) ? 2 : 1;
>> +	ret = meson_gpio_alloc_irq_slot(data, num_slots, slots);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING))
>> +		val |= REG_EDGE_POL_EDGE(slots[0]);
>> +
>> +	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING))
>> +		val |= REG_EDGE_POL_LOW(slots[0]);
>> +
>> +	regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
>> +			   REG_EDGE_POL_MASK(slots[0]), val);
>> +
>> +	/*
>> +	 * The chip can create an interrupt for either rising or falling edge
>> +	 * only. Therefore use two interrupts in case of IRQ_TYPE_EDGE_BOTH,
>> +	 * first for falling edge and second one for rising edge.
>> +	 */
>> +	if (num_slots > 1) {
>> +		val = REG_EDGE_POL_EDGE(slots[1]);
>> +		regmap_update_bits(meson_gpio_irq_regmap, REG_EDGE_POL,
>> +				   REG_EDGE_POL_MASK(slots[1]), val);
>> +	}
>> +
>> +	if (type & IRQ_TYPE_EDGE_BOTH)
>> +		val = IRQ_TYPE_EDGE_RISING;
>> +	else
>> +		val = IRQ_TYPE_LEVEL_HIGH;
>> +
>> +	for (i = 0; i < num_slots; i++) {
>> +		irq = meson_gpio_irq_slots[slots[i]].irq;
>> +		ret = irq_set_irq_type(irq, val);
>> +		if (ret)
>> +			break;
>> +		ret = request_irq(irq, meson_gpio_irq_handler, 0,
>> +				  "GPIO parent", data);
> It seems weird to use request_irq here. With the eventual reuqeste_irq in the
> consumer driver, wouldn't you end with to virq allocated for what is only on irq
> line really ?
> Was changed to chained irq handling in v3 and this issue is gone therefore.

> Isn't it showing that you should use domains and mappings ?
> 
> 
>> +		if (ret)
>> +			break;
>> +	}
>> +
>> +	if (ret)
>> +		while (--i >= 0)
>> +			free_irq(meson_gpio_irq_slots[slots[i]].irq, data);
>> +
>> +	return ret;
>> +}
>> +
>> +static void meson_gpio_irq_bus_lock(struct irq_data *data)
>> +{
>> +	int hwirq = meson_gpio_data_to_hwirq(data);
>> +
>> +	set_bit(hwirq, meson_gpio_irq_locked);
>> +}
>> +
>> +static void meson_gpio_irq_bus_sync_unlock(struct irq_data *data)
>> +{
>> +	int hwirq = meson_gpio_data_to_hwirq(data);
>> +
>> +	clear_bit(hwirq, meson_gpio_irq_locked);
>> +}
>> +
>> +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_shutdown = meson_gpio_irq_shutdown,
>> +	.irq_bus_lock = meson_gpio_irq_bus_lock,
>> +	.irq_bus_sync_unlock = meson_gpio_irq_bus_sync_unlock,
>> +};
>> +
>> +static int meson_gpio_get_irqs(struct platform_device *pdev)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	int irq, i;
>> +
>> +	for (i = 0; i < MESON_GPIO_MAX_PARENT_IRQ_NUM; i++) {
>> +		irq = irq_of_parse_and_map(np, i);
>> +		if (!irq)
>> +			break;
>> +		meson_gpio_irq_slots[i].irq = irq;
>> +	}
>> +
>> +	meson_gpio_num_irq_slots = i;
>> +
>> +	return i ? 0 : -EINVAL;
>> +}
>> +
>> +static const struct regmap_config meson_gpio_regmap_config = {
>> +	.reg_bits       = 32,
>> +	.reg_stride     = 4,
>> +	.val_bits       = 32,
>> +	.max_register	= REG_FILTER_SEL,
>> +};
>> +
>> +static int meson_gpio_irq_probe(struct platform_device *pdev)
>> +{
>> +	struct resource *res;
>> +	void __iomem *io_base;
>> +	int ret;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	io_base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(io_base))
>> +		return PTR_ERR(io_base);
>> +
>> +	meson_gpio_irq_regmap = devm_regmap_init_mmio(&pdev->dev, io_base,
>> +						&meson_gpio_regmap_config);
>> +	if (IS_ERR(meson_gpio_irq_regmap))
>> +		return PTR_ERR(meson_gpio_irq_regmap);
>> +
>> +	/* initialize to IRQ_TYPE_LEVEL_HIGH */
>> +	regmap_write(meson_gpio_irq_regmap, REG_EDGE_POL, 0);
>> +	/* disable all GPIO interrupt sources */
>> +	regmap_write(meson_gpio_irq_regmap, REG_PIN_03_SEL, 0xffffffff);
>> +	regmap_write(meson_gpio_irq_regmap, REG_PIN_47_SEL, 0xffffffff);
>> +	/* disable filtering */
>> +	regmap_write(meson_gpio_irq_regmap, REG_FILTER_SEL, 0);
>> +
>> +	ret = meson_gpio_get_irqs(pdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	meson_pinctrl_irq_chip = &meson_gpio_irq_chip;
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id meson_gpio_irq_dt_match[] = {
>> +	{ .compatible = "amlogic,meson-gpio-interrupt" },
>> +	{ },
>> +};
>> +
>> +static struct platform_driver meson_gpio_irq_driver = {
>> +	.probe		= meson_gpio_irq_probe,
>> +	.driver = {
>> +		.name	= "meson-gpio-interrupt",
>> +		.of_match_table = meson_gpio_irq_dt_match,
>> +	},
>> +};
>> +builtin_platform_driver(meson_gpio_irq_driver);
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.c
>> b/drivers/pinctrl/meson/pinctrl-meson.c
>> index 39ad9861..c587f6f0 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"
>>  
>> +struct irq_chip *meson_pinctrl_irq_chip;
>> +
>>  /**
>>   * meson_pinctrl_get_bank() - find the bank containing a given pin
>>   *
>> @@ -551,7 +553,8 @@ static int meson_gpiolib_register(struct meson_pinctrl
>> *pc)
>>  		return ret;
>>  	}
>>  
>> -	return 0;
>> +	return gpiochip_irqchip_add(&pc->chip, meson_pinctrl_irq_chip, 0,
>> +				    handle_simple_irq, IRQ_TYPE_NONE);
>>  }
>>  
>>  static struct regmap_config meson_regmap_config = {
>> @@ -640,6 +643,9 @@ static int meson_pinctrl_probe(struct platform_device
>> *pdev)
>>  	struct meson_pinctrl *pc;
>>  	int ret;
>>  
>> +	if (!meson_pinctrl_irq_chip)
>> +		return -EPROBE_DEFER;
>> +
>>  	pc = devm_kzalloc(dev, sizeof(struct meson_pinctrl), GFP_KERNEL);
>>  	if (!pc)
>>  		return -ENOMEM;
>> diff --git a/drivers/pinctrl/meson/pinctrl-meson.h
>> b/drivers/pinctrl/meson/pinctrl-meson.h
>> index 40b56aff..16aab328 100644
>> --- a/drivers/pinctrl/meson/pinctrl-meson.h
>> +++ b/drivers/pinctrl/meson/pinctrl-meson.h
>> @@ -176,6 +176,7 @@ extern struct meson_pinctrl_data
>> meson_gxbb_periphs_pinctrl_data;
>>  extern struct meson_pinctrl_data meson_gxbb_aobus_pinctrl_data;
>>  extern struct meson_pinctrl_data meson_gxl_periphs_pinctrl_data;
>>  extern struct meson_pinctrl_data meson_gxl_aobus_pinctrl_data;
>> +extern struct irq_chip *meson_pinctrl_irq_chip;
>>  
>>  struct meson_bank *meson_pinctrl_get_bank(const struct meson_pinctrl *pc,
>>  					  unsigned int pin);
> 

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

Am 17.05.2017 um 01:16 schrieb Jerome Brunet:
> On Tue, 2017-05-16 at 20:31 +0200, Heiner Kallweit wrote:
>> Am 16.05.2017 um 09:54 schrieb Neil Armstrong:
>>> Hi Heiner,
>>>
>>> On 05/15/2017 09:00 PM, Heiner Kallweit wrote:
>>>> Am 15.05.2017 um 10:05 schrieb Neil Armstrong:
>>>>> On 05/12/2017 09:14 PM, Heiner Kallweit wrote:
>>>>>> Add support for GPIO interrupts on Amlogic Meson SoC's.
>>>>>>
>>>>>> There's a limit of 8 parent interupts which can be used in total.
>>>>>> Note that IRQ_TYPE_EDGE_BOTH interrupts reserve two parent IRQ's,
>>>>>> one for each edge.
>>>>>>
>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>>> ---
>>>>>> v2:
>>>>>> - make the GPIO IRQ controller a separate driver
>>>>>> - several smaller improvements
>>>>>> ---
>>>>>>  drivers/pinctrl/Kconfig                   |   1 +
>>>>>>  drivers/pinctrl/meson/Makefile            |   2 +-
>>>>>>  drivers/pinctrl/meson/pinctrl-meson-irq.c | 367
>>>>>> ++++++++++++++++++++++++++++++
>>>>>
>>>>> Hi Heiner,
>>>>>
>>>>> This code has nothing to do with GPIOs on pinmux handling here, what we
>>>>> figured with Jerome
>>>>> is that this must be an independent IRQ Controller in drivers/irqchip.
>>>>>
>>>>
>>>> I'm not convinced and would like to hear more opinions on that. I see it
>>>> like this:
>>>> The driver implements an irqchip, right. But it's not an interrupt
>>>> controller.
>>>> Due to using GPIOLIB_IRQCHIP the gpio controller now has an own irq domain
>>>> (one for ao
>>>> and one for periphs GPIO domain). Therefore the gpio-controller now also
>>>> acts as
>>>> interrupt-controller. And both gpio (and interrupt) controllers just use
>>>> the irqchip
>>>> exposed by the new driver.
>>>> Last but not least the irqchip can be used with GPIOs only.
>>>
>>> In fact it's an interrupt controller since it can mask/unmask and control a
>>> singal that
>>> can wake up an interrupt for the ARM Core.
>>>
>>> Please look at the STM32 EXTI code, the design is quite similar except they
>>> don't have a
>>> dynamic management of links, but fixed ones.
>>> They have a proper independant IRQCHIP driver and a link from the pinctrl
>>> driver, and this
>>> should be the right design.
>>> They have a flaw since they do the mapping from the gpio_to_irq, and Linus
>>> won't allow
>>> this anymore.
>>>
>>
>> At first I involve Rob as he also provided feedback regarding the DT part.
>>
>> I had a look at the STM32 EXTI code and it looks very similar to Jerome's
>> version.
>> Actually I'd assume that the first Meson driver attempt was modeled after
>> STM32 EXTI.
> Well, no. EXTI has been merged when I already submitted the RFC for this driver,
> but that's not the point I suppose.
> 
>>
>> As you just mentioned there are at least two issues:
>> 1. The mapping can be requested via gpio_to_irq but it doesn't have to. A
>> driver could
> Maybe you missed it in our previous discussion with Linus but, no you can't
> create mapping in gpio_to_irq. This callback is fast and creating mappings might
> sleep because of the irq_domain mutex
> 
> https://patchwork.ozlabs.org/patch/684208/
> 
My comment was misunderstandable. What I meant was that a driver doesn't have to
use gpio_to_irq to request an interrupt, this can also be done via DT.

>>    also request a GPIO IRQ via DT.
>> 2. Missing is the handling of IRQ_TYPE_EDGE_BOTH which requires the allocation
>> of two
>>    parent interrupts.
>>
>> When looking at the drivers under drivers/irqchip basically all of them
>> implement not
>> only an irqchip but also an IRQ domain. Providing an IRQ domain seems to me to
>> be
>> the main criteria whether something should be considered an interrupt
>> controller
>> (please correct me if this understanding is wrong).
>>
>> The STM32 EXTI drivers seems to me to be unnecessarily complex due to not
>> using
>> GPIOLIB_IRQCHIP. This GPIO framework extension can hide a significant part of
>> the
>> GPIO IRQ complexity.
> I can only speculate regarding the design choice of STM EXTI, but I suppose the
> EXTI controller is independent of the pinmux/gpio subsystem HW wise. It's only
> weakly linked to the gpios, in the way that you have (or can create) a "map"
> between the gpio and the irq line number provided.
> 
> That is also the case for the amlogic gpio-irq. I suppose that's why Neil
> suggested to look at EXTI. That's also why I commented that this should be first
> implemented as an independent controller of the gpio subsys  (so in
> drivers/irqchip)
> 
> You may find it more complex, which is arguable, but it is a more accurate
> representation of the HW.
> 
>>
>> Coming back to my version:
>> The new driver just implements an irqchip, no IRQ domain.
>>  This irqchip then is
>> provided to the IRQ domains bound to the respective gpio controllers (for AO
>> and PERIPHS
>> GPIO domain) - thanks to GPIOLIB_IRQCHIP handling of the IRQ domains comes for
>> free.
>>
>> Adding the interrupt-controller property to the gpio-controller nodes is one
>> thing
>> still missing in my patch series. Then a driver can request a GPIO IRQ also
>> via DT, e.g.:
>>
>> interrupt-parent = <&gpio>;
>> interrupts = <GPIOZ_0 IRQ_TYPE_EDGE_BOTH>;
>>
>> interrupt-parent = <&gpio_ao>;
>> interrupts = <GPIOAO_0 IRQ_TYPE_EDGE_BOTH>;
>>
> 
> Which is also possible in the series, where the controller is gpio-irq device
> itself. This is what the HW provide, so it should be possible. It would be nice
> to have your way working as well, which I think is possible (PSB)
> 
>> Advantage of having an IRQ domain per GPIO controller is being able to to use
>> the GPIO
>> defines as is. Or in other words:
>> GPIOAO_0 and GPIOZ_0 both have the value 0. This works here due to having one
>> IRQ domain
>> per GPIO controller (both using the same new irqchip).
>>
> 
> I think you are onto something. As I told you previously, the problem was to
> create the mappings in pinctrl w/o allocating the parent. I struggled with that
> back in November and I had no time to really get back to it since.
> 
> Creating domains in each gpio controller, w/ all the mapping created at startup,
>   stacked to the domain provided by the driver/irqchip would solve the problem.
> 
> We would just have to call find_mapping in gpio_to_irq, which is fine and
> allocate the parent irq in the request_ressource callback.
> 
In request_resource we don't know the irq type yet (and therefore whether we need
one or wo parent interrupts). IMHO we can't get completely rid of allocating
parents in set_type.

>> And all I had to do is implementing one irq_chip and changing one line in the
>> existing
>> pinctrl driver.
>> Whilst I have to admit that e.g. calling request_irq for a parent irq from the
>> irq_set_type
>> callback may be something people find ugly and hacky.
>>
> The fact is that the HW does not directly support IRQ_EDGE_BOTH. Yes, it would
> be nice to find a way to support it anyway. There two possibilities for it:
> * Change the change the edge type depending on the next expected edge: Marc
> already stated that he is firmly against that. It is indeed not very robust
> * Allocate and deallocate additional parent irq to get secondary irq in
> set_type: This indeed looks hacky, I'd like to get the view of irq maintainers
> on this.
> 
Support for IRQ_TYPE_EDGE_BOTH I'd consider a mandatory feature. It's needed
e.g. by the SD card detection (drivers/mmc/slot-gpio.c).

> 
>> Compared to this implementation the STM32 EXTI drivers needs significantly
>> more data
>> structures, e.g. irqchip + irq domain in the irqchip driver, then hierarchical
>> IRQ domain
>> handling + further irqchip and irq domain + irq domain ops in the pinctrl
>> driver.
> 
> Within reasonable limits, having more or less data, code lines does not make a
> driver better or worse. 
> 
>>
>> Last but not least coming back to the initial talk with Rob about where to
>> best place the
>> DT docu for this irqchip implementation:
>> If acceptable I'd prefer to do it like .e.g in
>> bindings/pinctrl/allwinner,sunxi-pinctrl.txt
>> or bindings/pinctrl/atmel,at91-pio4-pinctrl.txt.
>> There the interrupt-controller properties are documented as part of the
>> pinctrl driver and
>> not separately under bindings/interrupt-controller.
>>
>> Maybe this explains a little bit better why I chose this approach.
>>
>> Rgds, Heiner
>>
>>>>
>>>> In the irqchip implementation we need the SoC-specific mapping from GPIO
>>>> number
>>>> to internal GPIO IRQ number. Having to export this from
>>>> drivers/pinctrl/meson for use
>>>> under drivers/irqchip most likely would also cause objections.
>>>
>>> You won't need, this interrupt controller will take the number either from
>>> DT either
>>> from a mapping creating from the pinctrl driver. The link will only be
>>> through the
>>> irq subsystem.
>>>
>>>>
>>>> So far my impression is that the very specific way GPIO IRQ's are handled
>>>> on Meson
>>>> doesn't fit perfectly into the current IRQ subsystem. Therefore the
>>>> discussion
>>>> about Jerome's version didn't result in the IRQ maintainers stating: do it
>>>> this way ..
>>>> Having said that most likely every possible approach is going to raise
>>>> some concerns.
>>>
>>> It doesn't fit exactly, but the subsystem can certainly be used to achieve
>>> it either by
>>> using all it's capacity or by eventually discussing with the maintainers to
>>> adapt it.
>>>
>>> Jerome has some hints hot to achieve the pinctrl part with everyone "happy".
>>>
>>>>
>>>>> Please move it and make independent, you should be able to request irqs
>>>>> without any links
>>>>> to the pinmux/gpio since physically the GPIO lines input are always
>>>>> connected to this
>>>>> irq controller, and the pinmux has no impact on the interrupt management
>>>>> here.
>>>>>> From the GPIO-IRQ Controller perspective, the GPIOs are only a number
>>>>>> and the pinmux code
>>>>>
>>>>> is only here to make the translation to a specific GPIO to a GPIO-IRQ
>>>>> number.
>>>>>
>>>>> For more chance to have it upstreamed in the right way, we should :
>>>>> 1) Collaborate, we can chat over IRC, maybe Slack, E-mail, skype,
>>>>> forums, ...
>>>>> 2) Push an independent IRQ controller that matches the capacity of the
>>>>> HW
>>>>> 3) Push a link from the pinctrl driver to have the to_gpio_irq mapping
>>>>> done in the right way
>>>>>
>>>>> Jerome spent quite a lot of time and had a chat with the IRQ subsystem
>>>>> maintainers to have s
>>>>> clear image of how this should be implemented, and it would be a good
>>>>> point to actually
>>>>> have a chat with them to elaborate find a strong solution.
>>>>>
>>>>
>>>> I know and I really appreciate Jerome's work and his discussion with the
>>>> IRQ maintainers.
>>>> My current attempt was inspired by his work.
>>>> However the discussion last year ended w/o result and the topic of GPIO
>>>> IRQs has been dead
>>>> since then. And I think discussing approaches works best based on a
>>>> concrete piece of code.
>>>> Therefore I submitted my version as discussion basis. I didn't expect that
>>>> everybody would
>>>> be totally happy with it and it would go to mainline unchanged.
>>>
>>> Sure, thanks for the work,
>>>
>>>>
>>>>> I'm sorry to say that pushing this code without really understanding how
>>>>> and why will lead to
>>>>> nothing expect frustration from everybody.
>>>>>
>>>>
>>>> I can only speak for myself: I'm not frustrated and I can live with
>>>> critical review comments.
>>>
>>> Great ! Anyway thanks for your work and I hope this will lead to mainline !
>>>
>>>> Regards, Heiner
>>>>
>>>>> Neil
>>>
>>> Neil
>>>
>>> [...]
>>>
>>
>>
> 

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

On Wed, 2017-05-17 at 22:36 +0200, Heiner Kallweit wrote:
> Am 17.05.2017 um 01:16 schrieb Jerome Brunet:
> > On Tue, 2017-05-16 at 20:31 +0200, Heiner Kallweit wrote:

[snip]

> > 
> > Maybe you missed it in our previous discussion with Linus but, no you can't
> > create mapping in gpio_to_irq. This callback is fast and creating mappings
> > might
> > sleep because of the irq_domain mutex
> > 
> > https://patchwork.ozlabs.org/patch/684208/
> > 
> 
> My comment was misunderstandable. What I meant was that a driver doesn't have
> to
> use gpio_to_irq to request an interrupt, this can also be done via DT.
> 

If you don't provide gpio_to_irq, you won't support the drivers which only
request a gpio through DT (because they need to read the pin state) and later
request an irq from it (to get an event and avoid polling)

That's a fairly common use-case. IMHO, this callback should be implemented by
the gpio controller, when it is an interrupt controller.

[snip]

> > > I think you are onto something. As I told you previously, the problem was
> > > to
> > create the mappings in pinctrl w/o allocating the parent. I struggled with
> > that
> > back in November and I had no time to really get back to it since.
> > 
> > Creating domains in each gpio controller, w/ all the mapping created at
> > startup,
> >   stacked to the domain provided by the driver/irqchip would solve the
> > problem.
> > 
> > We would just have to call find_mapping in gpio_to_irq, which is fine and
> > allocate the parent irq in the request_ressource callback.
> > 
> 
> In request_resource we don't know the irq type yet (and therefore whether we
> need
> one or wo parent interrupts). IMHO we can't get completely rid of allocating
> parents in set_type.

Yes you can. The HW does *not* support IRQ_BOTH on an irq line. You are trying
to come up with a SW workaround to get this feature. W/O this workaround, there
no reason to allocate a parent irq in set_type.
At this point, no one has come up with a clean solution to add this feature to
the meson-gpio-irq device.
Maybe evolutions in the irq framework will allow this cleanly in the future, I
don't think this is the case right now. 

[snip]
> 
> > > And all I had to do is implementing one irq_chip and changing one line in
> > > the
> > > existing
> > > pinctrl driver.
> > > Whilst I have to admit that e.g. calling request_irq for a parent irq from
> > > the
> > > irq_set_type
> > > callback may be something people find ugly and hacky.
> > > 
> > 
> > The fact is that the HW does not directly support IRQ_EDGE_BOTH. Yes, it
> > would
> > be nice to find a way to support it anyway. There two possibilities for it:
> > * Change the change the edge type depending on the next expected edge: Marc
> > already stated that he is firmly against that. It is indeed not very robust
> > * Allocate and deallocate additional parent irq to get secondary irq in
> > set_type: This indeed looks hacky, I'd like to get the view of irq
> > maintainers
> > on this.
> > 
> 
> Support for IRQ_TYPE_EDGE_BOTH I'd consider a mandatory feature. It's needed
> e.g. by the SD card detection (drivers/mmc/slot-gpio.c).
> 
"You'd consider" maybe, but the fact is that it is not mandatory. Having irq
based card detect (or buttons, or jack insert, or whatever) is probably more
elegant, but the polled version also works well. Elegant and mandatory are not
the same things



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

Am 23.05.2017 um 10:38 schrieb Jerome Brunet:
> On Wed, 2017-05-17 at 22:36 +0200, Heiner Kallweit wrote:
>> Am 17.05.2017 um 01:16 schrieb Jerome Brunet:
>>> On Tue, 2017-05-16 at 20:31 +0200, Heiner Kallweit wrote:
> 
> [snip]
> 
>>>
>>> Maybe you missed it in our previous discussion with Linus but, no you can't
>>> create mapping in gpio_to_irq. This callback is fast and creating mappings
>>> might
>>> sleep because of the irq_domain mutex
>>>
>>> https://patchwork.ozlabs.org/patch/684208/
>>>
>>
>> My comment was misunderstandable. What I meant was that a driver doesn't have
>> to
>> use gpio_to_irq to request an interrupt, this can also be done via DT.
>>
> 
> If you don't provide gpio_to_irq, you won't support the drivers which only
> request a gpio through DT (because they need to read the pin state) and later
> request an irq from it (to get an event and avoid polling)
> 
> That's a fairly common use-case. IMHO, this callback should be implemented by
> the gpio controller, when it is an interrupt controller.
> 
gio_to_irq is provided by the GPIOLIB_IRQCHIP core. So fortunately we don't
have to take care.

> [snip]
> 
>>>> I think you are onto something. As I told you previously, the problem was
>>>> to
>>> create the mappings in pinctrl w/o allocating the parent. I struggled with
>>> that
>>> back in November and I had no time to really get back to it since.
>>>
>>> Creating domains in each gpio controller, w/ all the mapping created at
>>> startup,
>>>   stacked to the domain provided by the driver/irqchip would solve the
>>> problem.
>>>
>>> We would just have to call find_mapping in gpio_to_irq, which is fine and
>>> allocate the parent irq in the request_ressource callback.
>>>
>>
>> In request_resource we don't know the irq type yet (and therefore whether we
>> need
>> one or wo parent interrupts). IMHO we can't get completely rid of allocating
>> parents in set_type.
> 
> Yes you can. The HW does *not* support IRQ_BOTH on an irq line. You are trying
> to come up with a SW workaround to get this feature. W/O this workaround, there
> no reason to allocate a parent irq in set_type.
> At this point, no one has come up with a clean solution to add this feature to
> the meson-gpio-irq device.
> Maybe evolutions in the irq framework will allow this cleanly in the future, I
> don't think this is the case right now. 
> 
Sure, it's a SW workaround. However working on a solution not supporting
IRQ_TYPE_EDGE_BOTH I'd consider wasted energy (YMMV).
Waiting for a future irq framework extension most likely is no solution as we
need a solution before production of Amlogic Meson chips ends ..

I will come up with a new version of the patch set addressing (most of) the
review comments. Then we have a updated basis for further discussion.

Thanks for the review / remarks.

> [snip]
>>
>>>> And all I had to do is implementing one irq_chip and changing one line in
>>>> the
>>>> existing
>>>> pinctrl driver.
>>>> Whilst I have to admit that e.g. calling request_irq for a parent irq from
>>>> the
>>>> irq_set_type
>>>> callback may be something people find ugly and hacky.
>>>>
>>>
>>> The fact is that the HW does not directly support IRQ_EDGE_BOTH. Yes, it
>>> would
>>> be nice to find a way to support it anyway. There two possibilities for it:
>>> * Change the change the edge type depending on the next expected edge: Marc
>>> already stated that he is firmly against that. It is indeed not very robust
>>> * Allocate and deallocate additional parent irq to get secondary irq in
>>> set_type: This indeed looks hacky, I'd like to get the view of irq
>>> maintainers
>>> on this.
>>>
>>
>> Support for IRQ_TYPE_EDGE_BOTH I'd consider a mandatory feature. It's needed
>> e.g. by the SD card detection (drivers/mmc/slot-gpio.c).
>>
> "You'd consider" maybe, but the fact is that it is not mandatory. Having irq
> based card detect (or buttons, or jack insert, or whatever) is probably more
> elegant, but the polled version also works well. Elegant and mandatory are not
> the same things
> 
> 
> 
> 

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