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

[Heiner Kallweit <hkallweit1@xxxxxxxxx>]

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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxx>
>> + * Based on a first version by Jerome Brunet <jbrunet@xxxxxxxxxxxx>
>> + *
>> + * 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html