| Message ID | 20210914100415.1549208-3-daniel@0x0f.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SigmaStar SSD20XD GPIO interrupt controller | expand |
On Tue, 14 Sep 2021 11:04:14 +0100, Daniel Palmer <daniel@0x0f.com> wrote: > > Add support for the SigmaStar GPIO interrupt controller, gpi, > that is present in SSD201 and SSD202D SoCs. > > This routes interrupts from many interrupts into a single > interrupt that is connected to the peripheral interrupt > controller. > > Signed-off-by: Daniel Palmer <daniel@0x0f.com> > --- > MAINTAINERS | 1 + > drivers/irqchip/Kconfig | 11 ++ > drivers/irqchip/Makefile | 2 + > drivers/irqchip/irq-ssd20xd-gpi.c | 211 ++++++++++++++++++++++++++++++ > 4 files changed, 225 insertions(+) > create mode 100644 drivers/irqchip/irq-ssd20xd-gpi.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 3004c0f735b6..487d5e62c287 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -2248,6 +2248,7 @@ F: arch/arm/boot/dts/mstar-* > F: arch/arm/mach-mstar/ > F: drivers/clk/mstar/ > F: drivers/gpio/gpio-msc313.c > +F: drivers/irqchip/irq-ssd20xd-gpi.c > F: drivers/watchdog/msc313e_wdt.c > F: include/dt-bindings/clock/mstar-* > F: include/dt-bindings/gpio/msc313-gpio.h > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig > index 4d5924e9f766..8786aed7f776 100644 > --- a/drivers/irqchip/Kconfig > +++ b/drivers/irqchip/Kconfig > @@ -582,6 +582,17 @@ config MST_IRQ > help > Support MStar Interrupt Controller. > > +config SSD20XD_GPI > + bool "SigmaStar SSD20xD GPIO Interrupt Controller" > + depends on ARCH_MSTARV7 || COMPILE_TEST > + default ARCH_MSTARV7 > + select IRQ_DOMAIN > + select IRQ_DOMAIN_HIERARCHY > + select REGMAP > + help > + Support for the SigmaStar GPIO interrupt controller > + found in SSD201, SSD202D and others. > + > config WPCM450_AIC > bool "Nuvoton WPCM450 Advanced Interrupt Controller" > depends on ARCH_WPCM450 > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile > index f88cbf36a9d2..1a6c3dbd67a8 100644 > --- a/drivers/irqchip/Makefile > +++ b/drivers/irqchip/Makefile > @@ -116,3 +116,5 @@ obj-$(CONFIG_MACH_REALTEK_RTL) += irq-realtek-rtl.o > obj-$(CONFIG_WPCM450_AIC) += irq-wpcm450-aic.o > obj-$(CONFIG_IRQ_IDT3243X) += irq-idt3243x.o > obj-$(CONFIG_APPLE_AIC) += irq-apple-aic.o > +obj-$(CONFIG_SSD20XD_GPI) += irq-ssd20xd-gpi.o > + > diff --git a/drivers/irqchip/irq-ssd20xd-gpi.c b/drivers/irqchip/irq-ssd20xd-gpi.c > new file mode 100644 > index 000000000000..c34f41380717 > --- /dev/null > +++ b/drivers/irqchip/irq-ssd20xd-gpi.c > @@ -0,0 +1,211 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp> > + */ > + > +#include <linux/irq.h> > +#include <linux/irqchip.h> > +#include <linux/irqdomain.h> > +#include <linux/irqchip/chained_irq.h> > +#include <linux/of.h> > +#include <linux/of_address.h> > +#include <linux/of_irq.h> > +#include <linux/mfd/syscon.h> > +#include <linux/regmap.h> > +#include <linux/interrupt.h> > + > +#define NUM_IRQ 76 > +#define IRQS_PER_REG 16 > +#define STRIDE 4 > + > +#define REG_MASK 0x0 > +#define REG_ACK 0x28 > +#define REG_TYPE 0x40 > +#define REG_STATUS 0xc0 > + > +struct ssd20xd_gpi { > + struct regmap *regmap; > + struct irq_domain *domain; > +}; > + > +#define REG_OFFSET(_hwirq) ((hwirq >> 4) * STRIDE) > +#define BIT_OFFSET(_hwirq) (1 << (hwirq & 0xf)) > + > +static void ssd20xd_gpi_mask_irq(struct irq_data *data) > +{ > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); > + int offset_reg = REG_OFFSET(hwirq); > + int offset_bit = BIT_OFFSET(hwirq); > + > + regmap_update_bits(gpi->regmap, REG_MASK + offset_reg, offset_bit, offset_bit); > +} > + > +static void ssd20xd_gpi_unmask_irq(struct irq_data *data) > +{ > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); > + int offset_reg = REG_OFFSET(hwirq); > + int offset_bit = BIT_OFFSET(hwirq); > + > + regmap_update_bits(gpi->regmap, REG_MASK + offset_reg, offset_bit, 0); Is this regmap call atomic? When running this, you are holding a raw_spinlock already. From what I can see, this is unlikely to work correctly with the current state of regmap. > +} > + > +static void ssd20xd_gpi_irq_eoi(struct irq_data *data) > +{ > + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + int offset_reg = REG_OFFSET(hwirq); > + int offset_bit = BIT_OFFSET(hwirq); > + > + regmap_update_bits_base(gpi->regmap, REG_ACK + offset_reg, > + offset_bit, offset_bit, NULL, false, true); > +} > + > +static int ssd20xd_gpi_set_type_irq(struct irq_data *data, unsigned int flow_type) > +{ > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); > + int offset_reg = REG_OFFSET(hwirq); > + int offset_bit = BIT_OFFSET(hwirq); > + > + switch (flow_type) { > + case IRQ_TYPE_EDGE_FALLING: > + regmap_update_bits(gpi->regmap, REG_TYPE + offset_reg, offset_bit, offset_bit); > + break; > + case IRQ_TYPE_EDGE_RISING: > + regmap_update_bits(gpi->regmap, REG_TYPE + offset_reg, offset_bit, 0); > + break; > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > +static struct irq_chip ssd20xd_gpi_chip = { > + .name = "GPI", > + .irq_mask = ssd20xd_gpi_mask_irq, > + .irq_unmask = ssd20xd_gpi_unmask_irq, > + .irq_eoi = ssd20xd_gpi_irq_eoi, > + .irq_set_type = ssd20xd_gpi_set_type_irq, > +}; > + > +static int ssd20xd_gpi_domain_alloc(struct irq_domain *domain, unsigned int virq, > + unsigned int nr_irqs, void *arg) > +{ > + struct ssd20xd_gpi *intc = domain->host_data; > + struct irq_fwspec *fwspec = arg; > + int i; > + > + for (i = 0; i < nr_irqs; i++) > + irq_domain_set_info(domain, virq + i, fwspec->param[0] + i, > + &ssd20xd_gpi_chip, intc, handle_fasteoi_irq, NULL, NULL); > + > + return 0; > +} > + > +static void ssd20xd_gpi_domain_free(struct irq_domain *domain, unsigned int virq, > + unsigned int nr_irqs) > +{ > + int i; > + > + for (i = 0; i < nr_irqs; i++) { > + struct irq_data *d = irq_domain_get_irq_data(domain, virq + i); > + > + irq_set_handler(virq + i, NULL); > + irq_domain_reset_irq_data(d); > + } > +} > + > +static const struct irq_domain_ops ssd20xd_gpi_domain_ops = { > + .alloc = ssd20xd_gpi_domain_alloc, > + .free = ssd20xd_gpi_domain_free, > +}; > + > +static const struct regmap_config ssd20xd_gpi_regmap_config = { > + .reg_bits = 16, > + .val_bits = 16, > + .reg_stride = 4, > +}; > + > +static void ssd20x_gpi_chainedhandler(struct irq_desc *desc) > +{ > + struct ssd20xd_gpi *intc = irq_desc_get_handler_data(desc); > + struct irq_chip *chip = irq_desc_get_chip(desc); > + unsigned int irqbit, hwirq, virq, status; > + int i; > + > + chained_irq_enter(chip, desc); > + > + for (i = 0; i < NUM_IRQ / IRQS_PER_REG; i++) { > + int offset_reg = STRIDE * i; > + int offset_irq = IRQS_PER_REG * i; > + > + regmap_read(intc->regmap, REG_STATUS + offset_reg, &status); Does this act as an ACK in the HW? > + > + while (status) { > + irqbit = __ffs(status); > + hwirq = offset_irq + irqbit; > + virq = irq_find_mapping(intc->domain, hwirq); > + if (virq) > + generic_handle_irq(virq); Please replace this with generic_handle_domain_irq(). > + status &= ~BIT(irqbit); > + } > + } > + > + chained_irq_exit(chip, desc); > +} > + > +static int __init ssd20xd_gpi_of_init(struct device_node *node, > + struct device_node *parent) > +{ > + struct ssd20xd_gpi *intc; > + void __iomem *base; > + int irq, ret; > + > + intc = kzalloc(sizeof(*intc), GFP_KERNEL); > + if (!intc) > + return -ENOMEM; > + > + base = of_iomap(node, 0); > + if (!base) { > + ret = -ENODEV; > + goto out_free; > + } > + > + intc->regmap = regmap_init_mmio(NULL, base, &ssd20xd_gpi_regmap_config); > + if (IS_ERR(intc->regmap)) { > + ret = PTR_ERR(intc->regmap); > + goto out_unmap; > + } > + > + intc->domain = irq_domain_add_linear(node, NUM_IRQ, &ssd20xd_gpi_domain_ops, intc); > + if (!intc->domain) { > + ret = -ENOMEM; > + goto out_free_regmap; > + } > + > + irq = of_irq_get(node, 0); > + if (irq <= 0) { > + ret = irq; > + goto out_free_domain; > + } > + > + irq_set_chained_handler_and_data(irq, ssd20x_gpi_chainedhandler, > + intc); > + > + return 0; > + > +out_free_domain: > + irq_domain_remove(intc->domain); > +out_free_regmap: > + regmap_exit(intc->regmap); > +out_unmap: > + iounmap(base); > +out_free: > + kfree(intc); > + return ret; > +} > + > +IRQCHIP_DECLARE(ssd20xd_gpi, "sstar,ssd20xd-gpi", ssd20xd_gpi_of_init); Is there any reason why this isn't a standard platform device? In general, irqchips that are not part of the root hierarchy shouldn't need this anymore. M.
Hi Marc, On Mon, 20 Sept 2021 at 18:45, Marc Zyngier <maz@kernel.org> wrote: > > +static void ssd20xd_gpi_unmask_irq(struct irq_data *data) > > +{ > > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > > + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); > > + int offset_reg = REG_OFFSET(hwirq); > > + int offset_bit = BIT_OFFSET(hwirq); > > + > > + regmap_update_bits(gpi->regmap, REG_MASK + offset_reg, offset_bit, 0); > > Is this regmap call atomic? When running this, you are holding a > raw_spinlock already. From what I can see, this is unlikely to work > correctly with the current state of regmap. I didn't even think about it. I will check. > > +static void ssd20x_gpi_chainedhandler(struct irq_desc *desc) > > +{ > > + struct ssd20xd_gpi *intc = irq_desc_get_handler_data(desc); > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > + unsigned int irqbit, hwirq, virq, status; > > + int i; > > + > > + chained_irq_enter(chip, desc); > > + > > + for (i = 0; i < NUM_IRQ / IRQS_PER_REG; i++) { > > + int offset_reg = STRIDE * i; > > + int offset_irq = IRQS_PER_REG * i; > > + > > + regmap_read(intc->regmap, REG_STATUS + offset_reg, &status); > > Does this act as an ACK in the HW? Not that I'm aware of. The status registers have the interrupt bits set until the EOI callback is called from what I can tell. Technically I think the EOI callback should actually be ACK instead but from my fuzzy memory with the stack of IRQ controllers that resulted in a null pointer dereference. > > + > > + while (status) { > > + irqbit = __ffs(status); > > + hwirq = offset_irq + irqbit; > > + virq = irq_find_mapping(intc->domain, hwirq); > > + if (virq) > > + generic_handle_irq(virq); > > Please replace this with generic_handle_domain_irq(). Ok. > > +IRQCHIP_DECLARE(ssd20xd_gpi, "sstar,ssd20xd-gpi", ssd20xd_gpi_of_init); > > Is there any reason why this isn't a standard platform device? In > general, irqchips that are not part of the root hierarchy shouldn't > need this anymore. Nope. I can switch that over. Cheers, Daniel
On Mon, 20 Sep 2021 11:05:26 +0100, Daniel Palmer <daniel@0x0f.com> wrote: > > Hi Marc, > > On Mon, 20 Sept 2021 at 18:45, Marc Zyngier <maz@kernel.org> wrote: > > > +static void ssd20xd_gpi_unmask_irq(struct irq_data *data) > > > +{ > > > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > > > + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); > > > + int offset_reg = REG_OFFSET(hwirq); > > > + int offset_bit = BIT_OFFSET(hwirq); > > > + > > > + regmap_update_bits(gpi->regmap, REG_MASK + offset_reg, offset_bit, 0); > > > > Is this regmap call atomic? When running this, you are holding a > > raw_spinlock already. From what I can see, this is unlikely to work > > correctly with the current state of regmap. > > I didn't even think about it. I will check. You may want to enable lockdep to verify that. > > > > +static void ssd20x_gpi_chainedhandler(struct irq_desc *desc) > > > +{ > > > + struct ssd20xd_gpi *intc = irq_desc_get_handler_data(desc); > > > + struct irq_chip *chip = irq_desc_get_chip(desc); > > > + unsigned int irqbit, hwirq, virq, status; > > > + int i; > > > + > > > + chained_irq_enter(chip, desc); > > > + > > > + for (i = 0; i < NUM_IRQ / IRQS_PER_REG; i++) { > > > + int offset_reg = STRIDE * i; > > > + int offset_irq = IRQS_PER_REG * i; > > > + > > > + regmap_read(intc->regmap, REG_STATUS + offset_reg, &status); > > > > Does this act as an ACK in the HW? > > Not that I'm aware of. The status registers have the interrupt bits > set until the EOI callback is called from what I can tell. Then this doesn't work for edge signalling, as you will lose interrupts that occur between the handling and EOI. > Technically I think the EOI callback should actually be ACK instead > but from my fuzzy memory with the stack of IRQ controllers that > resulted in a null pointer dereference. That's probably because you are using the wrong flow handler. You should turn this irq_eoi into an irq_ack, because that's really what it is, and use handle_edge_irq() as the flow handler. Thanks, M.
Hi Marc, On Mon, 20 Sept 2021 at 20:24, Marc Zyngier <maz@kernel.org> wrote: > > > Is this regmap call atomic? When running this, you are holding a > > > raw_spinlock already. From what I can see, this is unlikely to work > > > correctly with the current state of regmap. > > > > I didn't even think about it. I will check. > > You may want to enable lockdep to verify that. I've just checked with lockdep and while it doesn't complain about this code it complains about similar code I have somewhere else that's almost identical (yet another interrupt controller driver I needed to write..). So it probably doesn't work properly as you say. I'll fix that up. > > Technically I think the EOI callback should actually be ACK instead > > but from my fuzzy memory with the stack of IRQ controllers that > > resulted in a null pointer dereference. > > That's probably because you are using the wrong flow handler. You > should turn this irq_eoi into an irq_ack, because that's really what > it is, and use handle_edge_irq() as the flow handler. If I do that (put the ack clearing callback into the ack slot, change the handler to handle_edge_irq()) I get this explosion: # gpiomon -r 0 44 [ 23.449571] 8<--- cut here --- [ 23.452673] Unable to handle kernel NULL pointer dereference at virtual address 00000000 [ 23.460804] pgd = (ptrval) [ 23.463534] [00000000] *pgd=23530835, *pte=00000000, *ppte=00000000 [ 23.469868] Internal error: Oops: 80000007 [#1] SMP ARM [ 23.475128] Modules linked in: [ 23.478211] CPU: 0 PID: 193 Comm: gpiomon Not tainted 5.15.0-rc2+ #2565 [ 23.484866] Hardware name: MStar/Sigmastar Armv7 (Device Tree) [ 23.490727] PC is at 0x0 [ 23.493283] LR is at handle_edge_irq+0x88/0xfc [ 23.497764] pc : [<00000000>] lr : [<c0180694>] psr: a0040193 [ 23.504064] sp : c2405d90 ip : 00000000 fp : c35a786c [ 23.509318] r10: 00000001 r9 : c1b96fc0 r8 : 00000020 [ 23.514572] r7 : 000000c8 r6 : c35a786c r5 : c35a7818 r4 : c35a7800 [ 23.521135] r3 : 00000000 r2 : 000015d6 r1 : 06f80000 r0 : c35a7818 This one is because the GPIO controller irqchip that is on top of this doesn't have an ack callback. So if I set irq_chip_ack_parent as the ack callback I get another explosion: # gpiomon -r 0 44 [ 22.370689] 8<--- cut here --- [ 22.373802] Unable to handle kernel NULL pointer dereference at virtual address 00000018 [ 22.381945] pgd = (ptrval) [ 22.384685] [00000018] *pgd=235cb835, *pte=00000000, *ppte=00000000 [ 22.391038] Internal error: Oops: 17 [#1] SMP ARM [ 22.395776] Modules linked in: [ 22.398860] CPU: 1 PID: 193 Comm: gpiomon Not tainted 5.15.0-rc2+ #2566 [ 22.405515] Hardware name: MStar/Sigmastar Armv7 (Device Tree) [ 22.411376] PC is at irq_chip_ack_parent+0x8/0x10 [ 22.416120] LR is at __irq_do_set_handler+0x3c/0x11c [ 22.421119] pc : [<c017f498>] lr : [<c018029c>] psr: a0040093 [ 22.427419] sp : c3505d68 ip : ffffe000 fp : 00000000 [ 22.432673] r10: c0d592d4 r9 : 00000001 r8 : 00000000 [ 22.437927] r7 : c3502618 r6 : 00000000 r5 : c017b9cc r4 : c3502600 [ 22.444489] r3 : 00000000 r2 : c10bb294 r1 : c10bb294 r0 : c26a3440 [ 22.451053] Flags: NzCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment user [ 22.458317] Control: 10c5387d Table: 235b006a DAC: 00000055 ---snip--- [ 22.725196] [<c017f498>] (irq_chip_ack_parent) from [<c018029c>] (__irq_do_set_handler+0x3c/0x11c) [ 22.734219] [<c018029c>] (__irq_do_set_handler) from [<c01803b4>] (__irq_set_handler+0x38/0x50) [ 22.742976] [<c01803b4>] (__irq_set_handler) from [<c0181880>] (irq_domain_set_info+0x34/0x48) [ 22.751649] [<c0181880>] (irq_domain_set_info) from [<c046f838>] (gpiochip_hierarchy_irq_domain_alloc+0x104/0x228) [ 22.762069] [<c046f838>] (gpiochip_hierarchy_irq_domain_alloc) from [<c0182c38>] (__irq_domain_alloc_irqs+0xd8/0x318) [ 22.772748] [<c0182c38>] (__irq_domain_alloc_irqs) from [<c01832e8>] (irq_create_fwspec_mapping+0x22c/0x298) [ 22.782641] [<c01832e8>] (irq_create_fwspec_mapping) from [<c0470124>] (gpiochip_to_irq+0x60/0x84) [ 22.791664] [<c0470124>] (gpiochip_to_irq) from [<c046ef18>] (gpiod_to_irq+0x48/0x60) [ 22.799552] [<c046ef18>] (gpiod_to_irq) from [<c0477a48>] (gpio_ioctl+0x1b4/0x420) [ 22.807178] [<c0477a48>] (gpio_ioctl) from [<c0262e4c>] (vfs_ioctl+0x20/0x38) [ 22.814371] [<c0262e4c>] (vfs_ioctl) from [<c0263708>] (sys_ioctl+0xb0/0x818) [ 22.821564] [<c0263708>] (sys_ioctl) from [<c0100060>] (ret_fast_syscall+0x0/0x1c) [ 22.829190] Exception stack(0xc3505fa8 to 0xc3505ff0) [ 22.834273] 5fa0: ???????? ???????? ???????? ???????? ???????? ???????? [ 22.842488] 5fc0: ???????? ???????? ???????? ???????? ???????? ???????? ???????? ???????? [ 22.850701] 5fe0: ???????? ???????? ???????? ???????? [ 22.855790] Code: e593301c e12fff13 e5900018 e5903010 (e5933018) [ 22.861919] ---[ end trace 10524aa06eced7e3 ]--- If I do something silly like the following diff the explosion stops and GPIO interrupt fires correctly each time I press the button it's connected to: diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index a98bcfc4be7b..969df9207358 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -1368,6 +1368,8 @@ EXPORT_SYMBOL_GPL(irq_chip_disable_parent); void irq_chip_ack_parent(struct irq_data *data) { data = data->parent_data; + if(!data->chip) + return; data->chip->irq_ack(data); } EXPORT_SYMBOL_GPL(irq_chip_ack_parent); I think I got stuck at this, switched it to the eoi handler instead and then forgot about it. I'll work out the proper solution for this for v2. Cheers, Daniel
Hi Marc, Sorry for the constant email. On Mon, 20 Sept 2021 at 20:24, Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 20 Sep 2021 11:05:26 +0100, > Daniel Palmer <daniel@0x0f.com> wrote: > > > > Hi Marc, > > > > On Mon, 20 Sept 2021 at 18:45, Marc Zyngier <maz@kernel.org> wrote: > > > > +static void ssd20xd_gpi_unmask_irq(struct irq_data *data) > > > > +{ > > > > + irq_hw_number_t hwirq = irqd_to_hwirq(data); > > > > + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); > > > > + int offset_reg = REG_OFFSET(hwirq); > > > > + int offset_bit = BIT_OFFSET(hwirq); > > > > + > > > > + regmap_update_bits(gpi->regmap, REG_MASK + offset_reg, offset_bit, 0); > > > > > > Is this regmap call atomic? When running this, you are holding a > > > raw_spinlock already. From what I can see, this is unlikely to work > > > correctly with the current state of regmap. > > > > I didn't even think about it. I will check. > > You may want to enable lockdep to verify that. After some research I think this can be solved by adding ".use_raw_spinlock = true" to the regmap config to force using a raw_spinlock instead of the default spinlock. This avoids having a spinlock inside of a raw_spinlock. lockdep doesn't actually complain about this currently but another interrupt controller driver I have uses a syscon because the interrupt registers are mixed in with unrelated stuff. lockdep is complaining about the spinlock inside of a raw_spinlock there. So I guess I'll need to add a new DT property for syscon to use raw_spinlocks for that driver. Cheers, Daniel
On Tue, 21 Sep 2021 05:16:35 +0100, Daniel Palmer <daniel@0x0f.com> wrote: + Linus. > So if I set irq_chip_ack_parent as the ack callback I get another explosion: > > # gpiomon -r 0 44 > [ 22.370689] 8<--- cut here --- > [ 22.373802] Unable to handle kernel NULL pointer dereference at > virtual address 00000018 > [ 22.381945] pgd = (ptrval) > [ 22.384685] [00000018] *pgd=235cb835, *pte=00000000, *ppte=00000000 > [ 22.391038] Internal error: Oops: 17 [#1] SMP ARM > [ 22.395776] Modules linked in: > [ 22.398860] CPU: 1 PID: 193 Comm: gpiomon Not tainted 5.15.0-rc2+ #2566 > [ 22.405515] Hardware name: MStar/Sigmastar Armv7 (Device Tree) > [ 22.411376] PC is at irq_chip_ack_parent+0x8/0x10 > [ 22.416120] LR is at __irq_do_set_handler+0x3c/0x11c > [ 22.421119] pc : [<c017f498>] lr : [<c018029c>] psr: a0040093 > [ 22.427419] sp : c3505d68 ip : ffffe000 fp : 00000000 > [ 22.432673] r10: c0d592d4 r9 : 00000001 r8 : 00000000 > [ 22.437927] r7 : c3502618 r6 : 00000000 r5 : c017b9cc r4 : c3502600 > [ 22.444489] r3 : 00000000 r2 : c10bb294 r1 : c10bb294 r0 : c26a3440 > [ 22.451053] Flags: NzCv IRQs off FIQs on Mode SVC_32 ISA ARM > Segment user > [ 22.458317] Control: 10c5387d Table: 235b006a DAC: 00000055 > ---snip--- > [ 22.725196] [<c017f498>] (irq_chip_ack_parent) from [<c018029c>] > (__irq_do_set_handler+0x3c/0x11c) > [ 22.734219] [<c018029c>] (__irq_do_set_handler) from [<c01803b4>] > (__irq_set_handler+0x38/0x50) > [ 22.742976] [<c01803b4>] (__irq_set_handler) from [<c0181880>] > (irq_domain_set_info+0x34/0x48) > [ 22.751649] [<c0181880>] (irq_domain_set_info) from [<c046f838>] > (gpiochip_hierarchy_irq_domain_alloc+0x104/0x228) > [ 22.762069] [<c046f838>] (gpiochip_hierarchy_irq_domain_alloc) from > [<c0182c38>] (__irq_domain_alloc_irqs+0xd8/0x318) > [ 22.772748] [<c0182c38>] (__irq_domain_alloc_irqs) from > [<c01832e8>] (irq_create_fwspec_mapping+0x22c/0x298) > [ 22.782641] [<c01832e8>] (irq_create_fwspec_mapping) from > [<c0470124>] (gpiochip_to_irq+0x60/0x84) > [ 22.791664] [<c0470124>] (gpiochip_to_irq) from [<c046ef18>] > (gpiod_to_irq+0x48/0x60) > [ 22.799552] [<c046ef18>] (gpiod_to_irq) from [<c0477a48>] > (gpio_ioctl+0x1b4/0x420) > [ 22.807178] [<c0477a48>] (gpio_ioctl) from [<c0262e4c>] (vfs_ioctl+0x20/0x38) > [ 22.814371] [<c0262e4c>] (vfs_ioctl) from [<c0263708>] (sys_ioctl+0xb0/0x818) > [ 22.821564] [<c0263708>] (sys_ioctl) from [<c0100060>] > (ret_fast_syscall+0x0/0x1c) > [ 22.829190] Exception stack(0xc3505fa8 to 0xc3505ff0) > [ 22.834273] 5fa0: ???????? ???????? ???????? > ???????? ???????? ???????? > [ 22.842488] 5fc0: ???????? ???????? ???????? ???????? ???????? > ???????? ???????? ???????? > [ 22.850701] 5fe0: ???????? ???????? ???????? ???????? > [ 22.855790] Code: e593301c e12fff13 e5900018 e5903010 (e5933018) > [ 22.861919] ---[ end trace 10524aa06eced7e3 ]--- This seems to be caused by your GPIO driver installing a flow handler (via irq_domain_set_info()), which is a bit odd. I would expect that only the root irqchip in the hierarchy would do that. At the point where this is called, the hierarchy isn't fully populated (the irq_domain_alloc_irqs_parent() call comes after that), and irq_chip_ack_parent() explodes as above. Linus: is there a reason why the gpiolib insist on setting its own handler while building the hierarchy? I guess this could be worked around by swapping the calls to irq_domain_set_info and irq_domain_alloc_irqs_parent, but having two levels of the hierarchy competing for the flow handler looks a bit odd. Thanks, M.
On Tue, Sep 21, 2021 at 10:27 AM Marc Zyngier <maz@kernel.org> wrote: > Linus: is there a reason why the gpiolib insist on setting its own > handler while building the hierarchy? Is it this? /* * We set handle_bad_irq because the .set_type() should * always be invoked and set the right type of handler. */ irq_domain_set_info(d, irq, hwirq, gc->irq.chip, gc, girq->handler, NULL, NULL); irq_set_probe(irq); (...) IIUC it's because sometimes, on elder systems (such as ixp4xx) some machines are still using boardfiles, and drivers are not obtaining IRQs dynamically from device tree or ACPI, instead they are set up statically at machine init. I assume it would otherwise be done as part of ops->translate? I suppose it could be solved with a patch that take this route only if we're not using device tree or ACPI? If this is the totally wrong answer then forgive me... a bit tired. Yours, Linus Walleij
Hi Linus, On Wed, 22 Sept 2021 at 03:23, Linus Walleij <linus.walleij@linaro.org> wrote: > I suppose it could be solved with a patch that take this route only if > we're not using device tree or ACPI? Is that something I could do with a small patch in my series or is there the potential to totally break everyone else's stuff to make mine work? Thanks, Daniel
[huh, missed this email...] On Tue, 21 Sep 2021 19:23:04 +0100, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Tue, Sep 21, 2021 at 10:27 AM Marc Zyngier <maz@kernel.org> wrote: > > > Linus: is there a reason why the gpiolib insist on setting its own > > handler while building the hierarchy? > > Is it this? > > /* > * We set handle_bad_irq because the .set_type() should > * always be invoked and set the right type of handler. > */ > irq_domain_set_info(d, > irq, > hwirq, > gc->irq.chip, > gc, > girq->handler, > NULL, NULL); > irq_set_probe(irq); > (...) It is its relative position wrt to irq_domain_alloc_irqs_parent() that has the potential for annoyance. irq_domain_set_info() will trigger an irq startup, which will explode if the parent level hasn't been initialised correctly. > > IIUC it's because sometimes, on elder systems (such as ixp4xx) some machines > are still using boardfiles, and drivers are not obtaining IRQs dynamically > from device tree or ACPI, instead they are set up statically at machine > init. > > I assume it would otherwise be done as part of ops->translate? No, this is the right spot if you really need to set the handler. But it should really be after the parent allocation (see below for something totally untested). Ultimately, setting the flow handler when there is a parent domain is a bit odd, as you'd expect the root domain to be in charge of the overall flow. Thanks, M. diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index abfbf546d159..53221d54c4be 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1103,19 +1103,6 @@ static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d, } chip_dbg(gc, "found parent hwirq %u\n", parent_hwirq); - /* - * We set handle_bad_irq because the .set_type() should - * always be invoked and set the right type of handler. - */ - irq_domain_set_info(d, - irq, - hwirq, - gc->irq.chip, - gc, - girq->handler, - NULL, NULL); - irq_set_probe(irq); - /* This parent only handles asserted level IRQs */ parent_arg = girq->populate_parent_alloc_arg(gc, parent_hwirq, parent_type); if (!parent_arg) @@ -1137,6 +1124,18 @@ static int gpiochip_hierarchy_irq_domain_alloc(struct irq_domain *d, parent_hwirq, hwirq); kfree(parent_arg); + + if (!ret) { + irq_domain_set_info(d, + irq, + hwirq, + gc->irq.chip, + gc, + girq->handler, + NULL, NULL); + irq_set_probe(irq); + } + return ret; }
On Thu, 30 Sep 2021 13:39:04 +0100, Daniel Palmer <daniel@0x0f.com> wrote: > > Hi Linus, > > On Wed, 22 Sept 2021 at 03:23, Linus Walleij <linus.walleij@linaro.org> wrote: > > I suppose it could be solved with a patch that take this route only if > > we're not using device tree or ACPI? > > Is that something I could do with a small patch in my series or is > there the potential to totally break everyone else's stuff to make > mine work? Can you give the hack that sits in my reply to Linus a go? Thanks, M.
Hi Marc,
On Thu, 30 Sept 2021 at 22:07, Marc Zyngier <maz@kernel.org> wrote:
> Can you give the hack that sits in my reply to Linus a go?
Yep. Doing it now.
Thanks,
Daniel
Hi Marc, On Thu, 30 Sept 2021 at 22:06, Marc Zyngier <maz@kernel.org> wrote: > No, this is the right spot if you really need to set the handler. But > it should really be after the parent allocation (see below for > something totally untested). Your change resolves the null pointer difference when enabling the interrupt but when it triggers the below happens. This might just be my driver so I'll try to debug. Thanks, Daniel # gpiomon -r 0 44 [ 61.770519] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0 [ 61.770646] [ 61.770659] ============================= [ 61.770670] [ BUG: Invalid wait context ] [ 61.770683] 5.15.0-rc2+ #2583 Not tainted [ 61.770702] ----------------------------- [ 61.770712] swapper/0/0 is trying to lock: [ 61.770729] c1789eb0 (&port_lock_key){-.-.}-{3:3}, at: serial8250_console_write+0x1b0/0x254 [ 61.770840] other info that might help us debug this: [ 61.770853] context-{2:2} [ 61.770868] 2 locks held by swapper/0/0: [ 61.770889] #0: c10189ec (console_lock){+.+.}-{0:0}, at: vprintk_emit+0xa4/0x200 [ 61.770986] #1: c1018b44 (console_owner){-...}-{0:0}, at: console_unlock+0x1e8/0x4b4 [ 61.771080] stack backtrace: [ 61.771093] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.15.0-rc2+ #2583 [ 61.771130] Hardware name: MStar/Sigmastar Armv7 (Device Tree) [ 61.771156] [<c010daf0>] (unwind_backtrace) from [<c0109f54>] (show_stack+0x10/0x14) [ 61.771235] [<c0109f54>] (show_stack) from [<c09303a0>] (dump_stack_lvl+0x58/0x70) [ 61.771312] [<c09303a0>] (dump_stack_lvl) from [<c016fbd8>] (__lock_acquire+0x384/0x16a0) [ 61.771394] [<c016fbd8>] (__lock_acquire) from [<c017191c>] (lock_acquire+0x2a0/0x320) [ 61.771470] [<c017191c>] (lock_acquire) from [<c093de84>] (_raw_spin_lock_irqsave+0x5c/0x70) [ 61.771542] [<c093de84>] (_raw_spin_lock_irqsave) from [<c04cdf98>] (serial8250_console_write+0x1b0/0x254) [ 61.771620] [<c04cdf98>] (serial8250_console_write) from [<c0178068>] (console_unlock+0x3fc/0x4b4) [ 61.771699] [<c0178068>] (console_unlock) from [<c0179750>] (vprintk_emit+0x1d0/0x200) [ 61.771769] [<c0179750>] (vprintk_emit) from [<c017979c>] (vprintk_default+0x1c/0x24) [ 61.771840] [<c017979c>] (vprintk_default) from [<c092d178>] (_printk+0x18/0x28) [ 61.771914] [<c092d178>] (_printk) from [<c017b9d0>] (handle_bad_irq+0x44/0x22c) [ 61.771991] [<c017b9d0>] (handle_bad_irq) from [<c017ade4>] (handle_irq_desc+0x24/0x34) [ 61.772066] [<c017ade4>] (handle_irq_desc) from [<c0467274>] (ssd20xd_gpi_chainedhandler+0xb4/0xc4) [ 61.772147] [<c0467274>] (ssd20xd_gpi_chainedhandler) from [<c017ade4>] (handle_irq_desc+0x24/0x34) [ 61.772223] [<c017ade4>] (handle_irq_desc) from [<c017b544>] (handle_domain_irq+0x40/0x54) [ 61.772299] [<c017b544>] (handle_domain_irq) from [<c0465f20>] (gic_handle_irq+0x60/0x6c) [ 61.772372] [<c0465f20>] (gic_handle_irq) from [<c0100aac>] (__irq_svc+0x4c/0x64) [ 61.772435] Exception stack(0xc1001f20 to 0xc1001f68) [ 61.772466] 1f20: ???????? ???????? ???????? ???????? ???????? ???????? ???????? ???????? [ 61.772490] 1f40: ???????? ???????? ???????? ???????? ???????? ???????? ???????? ???????? [ 61.772510] 1f60: ???????? ???????? [ 61.772531] [<c0100aac>] (__irq_svc) from [<c010715c>] (arch_cpu_idle+0x1c/0x38) [ 61.772605] [<c010715c>] (arch_cpu_idle) from [<c093dc44>] (default_idle_call+0x50/0x8c) [ 61.772677] [<c093dc44>] (default_idle_call) from [<c0155984>] (do_idle+0xf0/0x25c) [ 61.772743] [<c0155984>] (do_idle) from [<c0155ea4>] (cpu_startup_entry+0x18/0x1c) [ 61.772807] [<c0155ea4>] (cpu_startup_entry) from [<c0f00e58>] (start_kernel+0x560/0x628) [ 62.055133] ->handle_irq(): (ptrval), handle_bad_irq+0x0/0x22c [ 62.061099] ->irq_data.chip(): (ptrval), msc313_gpio_irqchip+0x0/0x90 [ 62.067585] ->action(): (ptrval) [ 62.070833] ->action->handler(): (ptrval), lineevent_irq_handler+0x0/0x1c [ 62.077664] unexpected IRQ trap at vector 42 [ 62.082014] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0 [ 62.088411] ->handle_irq(): (ptrval), handle_bad_irq+0x0/0x22c [ 62.094377] ->irq_data.chip(): (ptrval), msc313_gpio_irqchip+0x0/0x90 [ 62.100862] ->action(): (ptrval) [ 62.104111] ->action->handler(): (ptrval), lineevent_irq_handler+0x0/0x1c [ 62.110941] unexpected IRQ trap at vector 42 [ 62.115312] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0 [ 62.121712] ->handle_irq(): (ptrval), handle_bad_irq+0x0/0x22c [ 62.127675] ->irq_data.chip(): (ptrval), msc313_gpio_irqchip+0x0/0x90 [ 62.134165] ->action(): (ptrval) [ 62.137416] ->action->handler(): (ptrval), lineevent_irq_handler+0x0/0x1c [ 62.144246] unexpected IRQ trap at vector 42 [ 62.148588] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0 [ 62.154988] ->handle_irq(): (ptrval), handle_bad_irq+0x0/0x22c [ 62.160955] ->irq_data.chip(): (ptrval), msc313_gpio_irqchip+0x0/0x90 [ 62.167440] ->action(): (ptrval) [ 62.170689] ->action->handler(): (ptrval), lineevent_irq_handler+0x0/0x1c [ 62.177517] unexpected IRQ trap at vector 42 [ 62.181840] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0 [ 62.188237] ->handle_irq(): (ptrval), handle_bad_irq+0x0/0x22c [ 62.194201] ->irq_data.chip(): (ptrval), msc313_gpio_irqchip+0x0/0x90 [ 62.200686] ->action(): (ptrval) [ 62.203934] ->action->handler(): (ptrval), lineevent_irq_handler+0x0/0x1c [ 62.210763] unexpected IRQ trap at vector 42 [ 62.215095] unexpected IRQ trap at vector 42 [ 62.219424] unexpected IRQ trap at vector 42 [ 62.223759] unexpected IRQ trap at vector 42 [ 62.228084] unexpected IRQ trap at vector 42 [ 62.232407] unexpected IRQ trap at vector 42 [ 62.236729] unexpected IRQ trap at vector 42 [ 62.241052] unexpected IRQ trap at vector 42 [ 62.245377] unexpected IRQ trap at vector 42 [ 62.249703] unexpected IRQ trap at vector 42 [ 62.254037] unexpected IRQ trap at vector 42
On Thu, 30 Sep 2021 14:36:52 +0100, Daniel Palmer <daniel@0x0f.com> wrote: > > Hi Marc, > > On Thu, 30 Sept 2021 at 22:06, Marc Zyngier <maz@kernel.org> wrote: > > No, this is the right spot if you really need to set the handler. But > > it should really be after the parent allocation (see below for > > something totally untested). > > Your change resolves the null pointer difference when enabling the > interrupt but when it triggers the below happens. > This might just be my driver so I'll try to debug. > > Thanks, > > Daniel > > # gpiomon -r 0 44 > [ 61.770519] irq 66, desc: (ptrval), depth: 0, count: 0, unhandled: 0 > [ 61.770646] > [ 61.770659] ============================= > [ 61.770670] [ BUG: Invalid wait context ] > [ 61.770683] 5.15.0-rc2+ #2583 Not tainted > [ 61.770702] ----------------------------- > [ 61.770712] swapper/0/0 is trying to lock: > [ 61.770729] c1789eb0 (&port_lock_key){-.-.}-{3:3}, at: > serial8250_console_write+0x1b0/0x254 > [ 61.770840] other info that might help us debug this: > [ 61.770853] context-{2:2} > [ 61.770868] 2 locks held by swapper/0/0: > [ 61.770889] #0: c10189ec (console_lock){+.+.}-{0:0}, at: > vprintk_emit+0xa4/0x200 > [ 61.770986] #1: c1018b44 (console_owner){-...}-{0:0}, at: > console_unlock+0x1e8/0x4b4 > [ 61.771080] stack backtrace: > [ 61.771093] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.15.0-rc2+ #2583 > [ 61.771130] Hardware name: MStar/Sigmastar Armv7 (Device Tree) > [ 61.771156] [<c010daf0>] (unwind_backtrace) from [<c0109f54>] > (show_stack+0x10/0x14) > [ 61.771235] [<c0109f54>] (show_stack) from [<c09303a0>] > (dump_stack_lvl+0x58/0x70) > [ 61.771312] [<c09303a0>] (dump_stack_lvl) from [<c016fbd8>] > (__lock_acquire+0x384/0x16a0) > [ 61.771394] [<c016fbd8>] (__lock_acquire) from [<c017191c>] > (lock_acquire+0x2a0/0x320) > [ 61.771470] [<c017191c>] (lock_acquire) from [<c093de84>] > (_raw_spin_lock_irqsave+0x5c/0x70) > [ 61.771542] [<c093de84>] (_raw_spin_lock_irqsave) from [<c04cdf98>] > (serial8250_console_write+0x1b0/0x254) > [ 61.771620] [<c04cdf98>] (serial8250_console_write) from > [<c0178068>] (console_unlock+0x3fc/0x4b4) > [ 61.771699] [<c0178068>] (console_unlock) from [<c0179750>] > (vprintk_emit+0x1d0/0x200) > [ 61.771769] [<c0179750>] (vprintk_emit) from [<c017979c>] > (vprintk_default+0x1c/0x24) > [ 61.771840] [<c017979c>] (vprintk_default) from [<c092d178>] > (_printk+0x18/0x28) > [ 61.771914] [<c092d178>] (_printk) from [<c017b9d0>] > (handle_bad_irq+0x44/0x22c) > [ 61.771991] [<c017b9d0>] (handle_bad_irq) from [<c017ade4>] > (handle_irq_desc+0x24/0x34) > [ 61.772066] [<c017ade4>] (handle_irq_desc) from [<c0467274>] > (ssd20xd_gpi_chainedhandler+0xb4/0xc4) > [ 61.772147] [<c0467274>] (ssd20xd_gpi_chainedhandler) from > [<c017ade4>] (handle_irq_desc+0x24/0x34) > [ 61.772223] [<c017ade4>] (handle_irq_desc) from [<c017b544>] > (handle_domain_irq+0x40/0x54) > [ 61.772299] [<c017b544>] (handle_domain_irq) from [<c0465f20>] > (gic_handle_irq+0x60/0x6c) > [ 61.772372] [<c0465f20>] (gic_handle_irq) from [<c0100aac>] > (__irq_svc+0x4c/0x64) Somehow, the handler for this interrupt is set to handle_bad_irq(), which probably isn't what you want. You'll have to find out who sets this (there is a comment about that in gpiolib.c, but I haven't had a chance to find where this is coming from). Do you happen to set it in your driver? M.
Hi Marc, On Thu, 30 Sept 2021 at 22:53, Marc Zyngier <maz@kernel.org> wrote: > Somehow, the handler for this interrupt is set to handle_bad_irq(), > which probably isn't what you want. You'll have to find out who sets > this (there is a comment about that in gpiolib.c, but I haven't had a > chance to find where this is coming from). > > Do you happen to set it in your driver? The gpio driver (gpio-msc313.c) sets it during probe: gpioirqchip = &gpiochip->irq; gpioirqchip->chip = &msc313_gpio_irqchip; gpioirqchip->fwnode = of_node_to_fwnode(dev->of_node); gpioirqchip->parent_domain = parent_domain; gpioirqchip->child_to_parent_hwirq = match_data->child_to_parent_hwirq; gpioirqchip->populate_parent_alloc_arg = match_data->populate_parent_fwspec; gpioirqchip->handler = handle_bad_irq; gpioirqchip->default_type = IRQ_TYPE_NONE; Cheers, Daniel
On Thu, 30 Sep 2021 14:59:24 +0100, Daniel Palmer <daniel@0x0f.com> wrote: > > Hi Marc, > > On Thu, 30 Sept 2021 at 22:53, Marc Zyngier <maz@kernel.org> wrote: > > Somehow, the handler for this interrupt is set to handle_bad_irq(), > > which probably isn't what you want. You'll have to find out who sets > > this (there is a comment about that in gpiolib.c, but I haven't had a > > chance to find where this is coming from). > > > > Do you happen to set it in your driver? > > The gpio driver (gpio-msc313.c) sets it during probe: > > gpioirqchip = &gpiochip->irq; > gpioirqchip->chip = &msc313_gpio_irqchip; > gpioirqchip->fwnode = of_node_to_fwnode(dev->of_node); > gpioirqchip->parent_domain = parent_domain; > gpioirqchip->child_to_parent_hwirq = match_data->child_to_parent_hwirq; > gpioirqchip->populate_parent_alloc_arg = match_data->populate_parent_fwspec; > gpioirqchip->handler = handle_bad_irq; > gpioirqchip->default_type = IRQ_TYPE_NONE; Right. I have no idea why this is a requirement, and I would normally set it to whatever is the normal flow handler on this HW, but this looks like the GPIO subsystem has some expectations here. I'll let Linus comment on it. M.
On Thu, Sep 30, 2021 at 4:11 PM Marc Zyngier <maz@kernel.org> wrote: > On Thu, 30 Sep 2021 14:59:24 +0100, > Daniel Palmer <daniel@0x0f.com> wrote: > > gpioirqchip->handler = handle_bad_irq; > > gpioirqchip->default_type = IRQ_TYPE_NONE; > > Right. I have no idea why this is a requirement, and I would normally > set it to whatever is the normal flow handler on this HW, but this > looks like the GPIO subsystem has some expectations here. > > I'll let Linus comment on it. The handle_bad_irq() as default handler is because many GPIO IRQ controllers in difference from on-chip IRQ controllers support two levels and two edges of triggers, and there is nothing "normal" (it is "general purpose" after all) so we need to defer the selection of a suitable flow handler to .set_type(). With device tree the trigger is often specified in the second cell in the DTS, so .set_type() will be called for any consumer as part of the request_irq() and the appropriate handler is set from .set_type(). In my mind this is following the DT (ACPI) usage model of allocating and initializing resources dynamically as they are requested. I don't know if this reasoning is wrong in some way, what should we do otherwise? (Confused...) Yours, Linus Walleij
On Thu, Sep 30, 2021 at 3:37 PM Daniel Palmer <daniel@0x0f.com> wrote: > On Thu, 30 Sept 2021 at 22:06, Marc Zyngier <maz@kernel.org> wrote: > > No, this is the right spot if you really need to set the handler. But > > it should really be after the parent allocation (see below for > > something totally untested). > > Your change resolves the null pointer difference when enabling the > interrupt but when it triggers the below happens. > This might just be my driver so I'll try to debug. To me this looks like your IRQ handler is firing for unused IRQs, i.e. you are getting spurious IRQs. Are you missing to disable all IRQs as part of the set-up before registering the GPIO chip? (Usually some registers need to be written with zeroes.) Yours, Linus Walleij
Hi Linus, On Fri, 1 Oct 2021 at 01:13, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Thu, Sep 30, 2021 at 3:37 PM Daniel Palmer <daniel@0x0f.com> wrote: > To me this looks like your IRQ handler is firing for unused IRQs, i.e. > you are getting spurious IRQs. > > Are you missing to disable all IRQs as part of the set-up before > registering the GPIO chip? (Usually some registers need to > be written with zeroes.) I don't think it's something like the wrong IRQ firing as the same driver was previously working with EOI to clear the interrupt instead of ACK. I'll double check though. Cheers, Daniel
Hi Linus, Sorry for the constant spam on this.. On Fri, 1 Oct 2021 at 01:13, Linus Walleij <linus.walleij@linaro.org> wrote: > To me this looks like your IRQ handler is firing for unused IRQs, i.e. > you are getting spurious IRQs. > > Are you missing to disable all IRQs as part of the set-up before > registering the GPIO chip? (Usually some registers need to > be written with zeroes.) Changing the handler to handle_edge_irq() on the gpio side resolves the issue and gpiomon registers edges from the gpio like it should. So I think it's an ordering thing. Something like the gpio side sets the handler to handle_bad_irq() after the irqchip side sets it to handle_edge_irq(). Thanks for the help. Cheers, Daniel
diff --git a/MAINTAINERS b/MAINTAINERS index 3004c0f735b6..487d5e62c287 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2248,6 +2248,7 @@ F: arch/arm/boot/dts/mstar-* F: arch/arm/mach-mstar/ F: drivers/clk/mstar/ F: drivers/gpio/gpio-msc313.c +F: drivers/irqchip/irq-ssd20xd-gpi.c F: drivers/watchdog/msc313e_wdt.c F: include/dt-bindings/clock/mstar-* F: include/dt-bindings/gpio/msc313-gpio.h diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index 4d5924e9f766..8786aed7f776 100644 --- a/drivers/irqchip/Kconfig +++ b/drivers/irqchip/Kconfig @@ -582,6 +582,17 @@ config MST_IRQ help Support MStar Interrupt Controller. +config SSD20XD_GPI + bool "SigmaStar SSD20xD GPIO Interrupt Controller" + depends on ARCH_MSTARV7 || COMPILE_TEST + default ARCH_MSTARV7 + select IRQ_DOMAIN + select IRQ_DOMAIN_HIERARCHY + select REGMAP + help + Support for the SigmaStar GPIO interrupt controller + found in SSD201, SSD202D and others. + config WPCM450_AIC bool "Nuvoton WPCM450 Advanced Interrupt Controller" depends on ARCH_WPCM450 diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index f88cbf36a9d2..1a6c3dbd67a8 100644 --- a/drivers/irqchip/Makefile +++ b/drivers/irqchip/Makefile @@ -116,3 +116,5 @@ obj-$(CONFIG_MACH_REALTEK_RTL) += irq-realtek-rtl.o obj-$(CONFIG_WPCM450_AIC) += irq-wpcm450-aic.o obj-$(CONFIG_IRQ_IDT3243X) += irq-idt3243x.o obj-$(CONFIG_APPLE_AIC) += irq-apple-aic.o +obj-$(CONFIG_SSD20XD_GPI) += irq-ssd20xd-gpi.o + diff --git a/drivers/irqchip/irq-ssd20xd-gpi.c b/drivers/irqchip/irq-ssd20xd-gpi.c new file mode 100644 index 000000000000..c34f41380717 --- /dev/null +++ b/drivers/irqchip/irq-ssd20xd-gpi.c @@ -0,0 +1,211 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2021 Daniel Palmer <daniel@thingy.jp> + */ + +#include <linux/irq.h> +#include <linux/irqchip.h> +#include <linux/irqdomain.h> +#include <linux/irqchip/chained_irq.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> +#include <linux/interrupt.h> + +#define NUM_IRQ 76 +#define IRQS_PER_REG 16 +#define STRIDE 4 + +#define REG_MASK 0x0 +#define REG_ACK 0x28 +#define REG_TYPE 0x40 +#define REG_STATUS 0xc0 + +struct ssd20xd_gpi { + struct regmap *regmap; + struct irq_domain *domain; +}; + +#define REG_OFFSET(_hwirq) ((hwirq >> 4) * STRIDE) +#define BIT_OFFSET(_hwirq) (1 << (hwirq & 0xf)) + +static void ssd20xd_gpi_mask_irq(struct irq_data *data) +{ + irq_hw_number_t hwirq = irqd_to_hwirq(data); + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); + int offset_reg = REG_OFFSET(hwirq); + int offset_bit = BIT_OFFSET(hwirq); + + regmap_update_bits(gpi->regmap, REG_MASK + offset_reg, offset_bit, offset_bit); +} + +static void ssd20xd_gpi_unmask_irq(struct irq_data *data) +{ + irq_hw_number_t hwirq = irqd_to_hwirq(data); + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); + int offset_reg = REG_OFFSET(hwirq); + int offset_bit = BIT_OFFSET(hwirq); + + regmap_update_bits(gpi->regmap, REG_MASK + offset_reg, offset_bit, 0); +} + +static void ssd20xd_gpi_irq_eoi(struct irq_data *data) +{ + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); + irq_hw_number_t hwirq = irqd_to_hwirq(data); + int offset_reg = REG_OFFSET(hwirq); + int offset_bit = BIT_OFFSET(hwirq); + + regmap_update_bits_base(gpi->regmap, REG_ACK + offset_reg, + offset_bit, offset_bit, NULL, false, true); +} + +static int ssd20xd_gpi_set_type_irq(struct irq_data *data, unsigned int flow_type) +{ + irq_hw_number_t hwirq = irqd_to_hwirq(data); + struct ssd20xd_gpi *gpi = irq_data_get_irq_chip_data(data); + int offset_reg = REG_OFFSET(hwirq); + int offset_bit = BIT_OFFSET(hwirq); + + switch (flow_type) { + case IRQ_TYPE_EDGE_FALLING: + regmap_update_bits(gpi->regmap, REG_TYPE + offset_reg, offset_bit, offset_bit); + break; + case IRQ_TYPE_EDGE_RISING: + regmap_update_bits(gpi->regmap, REG_TYPE + offset_reg, offset_bit, 0); + break; + default: + return -EINVAL; + } + + return 0; +} + +static struct irq_chip ssd20xd_gpi_chip = { + .name = "GPI", + .irq_mask = ssd20xd_gpi_mask_irq, + .irq_unmask = ssd20xd_gpi_unmask_irq, + .irq_eoi = ssd20xd_gpi_irq_eoi, + .irq_set_type = ssd20xd_gpi_set_type_irq, +}; + +static int ssd20xd_gpi_domain_alloc(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs, void *arg) +{ + struct ssd20xd_gpi *intc = domain->host_data; + struct irq_fwspec *fwspec = arg; + int i; + + for (i = 0; i < nr_irqs; i++) + irq_domain_set_info(domain, virq + i, fwspec->param[0] + i, + &ssd20xd_gpi_chip, intc, handle_fasteoi_irq, NULL, NULL); + + return 0; +} + +static void ssd20xd_gpi_domain_free(struct irq_domain *domain, unsigned int virq, + unsigned int nr_irqs) +{ + int i; + + for (i = 0; i < nr_irqs; i++) { + struct irq_data *d = irq_domain_get_irq_data(domain, virq + i); + + irq_set_handler(virq + i, NULL); + irq_domain_reset_irq_data(d); + } +} + +static const struct irq_domain_ops ssd20xd_gpi_domain_ops = { + .alloc = ssd20xd_gpi_domain_alloc, + .free = ssd20xd_gpi_domain_free, +}; + +static const struct regmap_config ssd20xd_gpi_regmap_config = { + .reg_bits = 16, + .val_bits = 16, + .reg_stride = 4, +}; + +static void ssd20x_gpi_chainedhandler(struct irq_desc *desc) +{ + struct ssd20xd_gpi *intc = irq_desc_get_handler_data(desc); + struct irq_chip *chip = irq_desc_get_chip(desc); + unsigned int irqbit, hwirq, virq, status; + int i; + + chained_irq_enter(chip, desc); + + for (i = 0; i < NUM_IRQ / IRQS_PER_REG; i++) { + int offset_reg = STRIDE * i; + int offset_irq = IRQS_PER_REG * i; + + regmap_read(intc->regmap, REG_STATUS + offset_reg, &status); + + while (status) { + irqbit = __ffs(status); + hwirq = offset_irq + irqbit; + virq = irq_find_mapping(intc->domain, hwirq); + if (virq) + generic_handle_irq(virq); + status &= ~BIT(irqbit); + } + } + + chained_irq_exit(chip, desc); +} + +static int __init ssd20xd_gpi_of_init(struct device_node *node, + struct device_node *parent) +{ + struct ssd20xd_gpi *intc; + void __iomem *base; + int irq, ret; + + intc = kzalloc(sizeof(*intc), GFP_KERNEL); + if (!intc) + return -ENOMEM; + + base = of_iomap(node, 0); + if (!base) { + ret = -ENODEV; + goto out_free; + } + + intc->regmap = regmap_init_mmio(NULL, base, &ssd20xd_gpi_regmap_config); + if (IS_ERR(intc->regmap)) { + ret = PTR_ERR(intc->regmap); + goto out_unmap; + } + + intc->domain = irq_domain_add_linear(node, NUM_IRQ, &ssd20xd_gpi_domain_ops, intc); + if (!intc->domain) { + ret = -ENOMEM; + goto out_free_regmap; + } + + irq = of_irq_get(node, 0); + if (irq <= 0) { + ret = irq; + goto out_free_domain; + } + + irq_set_chained_handler_and_data(irq, ssd20x_gpi_chainedhandler, + intc); + + return 0; + +out_free_domain: + irq_domain_remove(intc->domain); +out_free_regmap: + regmap_exit(intc->regmap); +out_unmap: + iounmap(base); +out_free: + kfree(intc); + return ret; +} + +IRQCHIP_DECLARE(ssd20xd_gpi, "sstar,ssd20xd-gpi", ssd20xd_gpi_of_init);
Add support for the SigmaStar GPIO interrupt controller, gpi, that is present in SSD201 and SSD202D SoCs. This routes interrupts from many interrupts into a single interrupt that is connected to the peripheral interrupt controller. Signed-off-by: Daniel Palmer <daniel@0x0f.com> --- MAINTAINERS | 1 + drivers/irqchip/Kconfig | 11 ++ drivers/irqchip/Makefile | 2 + drivers/irqchip/irq-ssd20xd-gpi.c | 211 ++++++++++++++++++++++++++++++ 4 files changed, 225 insertions(+) create mode 100644 drivers/irqchip/irq-ssd20xd-gpi.c