| Message ID | 89d02a38-7386-fcdc-4dce-ea7e531c90b4@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 8, 2017 at 9:38 PM, Heiner Kallweit <hkallweit1@gmail.com> wrote: > Add a driver supporting the GPIO interrupt controller on certain > Amlogic meson SoC's. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij -- 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 Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote: > Add a driver supporting the GPIO interrupt controller on certain > Amlogic meson SoC's. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > v5: > - changed Kconfig entry based on Neil's suggestion > - added authors > - extended explanation why 2 * n hwirqs are used > v6: > - change DT property parent-interrupts to interrupts > --- > drivers/irqchip/Kconfig | 5 + > drivers/irqchip/Makefile | 1 + > drivers/irqchip/irq-meson-gpio.c | 295 > +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 302 insertions(+) > create mode 100644 drivers/irqchip/irq-meson-gpio.c > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index 478f8ace..bdc86e14 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER > help > Say yes here to add support for the IRQ combiner devices embedded > in Qualcomm Technologies chips. > + > +config MESON_GPIO_INTC > + bool > + depends on ARCH_MESON > + default y > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index b64c59b8..1be482bd 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq- > eznps.o > obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o > obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o > obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o > +obj-$(CONFIG_MESON_GPIO_INTC) += irq-meson-gpio.o > diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson- > gpio.c > new file mode 100644 > index 00000000..b09228ea > --- /dev/null > +++ b/drivers/irqchip/irq-meson-gpio.c > @@ -0,0 +1,295 @@ > +/* > + * Amlogic Meson GPIO IRQ chip driver > + * > + * Copyright (c) 2015 Endless Mobile, Inc. > + * Author: Carlo Caione <carlo@endlessm.com> > + * Copyright (c) 2016 BayLibre, SAS. > + * Author: Jerome Brunet <jbrunet@baylibre.com> > + * Copyright (c) 2017 Heiner Kallweit <hkallweit1@gmail.com> > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation, version 2. > + */ > + > +#include <linux/device.h> > +#include <linux/init.h> > +#include <linux/interrupt.h> > +#include <linux/irqchip.h> > +#include <linux/of.h> > +#include <linux/of_irq.h> > +#include <linux/of_address.h> > +#include <linux/regmap.h> > + > +#define REG_EDGE_POL 0x00 > +#define REG_PIN_03_SEL 0x04 > +#define REG_PIN_47_SEL 0x08 > +#define REG_FILTER_SEL 0x0c > + > +#define REG_EDGE_POL_MASK(x) (BIT(x) | BIT(16 + (x))) > +#define REG_EDGE_POL_EDGE(x) BIT(x) > +#define REG_EDGE_POL_LOW(x) BIT(16 + (x)) > + > +#define MAX_PARENT_IRQ_NUM 8 > + > +/* maximum number of GPIO IRQs on supported platforms */ > +#define MAX_NUM_GPIO_IRQ 133 Only valid on gxbb. Not gxl nor meson8 > + > +/* > + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each > + * edge. That's due to HW constraints. > + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one > + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq > + * one bit to the right. > + */ > +#define NUM_GPIO_HWIRQ (2 * MAX_NUM_GPIO_IRQ) PSB > + > +struct meson_irq_slot { > + unsigned int irq; > + unsigned int owner; > +}; > + > +struct meson_data { > + struct regmap *regmap; > + struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM]; > + unsigned int num_slots; > + struct mutex lock; > +}; > + > +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq) > +{ > + int i, slot = -ENOSPC; > + > + mutex_lock(&md->lock); > + > + for (i = 0; i < md->num_slots && slot < 0; i++) > + if (!md->slots[i].owner) { > + md->slots[i].owner = virq; > + slot = i; > + } > + > + mutex_unlock(&md->lock); > + > + return slot; > +} > + > +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq) > +{ > + int i; > + > + mutex_lock(&md->lock); > + > + for (i = 0; i < md->num_slots; i++) > + if (md->slots[i].owner == virq) { > + md->slots[i].owner = 0; > + break; > + } > + > + mutex_unlock(&md->lock); > +} > + > +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq) > +{ > + int i, slot = -EINVAL; > + > + mutex_lock(&md->lock); > + > + for (i = 0; i < md->num_slots && slot < 0; i++) > + if (md->slots[i].owner == virq) > + slot = i; > + > + mutex_unlock(&md->lock); > + > + return slot; > +} > + > +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int > hwirq) > +{ > + int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL; > + int shift = 8 * (idx % 4); > + > + regmap_update_bits(md->regmap, reg, 0xff << shift, > + hwirq << shift); Access to the registers should be somehow protected. Several irqs use the same registers. There is a potential race condition here. > +} > + > +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq) > +{ > + struct meson_data *md = data->domain->host_data; > + int slot = meson_find_irq_slot(md, data->irq); > + > + if (slot >= 0) > + meson_set_hwirq(md, slot, hwirq); > +} > + > +static int meson_irq_set_type(struct irq_data *data, unsigned int type) > +{ > + struct meson_data *md = data->domain->host_data; > + int slot; > + unsigned int val = 0; > + > + if (type == IRQ_TYPE_EDGE_BOTH) > + return -EINVAL; > + > + slot = meson_find_irq_slot(md, data->irq); > + if (slot < 0) > + return slot; > + > + if (type & IRQ_TYPE_EDGE_BOTH) > + val |= REG_EDGE_POL_EDGE(slot); > + > + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) > + val |= REG_EDGE_POL_LOW(slot); > + > + regmap_update_bits(md->regmap, REG_EDGE_POL, > + REG_EDGE_POL_MASK(slot), val); Same here. > + > + if (type & IRQ_TYPE_EDGE_BOTH) > + val = IRQ_TYPE_EDGE_RISING; > + else > + val = IRQ_TYPE_LEVEL_HIGH; > + > + return irq_chip_set_type_parent(data, val); > +} > + > +static unsigned int meson_irq_startup(struct irq_data *data) > +{ > + irq_chip_unmask_parent(data); > + /* > + * An extra bit was added to allow having the same gpio hwirq twice > + * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the > + * gpio hwirq. > + */ > + meson_irq_set_hwirq(data, data->hwirq >> 1); Please keep in mind that any device can use this controller as irq parent. It has to make sense, even when not serving the gpio driver. This hack means that, in DT, we'd have to multiply by 2 the values given under section "22.3 GPIO interrupts" of the datasheet. This is an example Linux specific stuff in DT. It also means that the controller declares a lot more lines that it really has ... This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the mapping from the irq_set_type callback of the GPIO driver, which is still think should be dropped at this point. > + > + return 0; > +} > + > +static void meson_irq_shutdown(struct irq_data *data) > +{ > + meson_irq_set_hwirq(data, 0xff); > + irq_chip_mask_parent(data); > +} > + > +static struct irq_chip meson_irq_chip = { > + .name = "meson_gpio_intc", > + .irq_set_type = meson_irq_set_type, > + .irq_eoi = irq_chip_eoi_parent, > + .irq_mask = irq_chip_mask_parent, > + .irq_unmask = irq_chip_unmask_parent, > + .irq_startup = meson_irq_startup, > + .irq_shutdown = meson_irq_shutdown, > + .irq_set_affinity = irq_chip_set_affinity_parent, > +}; > + > +static int meson_irq_alloc(struct irq_domain *d, unsigned int virq, > + unsigned int nr_irqs, void *data) > +{ > + struct irq_fwspec parent_fwspec, *fwspec = data; > + struct meson_data *md = d->host_data; > + irq_hw_number_t hwirq; > + int ret, slot; > + > + slot = meson_alloc_irq_slot(md, virq); > + if (slot < 0) > + return slot; > + > + hwirq = fwspec->param[0]; > + irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL); > + > + parent_fwspec.fwnode = d->parent->fwnode; > + parent_fwspec.param_count = 3; > + parent_fwspec.param[0] = 0; /* SPI */ > + parent_fwspec.param[1] = md->slots[slot].irq; > + parent_fwspec.param[2] = IRQ_TYPE_NONE; > + > + ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec); > + if (ret) > + meson_free_irq_slot(md, virq); > + > + return ret; > +} > + > +static void meson_irq_free(struct irq_domain *d, unsigned int virq, > + unsigned int nr_irqs) > +{ > + struct meson_data *md = d->host_data; > + > + irq_domain_free_irqs_common(d, virq, nr_irqs); > + meson_free_irq_slot(md, virq); > +} > + > +static const struct irq_domain_ops meson_irq_ops = { > + .alloc = meson_irq_alloc, > + .free = meson_irq_free, > + .xlate = irq_domain_xlate_twocell, This is a hierarchic domain, shouldn't it implement translate rather than xlate ? > +}; > + > +static int meson_get_irqs(struct meson_data *md, struct device_node *node) > +{ > + int ret, i; > + u32 irq; > + > + for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) { > + ret = of_property_read_u32_index(node, "interrupts", i, > &irq); > + if (ret) > + break; > + md->slots[i].irq = irq; > + } > + > + md->num_slots = i; > + > + return i ? 0 : -EINVAL; > +} > + > +static const struct regmap_config meson_regmap_config = { > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > + .max_register = REG_FILTER_SEL, > +}; > + > +static int __init meson_gpio_irq_init(struct device_node *node, > + struct device_node *parent) > +{ > + struct irq_domain *meson_irq_domain, *parent_domain; > + struct meson_data *md; > + void __iomem *io_base; > + int ret; > + > + md = kzalloc(sizeof(*md), GFP_KERNEL); > + if (!md) > + return -ENOMEM; > + > + mutex_init(&md->lock); > + > + io_base = of_iomap(node, 0); > + if (!io_base) > + return -EINVAL; > + > + md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config); > + if (IS_ERR(md->regmap)) > + return PTR_ERR(md->regmap); > + > + /* initialize to IRQ_TYPE_LEVEL_HIGH */ > + regmap_write(md->regmap, REG_EDGE_POL, 0); > + /* disable all GPIO interrupt sources */ > + regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff); > + regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff); > + /* disable filtering */ > + regmap_write(md->regmap, REG_FILTER_SEL, 0); > + > + ret = meson_get_irqs(md, node); > + if (ret) > + return ret; > + > + parent_domain = irq_find_host(parent); > + if (!parent_domain) > + return -ENXIO; > + > + meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0, > + NUM_GPIO_HWIRQ, node, > + &meson_irq_ops, md); > + return meson_irq_domain ? 0 : -EINVAL; > +} > + > +IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc", > meson_gpio_irq_init); -- To unsubscribe from this list: send the line "unsubscribe 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 09.06.2017 um 11:07 schrieb Jerome Brunet: > On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote: >> Add a driver supporting the GPIO interrupt controller on certain >> Amlogic meson SoC's. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> v5: >> - changed Kconfig entry based on Neil's suggestion >> - added authors >> - extended explanation why 2 * n hwirqs are used >> v6: >> - change DT property parent-interrupts to interrupts >> --- >> drivers/irqchip/Kconfig | 5 + >> drivers/irqchip/Makefile | 1 + >> drivers/irqchip/irq-meson-gpio.c | 295 >> +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 302 insertions(+) >> create mode 100644 drivers/irqchip/irq-meson-gpio.c >> >> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig >> index 478f8ace..bdc86e14 100644 >> --- a/drivers/irqchip/Kconfig >> +++ b/drivers/irqchip/Kconfig >> @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER >> help >> Say yes here to add support for the IRQ combiner devices embedded >> in Qualcomm Technologies chips. >> + >> +config MESON_GPIO_INTC >> + bool >> + depends on ARCH_MESON >> + default y >> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile >> index b64c59b8..1be482bd 100644 >> --- a/drivers/irqchip/Makefile >> +++ b/drivers/irqchip/Makefile >> @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq- >> eznps.o >> obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o >> obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o >> obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o >> +obj-$(CONFIG_MESON_GPIO_INTC) += irq-meson-gpio.o >> diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson- >> gpio.c >> new file mode 100644 >> index 00000000..b09228ea >> --- /dev/null >> +++ b/drivers/irqchip/irq-meson-gpio.c >> @@ -0,0 +1,295 @@ >> +/* >> + * Amlogic Meson GPIO IRQ chip driver >> + * >> + * Copyright (c) 2015 Endless Mobile, Inc. >> + * Author: Carlo Caione <carlo@endlessm.com> >> + * Copyright (c) 2016 BayLibre, SAS. >> + * Author: Jerome Brunet <jbrunet@baylibre.com> >> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1@gmail.com> >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License as >> + * published by the Free Software Foundation, version 2. >> + */ >> + >> +#include <linux/device.h> >> +#include <linux/init.h> >> +#include <linux/interrupt.h> >> +#include <linux/irqchip.h> >> +#include <linux/of.h> >> +#include <linux/of_irq.h> >> +#include <linux/of_address.h> >> +#include <linux/regmap.h> >> + >> +#define REG_EDGE_POL 0x00 >> +#define REG_PIN_03_SEL 0x04 >> +#define REG_PIN_47_SEL 0x08 >> +#define REG_FILTER_SEL 0x0c >> + >> +#define REG_EDGE_POL_MASK(x) (BIT(x) | BIT(16 + (x))) >> +#define REG_EDGE_POL_EDGE(x) BIT(x) >> +#define REG_EDGE_POL_LOW(x) BIT(16 + (x)) >> + >> +#define MAX_PARENT_IRQ_NUM 8 >> + >> +/* maximum number of GPIO IRQs on supported platforms */ >> +#define MAX_NUM_GPIO_IRQ 133 > > Only valid on gxbb. Not gxl nor meson8 > This max value is meant to be the max for all supported chips. Meson8 and GXL have less gpio irq sources. >> + >> +/* >> + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each >> + * edge. That's due to HW constraints. >> + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one >> + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq >> + * one bit to the right. >> + */ >> +#define NUM_GPIO_HWIRQ (2 * MAX_NUM_GPIO_IRQ) > > PSB > >> + >> +struct meson_irq_slot { >> + unsigned int irq; >> + unsigned int owner; >> +}; >> + >> +struct meson_data { >> + struct regmap *regmap; >> + struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM]; >> + unsigned int num_slots; >> + struct mutex lock; >> +}; >> + >> +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq) >> +{ >> + int i, slot = -ENOSPC; >> + >> + mutex_lock(&md->lock); >> + >> + for (i = 0; i < md->num_slots && slot < 0; i++) >> + if (!md->slots[i].owner) { >> + md->slots[i].owner = virq; >> + slot = i; >> + } >> + >> + mutex_unlock(&md->lock); >> + >> + return slot; >> +} >> + >> +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq) >> +{ >> + int i; >> + >> + mutex_lock(&md->lock); >> + >> + for (i = 0; i < md->num_slots; i++) >> + if (md->slots[i].owner == virq) { >> + md->slots[i].owner = 0; >> + break; >> + } >> + >> + mutex_unlock(&md->lock); >> +} >> + >> +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq) >> +{ >> + int i, slot = -EINVAL; >> + >> + mutex_lock(&md->lock); >> + >> + for (i = 0; i < md->num_slots && slot < 0; i++) >> + if (md->slots[i].owner == virq) >> + slot = i; >> + >> + mutex_unlock(&md->lock); >> + >> + return slot; >> +} >> + >> +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int >> hwirq) >> +{ >> + int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL; >> + int shift = 8 * (idx % 4); >> + >> + regmap_update_bits(md->regmap, reg, 0xff << shift, >> + hwirq << shift); > > Access to the registers should be somehow protected. Several irqs use the same > registers. There is a potential race condition here. > regmap comes with an integrated locking. >> +} >> + >> +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq) >> +{ >> + struct meson_data *md = data->domain->host_data; >> + int slot = meson_find_irq_slot(md, data->irq); >> + >> + if (slot >= 0) >> + meson_set_hwirq(md, slot, hwirq); >> +} >> + >> +static int meson_irq_set_type(struct irq_data *data, unsigned int type) >> +{ >> + struct meson_data *md = data->domain->host_data; >> + int slot; >> + unsigned int val = 0; >> + >> + if (type == IRQ_TYPE_EDGE_BOTH) >> + return -EINVAL; >> + >> + slot = meson_find_irq_slot(md, data->irq); >> + if (slot < 0) >> + return slot; >> + >> + if (type & IRQ_TYPE_EDGE_BOTH) >> + val |= REG_EDGE_POL_EDGE(slot); >> + >> + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) >> + val |= REG_EDGE_POL_LOW(slot); >> + >> + regmap_update_bits(md->regmap, REG_EDGE_POL, >> + REG_EDGE_POL_MASK(slot), val); > > Same here. > > >> + >> + if (type & IRQ_TYPE_EDGE_BOTH) >> + val = IRQ_TYPE_EDGE_RISING; >> + else >> + val = IRQ_TYPE_LEVEL_HIGH; >> + >> + return irq_chip_set_type_parent(data, val); >> +} >> + >> +static unsigned int meson_irq_startup(struct irq_data *data) >> +{ >> + irq_chip_unmask_parent(data); >> + /* >> + * An extra bit was added to allow having the same gpio hwirq twice >> + * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the >> + * gpio hwirq. >> + */ >> + meson_irq_set_hwirq(data, data->hwirq >> 1); > > Please keep in mind that any device can use this controller as irq parent. > It has to make sense, even when not serving the gpio driver. > Yes, theoretically there could be other drivers accessing this gpio irq controller. But most likely they would face the same issue too (support for IRQ_TYPE_EDGE_BOTH) and is there any realistic foreseeable use case where another driver would access the gpio irq controller? For me the question is: Do we want to go with something that is 80% nice, supports all major use cases and is available now (it can still be improved later)? Or do we want to go on waiting (like since Oct 2016) for something that suddenly steps out of the dark, is 100% nice and covers all corner cases? I'm also a big fan of proper architecture. But I'm also somewhat result-driven. W/o this I'm afraid I would have lost my job long ago. Last but not least I think what we have here is still much better than most vendor drivers. And v6 of the patch set already includes major changes based on the various review comments. > This hack means that, in DT, we'd have to multiply by 2 the values given under > section "22.3 GPIO interrupts" of the datasheet. This is an example Linux > specific stuff in DT. > It also means that the controller declares a lot more lines that it really has > ... > > This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the > mapping from the irq_set_type callback of the GPIO driver, which is still think > should be dropped at this point. > >> + >> + return 0; >> +} >> + >> +static void meson_irq_shutdown(struct irq_data *data) >> +{ >> + meson_irq_set_hwirq(data, 0xff); >> + irq_chip_mask_parent(data); >> +} >> + >> +static struct irq_chip meson_irq_chip = { >> + .name = "meson_gpio_intc", >> + .irq_set_type = meson_irq_set_type, >> + .irq_eoi = irq_chip_eoi_parent, >> + .irq_mask = irq_chip_mask_parent, >> + .irq_unmask = irq_chip_unmask_parent, >> + .irq_startup = meson_irq_startup, >> + .irq_shutdown = meson_irq_shutdown, >> + .irq_set_affinity = irq_chip_set_affinity_parent, >> +}; >> + >> +static int meson_irq_alloc(struct irq_domain *d, unsigned int virq, >> + unsigned int nr_irqs, void *data) >> +{ >> + struct irq_fwspec parent_fwspec, *fwspec = data; >> + struct meson_data *md = d->host_data; >> + irq_hw_number_t hwirq; >> + int ret, slot; >> + >> + slot = meson_alloc_irq_slot(md, virq); >> + if (slot < 0) >> + return slot; >> + >> + hwirq = fwspec->param[0]; >> + irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL); >> + >> + parent_fwspec.fwnode = d->parent->fwnode; >> + parent_fwspec.param_count = 3; >> + parent_fwspec.param[0] = 0; /* SPI */ >> + parent_fwspec.param[1] = md->slots[slot].irq; >> + parent_fwspec.param[2] = IRQ_TYPE_NONE; >> + >> + ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec); >> + if (ret) >> + meson_free_irq_slot(md, virq); >> + >> + return ret; >> +} >> + >> +static void meson_irq_free(struct irq_domain *d, unsigned int virq, >> + unsigned int nr_irqs) >> +{ >> + struct meson_data *md = d->host_data; >> + >> + irq_domain_free_irqs_common(d, virq, nr_irqs); >> + meson_free_irq_slot(md, virq); >> +} >> + >> +static const struct irq_domain_ops meson_irq_ops = { >> + .alloc = meson_irq_alloc, >> + .free = meson_irq_free, >> + .xlate = irq_domain_xlate_twocell, > > This is a hierarchic domain, shouldn't it implement translate rather than xlate > ? > If translate isn't defined there's a fallback to xlate. irq_domain_xlate_twocell comes for free so I didn't see any benefit in re-implementing it as a translate callback. >> +}; >> + >> +static int meson_get_irqs(struct meson_data *md, struct device_node *node) >> +{ >> + int ret, i; >> + u32 irq; >> + >> + for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) { >> + ret = of_property_read_u32_index(node, "interrupts", i, >> &irq); >> + if (ret) >> + break; >> + md->slots[i].irq = irq; >> + } >> + >> + md->num_slots = i; >> + >> + return i ? 0 : -EINVAL; >> +} >> + >> +static const struct regmap_config meson_regmap_config = { >> + .reg_bits = 32, >> + .reg_stride = 4, >> + .val_bits = 32, >> + .max_register = REG_FILTER_SEL, >> +}; >> + >> +static int __init meson_gpio_irq_init(struct device_node *node, >> + struct device_node *parent) >> +{ >> + struct irq_domain *meson_irq_domain, *parent_domain; >> + struct meson_data *md; >> + void __iomem *io_base; >> + int ret; >> + >> + md = kzalloc(sizeof(*md), GFP_KERNEL); >> + if (!md) >> + return -ENOMEM; >> + >> + mutex_init(&md->lock); >> + >> + io_base = of_iomap(node, 0); >> + if (!io_base) >> + return -EINVAL; >> + >> + md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config); >> + if (IS_ERR(md->regmap)) >> + return PTR_ERR(md->regmap); >> + >> + /* initialize to IRQ_TYPE_LEVEL_HIGH */ >> + regmap_write(md->regmap, REG_EDGE_POL, 0); >> + /* disable all GPIO interrupt sources */ >> + regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff); >> + regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff); >> + /* disable filtering */ >> + regmap_write(md->regmap, REG_FILTER_SEL, 0); >> + >> + ret = meson_get_irqs(md, node); >> + if (ret) >> + return ret; >> + >> + parent_domain = irq_find_host(parent); >> + if (!parent_domain) >> + return -ENXIO; >> + >> + meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0, >> + NUM_GPIO_HWIRQ, node, >> + &meson_irq_ops, md); >> + return meson_irq_domain ? 0 : -EINVAL; >> +} >> + >> +IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc", >> meson_gpio_irq_init); > > -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jerome Brunet <jbrunet@baylibre.com> writes: > On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote: >> Add a driver supporting the GPIO interrupt controller on certain >> Amlogic meson SoC's. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> [...] >> +static unsigned int meson_irq_startup(struct irq_data *data) >> +{ >> + irq_chip_unmask_parent(data); >> + /* >> + * An extra bit was added to allow having the same gpio hwirq twice >> + * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the >> + * gpio hwirq. >> + */ >> + meson_irq_set_hwirq(data, data->hwirq >> 1); > > Please keep in mind that any device can use this controller as irq parent. > It has to make sense, even when not serving the gpio driver. > > This hack means that, in DT, we'd have to multiply by 2 the values given under > section "22.3 GPIO interrupts" of the datasheet. This is an example Linux > specific stuff in DT. > It also means that the controller declares a lot more lines that it really has > ... > > This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the > mapping from the irq_set_type callback of the GPIO driver, which is still think > should be dropped at this point. +1 Please drop the hack for IRQ_TYPE_BOTH so we can reach agreement on the basic design and functionality. The gymnastics required to support this hack (due to broken hardware) are getting in the way of getting basic functionality merged. Also, we already know from previous discussions between Jerome and the IRQ maintainers that hacks for IRQ_TYPE_BOTH like this have been very thoroughly rejected. So while the IRQ maintainers haven't chimed in on this series, we have a very good idea what they will say when they do. So please, pretty please, let's avoid giving the IRQ maintainers a fun reason to NAK, and drop the IRQ_TYPE_BOTH stuff until we have an agreed upon way to support the hardware that actually works. Thanks, Kevin -- 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 09.06.2017 um 23:15 schrieb Kevin Hilman: > Jerome Brunet <jbrunet@baylibre.com> writes: > >> On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote: >>> Add a driver supporting the GPIO interrupt controller on certain >>> Amlogic meson SoC's. >>> >>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > [...] > >>> +static unsigned int meson_irq_startup(struct irq_data *data) >>> +{ >>> + irq_chip_unmask_parent(data); >>> + /* >>> + * An extra bit was added to allow having the same gpio hwirq twice >>> + * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the >>> + * gpio hwirq. >>> + */ >>> + meson_irq_set_hwirq(data, data->hwirq >> 1); >> >> Please keep in mind that any device can use this controller as irq parent. >> It has to make sense, even when not serving the gpio driver. >> >> This hack means that, in DT, we'd have to multiply by 2 the values given under >> section "22.3 GPIO interrupts" of the datasheet. This is an example Linux >> specific stuff in DT. >> It also means that the controller declares a lot more lines that it really has >> ... >> >> This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the >> mapping from the irq_set_type callback of the GPIO driver, which is still think >> should be dropped at this point. > > +1 > > Please drop the hack for IRQ_TYPE_BOTH so we can reach agreement on the > basic design and functionality. The gymnastics required to support this > hack (due to broken hardware) are getting in the way of getting basic > functionality merged. > I haven't heard any feedback on the other proposal yet: Always reserve two parent irq's in the request_resources callback, then set_type is easy. How about this one? Having max. 4 gpio irq's should be a fair price for a cleaner design. Rgds, Heiner > Also, we already know from previous discussions between Jerome and the > IRQ maintainers that hacks for IRQ_TYPE_BOTH like this have been very > thoroughly rejected. So while the IRQ maintainers haven't chimed in on > this series, we have a very good idea what they will say when they do. > > So please, pretty please, let's avoid giving the IRQ maintainers a fun > reason to NAK, and drop the IRQ_TYPE_BOTH stuff until we have an agreed > upon way to support the hardware that actually works. > > Thanks, > > Kevin > -- 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
Heiner Kallweit <hkallweit1@gmail.com> writes: > Am 09.06.2017 um 23:15 schrieb Kevin Hilman: >> Jerome Brunet <jbrunet@baylibre.com> writes: >> >>> On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote: >>>> Add a driver supporting the GPIO interrupt controller on certain >>>> Amlogic meson SoC's. >>>> >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> >> [...] >> >>>> +static unsigned int meson_irq_startup(struct irq_data *data) >>>> +{ >>>> + irq_chip_unmask_parent(data); >>>> + /* >>>> + * An extra bit was added to allow having the same gpio hwirq twice >>>> + * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the >>>> + * gpio hwirq. >>>> + */ >>>> + meson_irq_set_hwirq(data, data->hwirq >> 1); >>> >>> Please keep in mind that any device can use this controller as irq parent. >>> It has to make sense, even when not serving the gpio driver. >>> >>> This hack means that, in DT, we'd have to multiply by 2 the values given under >>> section "22.3 GPIO interrupts" of the datasheet. This is an example Linux >>> specific stuff in DT. >>> It also means that the controller declares a lot more lines that it really has >>> ... >>> >>> This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the >>> mapping from the irq_set_type callback of the GPIO driver, which is still think >>> should be dropped at this point. >> >> +1 >> >> Please drop the hack for IRQ_TYPE_BOTH so we can reach agreement on the >> basic design and functionality. The gymnastics required to support this >> hack (due to broken hardware) are getting in the way of getting basic >> functionality merged. >> > I haven't heard any feedback on the other proposal yet: > Always reserve two parent irq's in the request_resources callback, > then set_type is easy. How about this one? The feedback is the same: let's consider that after getting the basics reviewed, accepted and merged. > Having max. 4 gpio irq's should be a fair price for a cleaner design. Yuck, but better than no support for both _BOTH I guess. But again, discussion of hacks for the hardware limitation is getting in the way discussing and merging the basics. Let's get the basics right first, then add functionality. I understand it can be rather frustrating to give up features and/or functionality for having something "clean", but unfortunately, that's how the upstream linux kernel community has always worked. Most maintainers prefer clean, well-designed and maintainable code that comes in small, easily reviewable chunks over something that's difficult to understand with hacks and workarounds. Even if that means a limited feature set initially. Additional functionality (even if hacky) is much easier to review, discuss and maintain *later* when it's on top of a starting point that is clean. Kevin -- 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
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 478f8ace..bdc86e14 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -301,3 +301,8 @@ config QCOM_IRQ_COMBINER help Say yes here to add support for the IRQ combiner devices embedded in Qualcomm Technologies chips. + +config MESON_GPIO_INTC + bool + depends on ARCH_MESON + default y diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index b64c59b8..1be482bd 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -76,3 +76,4 @@ obj-$(CONFIG_EZNPS_GIC) += irq-eznps.o obj-$(CONFIG_ARCH_ASPEED) += irq-aspeed-vic.o obj-$(CONFIG_STM32_EXTI) += irq-stm32-exti.o obj-$(CONFIG_QCOM_IRQ_COMBINER) += qcom-irq-combiner.o +obj-$(CONFIG_MESON_GPIO_INTC) += irq-meson-gpio.o diff --git a/drivers/irqchip/irq-meson-gpio.c b/drivers/irqchip/irq-meson-gpio.c new file mode 100644 index 00000000..b09228ea --- /dev/null +++ b/drivers/irqchip/irq-meson-gpio.c @@ -0,0 +1,295 @@ +/* + * Amlogic Meson GPIO IRQ chip driver + * + * Copyright (c) 2015 Endless Mobile, Inc. + * Author: Carlo Caione <carlo@endlessm.com> + * Copyright (c) 2016 BayLibre, SAS. + * Author: Jerome Brunet <jbrunet@baylibre.com> + * Copyright (c) 2017 Heiner Kallweit <hkallweit1@gmail.com> + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation, version 2. + */ + +#include <linux/device.h> +#include <linux/init.h> +#include <linux/interrupt.h> +#include <linux/irqchip.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/of_address.h> +#include <linux/regmap.h> + +#define REG_EDGE_POL 0x00 +#define REG_PIN_03_SEL 0x04 +#define REG_PIN_47_SEL 0x08 +#define REG_FILTER_SEL 0x0c + +#define REG_EDGE_POL_MASK(x) (BIT(x) | BIT(16 + (x))) +#define REG_EDGE_POL_EDGE(x) BIT(x) +#define REG_EDGE_POL_LOW(x) BIT(16 + (x)) + +#define MAX_PARENT_IRQ_NUM 8 + +/* maximum number of GPIO IRQs on supported platforms */ +#define MAX_NUM_GPIO_IRQ 133 + +/* + * In case of IRQ_TYPE_EDGE_BOTH we need two parent interrupts, one for each + * edge. That's due to HW constraints. + * We use format 2 * GPIO_HWIRQ +(0|1) for the hwirq, so we can have one + * GPIO_HWIRQ twice and derive the GPIO_HWIRQ from hwirq by shifting hwirq + * one bit to the right. + */ +#define NUM_GPIO_HWIRQ (2 * MAX_NUM_GPIO_IRQ) + +struct meson_irq_slot { + unsigned int irq; + unsigned int owner; +}; + +struct meson_data { + struct regmap *regmap; + struct meson_irq_slot slots[MAX_PARENT_IRQ_NUM]; + unsigned int num_slots; + struct mutex lock; +}; + +static int meson_alloc_irq_slot(struct meson_data *md, unsigned int virq) +{ + int i, slot = -ENOSPC; + + mutex_lock(&md->lock); + + for (i = 0; i < md->num_slots && slot < 0; i++) + if (!md->slots[i].owner) { + md->slots[i].owner = virq; + slot = i; + } + + mutex_unlock(&md->lock); + + return slot; +} + +static void meson_free_irq_slot(struct meson_data *md, unsigned int virq) +{ + int i; + + mutex_lock(&md->lock); + + for (i = 0; i < md->num_slots; i++) + if (md->slots[i].owner == virq) { + md->slots[i].owner = 0; + break; + } + + mutex_unlock(&md->lock); +} + +static int meson_find_irq_slot(struct meson_data *md, unsigned int virq) +{ + int i, slot = -EINVAL; + + mutex_lock(&md->lock); + + for (i = 0; i < md->num_slots && slot < 0; i++) + if (md->slots[i].owner == virq) + slot = i; + + mutex_unlock(&md->lock); + + return slot; +} + +static void meson_set_hwirq(struct meson_data *md, int idx, unsigned int hwirq) +{ + int reg = idx > 3 ? REG_PIN_47_SEL : REG_PIN_03_SEL; + int shift = 8 * (idx % 4); + + regmap_update_bits(md->regmap, reg, 0xff << shift, + hwirq << shift); +} + +static void meson_irq_set_hwirq(struct irq_data *data, unsigned int hwirq) +{ + struct meson_data *md = data->domain->host_data; + int slot = meson_find_irq_slot(md, data->irq); + + if (slot >= 0) + meson_set_hwirq(md, slot, hwirq); +} + +static int meson_irq_set_type(struct irq_data *data, unsigned int type) +{ + struct meson_data *md = data->domain->host_data; + int slot; + unsigned int val = 0; + + if (type == IRQ_TYPE_EDGE_BOTH) + return -EINVAL; + + slot = meson_find_irq_slot(md, data->irq); + if (slot < 0) + return slot; + + if (type & IRQ_TYPE_EDGE_BOTH) + val |= REG_EDGE_POL_EDGE(slot); + + if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_EDGE_FALLING)) + val |= REG_EDGE_POL_LOW(slot); + + regmap_update_bits(md->regmap, REG_EDGE_POL, + REG_EDGE_POL_MASK(slot), val); + + if (type & IRQ_TYPE_EDGE_BOTH) + val = IRQ_TYPE_EDGE_RISING; + else + val = IRQ_TYPE_LEVEL_HIGH; + + return irq_chip_set_type_parent(data, val); +} + +static unsigned int meson_irq_startup(struct irq_data *data) +{ + irq_chip_unmask_parent(data); + /* + * An extra bit was added to allow having the same gpio hwirq twice + * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the + * gpio hwirq. + */ + meson_irq_set_hwirq(data, data->hwirq >> 1); + + return 0; +} + +static void meson_irq_shutdown(struct irq_data *data) +{ + meson_irq_set_hwirq(data, 0xff); + irq_chip_mask_parent(data); +} + +static struct irq_chip meson_irq_chip = { + .name = "meson_gpio_intc", + .irq_set_type = meson_irq_set_type, + .irq_eoi = irq_chip_eoi_parent, + .irq_mask = irq_chip_mask_parent, + .irq_unmask = irq_chip_unmask_parent, + .irq_startup = meson_irq_startup, + .irq_shutdown = meson_irq_shutdown, + .irq_set_affinity = irq_chip_set_affinity_parent, +}; + +static int meson_irq_alloc(struct irq_domain *d, unsigned int virq, + unsigned int nr_irqs, void *data) +{ + struct irq_fwspec parent_fwspec, *fwspec = data; + struct meson_data *md = d->host_data; + irq_hw_number_t hwirq; + int ret, slot; + + slot = meson_alloc_irq_slot(md, virq); + if (slot < 0) + return slot; + + hwirq = fwspec->param[0]; + irq_domain_set_hwirq_and_chip(d, virq, hwirq, &meson_irq_chip, NULL); + + parent_fwspec.fwnode = d->parent->fwnode; + parent_fwspec.param_count = 3; + parent_fwspec.param[0] = 0; /* SPI */ + parent_fwspec.param[1] = md->slots[slot].irq; + parent_fwspec.param[2] = IRQ_TYPE_NONE; + + ret = irq_domain_alloc_irqs_parent(d, virq, nr_irqs, &parent_fwspec); + if (ret) + meson_free_irq_slot(md, virq); + + return ret; +} + +static void meson_irq_free(struct irq_domain *d, unsigned int virq, + unsigned int nr_irqs) +{ + struct meson_data *md = d->host_data; + + irq_domain_free_irqs_common(d, virq, nr_irqs); + meson_free_irq_slot(md, virq); +} + +static const struct irq_domain_ops meson_irq_ops = { + .alloc = meson_irq_alloc, + .free = meson_irq_free, + .xlate = irq_domain_xlate_twocell, +}; + +static int meson_get_irqs(struct meson_data *md, struct device_node *node) +{ + int ret, i; + u32 irq; + + for (i = 0; i < MAX_PARENT_IRQ_NUM; i++) { + ret = of_property_read_u32_index(node, "interrupts", i, &irq); + if (ret) + break; + md->slots[i].irq = irq; + } + + md->num_slots = i; + + return i ? 0 : -EINVAL; +} + +static const struct regmap_config meson_regmap_config = { + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, + .max_register = REG_FILTER_SEL, +}; + +static int __init meson_gpio_irq_init(struct device_node *node, + struct device_node *parent) +{ + struct irq_domain *meson_irq_domain, *parent_domain; + struct meson_data *md; + void __iomem *io_base; + int ret; + + md = kzalloc(sizeof(*md), GFP_KERNEL); + if (!md) + return -ENOMEM; + + mutex_init(&md->lock); + + io_base = of_iomap(node, 0); + if (!io_base) + return -EINVAL; + + md->regmap = regmap_init_mmio(NULL, io_base, &meson_regmap_config); + if (IS_ERR(md->regmap)) + return PTR_ERR(md->regmap); + + /* initialize to IRQ_TYPE_LEVEL_HIGH */ + regmap_write(md->regmap, REG_EDGE_POL, 0); + /* disable all GPIO interrupt sources */ + regmap_write(md->regmap, REG_PIN_03_SEL, 0xffffffff); + regmap_write(md->regmap, REG_PIN_47_SEL, 0xffffffff); + /* disable filtering */ + regmap_write(md->regmap, REG_FILTER_SEL, 0); + + ret = meson_get_irqs(md, node); + if (ret) + return ret; + + parent_domain = irq_find_host(parent); + if (!parent_domain) + return -ENXIO; + + meson_irq_domain = irq_domain_add_hierarchy(parent_domain, 0, + NUM_GPIO_HWIRQ, node, + &meson_irq_ops, md); + return meson_irq_domain ? 0 : -EINVAL; +} + +IRQCHIP_DECLARE(meson_gpio_irq, "amlogic,meson-gpio-intc", meson_gpio_irq_init);
Add a driver supporting the GPIO interrupt controller on certain Amlogic meson SoC's. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v5: - changed Kconfig entry based on Neil's suggestion - added authors - extended explanation why 2 * n hwirqs are used v6: - change DT property parent-interrupts to interrupts --- drivers/irqchip/Kconfig | 5 + drivers/irqchip/Makefile | 1 + drivers/irqchip/irq-meson-gpio.c | 295 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 302 insertions(+) create mode 100644 drivers/irqchip/irq-meson-gpio.c