* [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller @ 2021-09-14 10:04 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-14 10:04 UTC (permalink / raw) To: devicetree, robh+dt, maz, tglx Cc: linux-arm-kernel, romain.perier, Daniel Palmer In new SigmaStar SoCs they have moved away from having a few interrupt capable GPIOs and instead have chained yet another interrupt controller in to provide interrupt support for all of the GPIOs. I'm hardly an IRQ expert so I expect I've made a total mess of this. No one else was going to write this driver so I had a go. Daniel Palmer (3): dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi irqchip: SigmaStar SSD20xD gpi ARM: dts: mstar: Add gpi interrupt controller to i2m .../sstar,ssd20xd-gpi.yaml | 53 +++++ MAINTAINERS | 2 + arch/arm/boot/dts/mstar-infinity2m.dtsi | 8 + drivers/irqchip/Kconfig | 11 + drivers/irqchip/Makefile | 2 + drivers/irqchip/irq-ssd20xd-gpi.c | 211 ++++++++++++++++++ 6 files changed, 287 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml create mode 100644 drivers/irqchip/irq-ssd20xd-gpi.c -- 2.33.0 ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller @ 2021-09-14 10:04 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-14 10:04 UTC (permalink / raw) To: devicetree, robh+dt, maz, tglx Cc: linux-arm-kernel, romain.perier, Daniel Palmer In new SigmaStar SoCs they have moved away from having a few interrupt capable GPIOs and instead have chained yet another interrupt controller in to provide interrupt support for all of the GPIOs. I'm hardly an IRQ expert so I expect I've made a total mess of this. No one else was going to write this driver so I had a go. Daniel Palmer (3): dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi irqchip: SigmaStar SSD20xD gpi ARM: dts: mstar: Add gpi interrupt controller to i2m .../sstar,ssd20xd-gpi.yaml | 53 +++++ MAINTAINERS | 2 + arch/arm/boot/dts/mstar-infinity2m.dtsi | 8 + drivers/irqchip/Kconfig | 11 + drivers/irqchip/Makefile | 2 + drivers/irqchip/irq-ssd20xd-gpi.c | 211 ++++++++++++++++++ 6 files changed, 287 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml create mode 100644 drivers/irqchip/irq-ssd20xd-gpi.c -- 2.33.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 1/3] dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi 2021-09-14 10:04 ` Daniel Palmer @ 2021-09-14 10:04 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-14 10:04 UTC (permalink / raw) To: devicetree, robh+dt, maz, tglx Cc: linux-arm-kernel, romain.perier, Daniel Palmer Add a binding description for the SigmaStar GPIO interrupt controller, gpi, found on the SSD201 and SSD202D. Signed-off-by: Daniel Palmer <daniel@0x0f.com> --- .../sstar,ssd20xd-gpi.yaml | 53 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml diff --git a/Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml b/Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml new file mode 100644 index 000000000000..1f7e6c5fef52 --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/interrupt-controller/sstar,ssd20xd-gpi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SigmaStar GPIO Interrupt Controller + +maintainers: + - Daniel Palmer <daniel@thingy.jp> + +description: |+ + Newer SigmaStar SoCs from the SSD201/SSD202D have an extra + interrupt controller that is just for handling interrupts + on GPIOs and then routing a single interrupt to the peripheral + interrupt controller instead of only having a few interrupt + capable GPIOs that are routed directly to the peripheral + interrupt controller like older SoCs. + +properties: + compatible: + const: sstar,ssd20xd-gpi + + interrupt-controller: true + + "#interrupt-cells": + const: 2 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + interrupt-controller@207a00 { + compatible = "sstar,ssd20xd-gpi"; + reg = <0x207a00 0x200>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts-extended = <&intc_irq GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; + }; +... diff --git a/MAINTAINERS b/MAINTAINERS index eeb4c70b3d5b..3004c0f735b6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2243,6 +2243,7 @@ T: git git://github.com/linux-chenxing/linux.git F: Documentation/devicetree/bindings/arm/mstar/* F: Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml F: Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml +F: Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml F: arch/arm/boot/dts/mstar-* F: arch/arm/mach-mstar/ F: drivers/clk/mstar/ -- 2.33.0 ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 1/3] dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi @ 2021-09-14 10:04 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-14 10:04 UTC (permalink / raw) To: devicetree, robh+dt, maz, tglx Cc: linux-arm-kernel, romain.perier, Daniel Palmer Add a binding description for the SigmaStar GPIO interrupt controller, gpi, found on the SSD201 and SSD202D. Signed-off-by: Daniel Palmer <daniel@0x0f.com> --- .../sstar,ssd20xd-gpi.yaml | 53 +++++++++++++++++++ MAINTAINERS | 1 + 2 files changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml diff --git a/Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml b/Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml new file mode 100644 index 000000000000..1f7e6c5fef52 --- /dev/null +++ b/Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/interrupt-controller/sstar,ssd20xd-gpi.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: SigmaStar GPIO Interrupt Controller + +maintainers: + - Daniel Palmer <daniel@thingy.jp> + +description: |+ + Newer SigmaStar SoCs from the SSD201/SSD202D have an extra + interrupt controller that is just for handling interrupts + on GPIOs and then routing a single interrupt to the peripheral + interrupt controller instead of only having a few interrupt + capable GPIOs that are routed directly to the peripheral + interrupt controller like older SoCs. + +properties: + compatible: + const: sstar,ssd20xd-gpi + + interrupt-controller: true + + "#interrupt-cells": + const: 2 + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + interrupt-controller@207a00 { + compatible = "sstar,ssd20xd-gpi"; + reg = <0x207a00 0x200>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts-extended = <&intc_irq GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; + }; +... diff --git a/MAINTAINERS b/MAINTAINERS index eeb4c70b3d5b..3004c0f735b6 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2243,6 +2243,7 @@ T: git git://github.com/linux-chenxing/linux.git F: Documentation/devicetree/bindings/arm/mstar/* F: Documentation/devicetree/bindings/clock/mstar,msc313-mpll.yaml F: Documentation/devicetree/bindings/gpio/mstar,msc313-gpio.yaml +F: Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml F: arch/arm/boot/dts/mstar-* F: arch/arm/mach-mstar/ F: drivers/clk/mstar/ -- 2.33.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH 1/3] dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi 2021-09-14 10:04 ` Daniel Palmer @ 2021-09-21 20:36 ` Rob Herring -1 siblings, 0 replies; 56+ messages in thread From: Rob Herring @ 2021-09-21 20:36 UTC (permalink / raw) To: Daniel Palmer Cc: maz, linux-arm-kernel, tglx, romain.perier, robh+dt, devicetree On Tue, 14 Sep 2021 19:04:13 +0900, Daniel Palmer wrote: > Add a binding description for the SigmaStar GPIO interrupt > controller, gpi, found on the SSD201 and SSD202D. > > Signed-off-by: Daniel Palmer <daniel@0x0f.com> > --- > .../sstar,ssd20xd-gpi.yaml | 53 +++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 54 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml > Reviewed-by: Rob Herring <robh@kernel.org> ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 1/3] dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi @ 2021-09-21 20:36 ` Rob Herring 0 siblings, 0 replies; 56+ messages in thread From: Rob Herring @ 2021-09-21 20:36 UTC (permalink / raw) To: Daniel Palmer Cc: maz, linux-arm-kernel, tglx, romain.perier, robh+dt, devicetree On Tue, 14 Sep 2021 19:04:13 +0900, Daniel Palmer wrote: > Add a binding description for the SigmaStar GPIO interrupt > controller, gpi, found on the SSD201 and SSD202D. > > Signed-off-by: Daniel Palmer <daniel@0x0f.com> > --- > .../sstar,ssd20xd-gpi.yaml | 53 +++++++++++++++++++ > MAINTAINERS | 1 + > 2 files changed, 54 insertions(+) > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/sstar,ssd20xd-gpi.yaml > Reviewed-by: Rob Herring <robh@kernel.org> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-14 10:04 ` Daniel Palmer @ 2021-09-14 10:04 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-14 10:04 UTC (permalink / raw) To: devicetree, robh+dt, maz, tglx Cc: linux-arm-kernel, romain.perier, Daniel Palmer 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); +} + +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); -- 2.33.0 ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-14 10:04 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-14 10:04 UTC (permalink / raw) To: devicetree, robh+dt, maz, tglx Cc: linux-arm-kernel, romain.perier, Daniel Palmer 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); +} + +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); -- 2.33.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-14 10:04 ` Daniel Palmer @ 2021-09-20 9:45 ` Marc Zyngier -1 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-20 9:45 UTC (permalink / raw) To: Daniel Palmer; +Cc: devicetree, robh+dt, tglx, linux-arm-kernel, romain.perier 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. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-20 9:45 ` Marc Zyngier 0 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-20 9:45 UTC (permalink / raw) To: Daniel Palmer; +Cc: devicetree, robh+dt, tglx, linux-arm-kernel, romain.perier 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. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-20 9:45 ` Marc Zyngier @ 2021-09-20 10:05 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-20 10:05 UTC (permalink / raw) To: Marc Zyngier Cc: DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-20 10:05 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-20 10:05 UTC (permalink / raw) To: Marc Zyngier Cc: DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-20 10:05 ` Daniel Palmer @ 2021-09-20 11:24 ` Marc Zyngier -1 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-20 11:24 UTC (permalink / raw) To: Daniel Palmer Cc: DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-20 11:24 ` Marc Zyngier 0 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-20 11:24 UTC (permalink / raw) To: Daniel Palmer Cc: DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-20 11:24 ` Marc Zyngier @ 2021-09-21 4:16 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-21 4:16 UTC (permalink / raw) To: Marc Zyngier Cc: DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-21 4:16 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-21 4:16 UTC (permalink / raw) To: Marc Zyngier Cc: DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-21 4:16 ` Daniel Palmer @ 2021-09-21 8:27 ` Marc Zyngier -1 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-21 8:27 UTC (permalink / raw) To: Daniel Palmer, Linus Walleij Cc: DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-21 8:27 ` Marc Zyngier 0 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-21 8:27 UTC (permalink / raw) To: Daniel Palmer, Linus Walleij Cc: DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-21 8:27 ` Marc Zyngier @ 2021-09-21 18:23 ` Linus Walleij -1 siblings, 0 replies; 56+ messages in thread From: Linus Walleij @ 2021-09-21 18:23 UTC (permalink / raw) To: Marc Zyngier Cc: Daniel Palmer, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-21 18:23 ` Linus Walleij 0 siblings, 0 replies; 56+ messages in thread From: Linus Walleij @ 2021-09-21 18:23 UTC (permalink / raw) To: Marc Zyngier Cc: Daniel Palmer, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-21 18:23 ` Linus Walleij @ 2021-09-30 12:39 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-30 12:39 UTC (permalink / raw) To: Linus Walleij Cc: Marc Zyngier, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-30 12:39 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-30 12:39 UTC (permalink / raw) To: Linus Walleij Cc: Marc Zyngier, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-30 12:39 ` Daniel Palmer @ 2021-09-30 13:07 ` Marc Zyngier -1 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-30 13:07 UTC (permalink / raw) To: Daniel Palmer Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-30 13:07 ` Marc Zyngier 0 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-30 13:07 UTC (permalink / raw) To: Daniel Palmer Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-30 13:07 ` Marc Zyngier @ 2021-09-30 13:10 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-30 13:10 UTC (permalink / raw) To: Marc Zyngier Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-30 13:10 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-30 13:10 UTC (permalink / raw) To: Marc Zyngier Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-21 18:23 ` Linus Walleij @ 2021-09-30 13:06 ` Marc Zyngier -1 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-30 13:06 UTC (permalink / raw) To: Linus Walleij Cc: Daniel Palmer, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier [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; } -- Without deviation from the norm, progress is not possible. ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-30 13:06 ` Marc Zyngier 0 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-30 13:06 UTC (permalink / raw) To: Linus Walleij Cc: Daniel Palmer, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier [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; } -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-30 13:06 ` Marc Zyngier @ 2021-09-30 13:36 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-30 13:36 UTC (permalink / raw) To: Marc Zyngier Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-30 13:36 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-30 13:36 UTC (permalink / raw) To: Marc Zyngier Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-30 13:36 ` Daniel Palmer @ 2021-09-30 13:53 ` Marc Zyngier -1 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-30 13:53 UTC (permalink / raw) To: Daniel Palmer Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-30 13:53 ` Marc Zyngier 0 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-30 13:53 UTC (permalink / raw) To: Daniel Palmer Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-30 13:53 ` Marc Zyngier @ 2021-09-30 13:59 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-30 13:59 UTC (permalink / raw) To: Marc Zyngier Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-30 13:59 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-30 13:59 UTC (permalink / raw) To: Marc Zyngier Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-30 13:59 ` Daniel Palmer @ 2021-09-30 14:11 ` Marc Zyngier -1 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-30 14:11 UTC (permalink / raw) To: Daniel Palmer Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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. -- Without deviation from the norm, progress is not possible. ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-30 14:11 ` Marc Zyngier 0 siblings, 0 replies; 56+ messages in thread From: Marc Zyngier @ 2021-09-30 14:11 UTC (permalink / raw) To: Daniel Palmer Cc: Linus Walleij, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-30 14:11 ` Marc Zyngier @ 2021-09-30 16:10 ` Linus Walleij -1 siblings, 0 replies; 56+ messages in thread From: Linus Walleij @ 2021-09-30 16:10 UTC (permalink / raw) To: Marc Zyngier Cc: Daniel Palmer, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-30 16:10 ` Linus Walleij 0 siblings, 0 replies; 56+ messages in thread From: Linus Walleij @ 2021-09-30 16:10 UTC (permalink / raw) To: Marc Zyngier Cc: Daniel Palmer, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-30 13:36 ` Daniel Palmer @ 2021-09-30 16:13 ` Linus Walleij -1 siblings, 0 replies; 56+ messages in thread From: Linus Walleij @ 2021-09-30 16:13 UTC (permalink / raw) To: Daniel Palmer Cc: Marc Zyngier, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-30 16:13 ` Linus Walleij 0 siblings, 0 replies; 56+ messages in thread From: Linus Walleij @ 2021-09-30 16:13 UTC (permalink / raw) To: Daniel Palmer Cc: Marc Zyngier, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-30 16:13 ` Linus Walleij @ 2021-10-01 12:33 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-10-01 12:33 UTC (permalink / raw) To: Linus Walleij Cc: Marc Zyngier, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-10-01 12:33 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-10-01 12:33 UTC (permalink / raw) To: Linus Walleij Cc: Marc Zyngier, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-30 16:13 ` Linus Walleij @ 2021-10-02 3:08 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-10-02 3:08 UTC (permalink / raw) To: Linus Walleij Cc: Marc Zyngier, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-10-02 3:08 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-10-02 3:08 UTC (permalink / raw) To: Linus Walleij Cc: Marc Zyngier, DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi 2021-09-20 11:24 ` Marc Zyngier @ 2021-09-21 6:11 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-21 6:11 UTC (permalink / raw) To: Marc Zyngier Cc: DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi @ 2021-09-21 6:11 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-21 6:11 UTC (permalink / raw) To: Marc Zyngier Cc: DTML, Rob Herring, Thomas Gleixner, linux-arm-kernel, Romain Perier 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* [PATCH 3/3] ARM: dts: mstar: Add gpi interrupt controller to i2m 2021-09-14 10:04 ` Daniel Palmer @ 2021-09-14 10:04 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-14 10:04 UTC (permalink / raw) To: devicetree, robh+dt, maz, tglx Cc: linux-arm-kernel, romain.perier, Daniel Palmer infinity2m chips have the GPI interrupt controller for GPIOs so add it to the dtsi for infinity2m. Signed-off-by: Daniel Palmer <daniel@0x0f.com> --- arch/arm/boot/dts/mstar-infinity2m.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/arch/arm/boot/dts/mstar-infinity2m.dtsi b/arch/arm/boot/dts/mstar-infinity2m.dtsi index 6d4d1d224e96..0ec8e4cadf5c 100644 --- a/arch/arm/boot/dts/mstar-infinity2m.dtsi +++ b/arch/arm/boot/dts/mstar-infinity2m.dtsi @@ -19,4 +19,12 @@ smpctrl: smpctrl@204000 { reg = <0x204000 0x200>; status = "disabled"; }; + + gpi: interrupt-controller@207a00 { + compatible = "sstar,ssd20xd-gpi"; + reg = <0x207a00 0x200>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts-extended = <&intc_irq GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; + }; }; -- 2.33.0 ^ permalink raw reply related [flat|nested] 56+ messages in thread
* [PATCH 3/3] ARM: dts: mstar: Add gpi interrupt controller to i2m @ 2021-09-14 10:04 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-14 10:04 UTC (permalink / raw) To: devicetree, robh+dt, maz, tglx Cc: linux-arm-kernel, romain.perier, Daniel Palmer infinity2m chips have the GPI interrupt controller for GPIOs so add it to the dtsi for infinity2m. Signed-off-by: Daniel Palmer <daniel@0x0f.com> --- arch/arm/boot/dts/mstar-infinity2m.dtsi | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/arch/arm/boot/dts/mstar-infinity2m.dtsi b/arch/arm/boot/dts/mstar-infinity2m.dtsi index 6d4d1d224e96..0ec8e4cadf5c 100644 --- a/arch/arm/boot/dts/mstar-infinity2m.dtsi +++ b/arch/arm/boot/dts/mstar-infinity2m.dtsi @@ -19,4 +19,12 @@ smpctrl: smpctrl@204000 { reg = <0x204000 0x200>; status = "disabled"; }; + + gpi: interrupt-controller@207a00 { + compatible = "sstar,ssd20xd-gpi"; + reg = <0x207a00 0x200>; + #interrupt-cells = <2>; + interrupt-controller; + interrupts-extended = <&intc_irq GIC_SPI 56 IRQ_TYPE_LEVEL_HIGH>; + }; }; -- 2.33.0 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 56+ messages in thread
* Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller 2021-09-14 10:04 ` Daniel Palmer @ 2021-09-14 15:59 ` Andrew Lunn -1 siblings, 0 replies; 56+ messages in thread From: Andrew Lunn @ 2021-09-14 15:59 UTC (permalink / raw) To: Daniel Palmer Cc: devicetree, robh+dt, maz, tglx, linux-arm-kernel, romain.perier On Tue, Sep 14, 2021 at 07:04:12PM +0900, Daniel Palmer wrote: > In new SigmaStar SoCs they have moved away from having a few > interrupt capable GPIOs and instead have chained yet another > interrupt controller in to provide interrupt support for > all of the GPIOs. > > I'm hardly an IRQ expert so I expect I've made a total > mess of this. No one else was going to write this driver > so I had a go. Hi Daniel How are the GPIOs mapped to the interrupts? Is it a simple 1:1? The GPIO core has some support for the GPIO drivers to be also interrupt controllers. So if this interrupt control is dedicated to GPIO, you would be better to make it part of the GPIO driver. Andrew ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller @ 2021-09-14 15:59 ` Andrew Lunn 0 siblings, 0 replies; 56+ messages in thread From: Andrew Lunn @ 2021-09-14 15:59 UTC (permalink / raw) To: Daniel Palmer Cc: devicetree, robh+dt, maz, tglx, linux-arm-kernel, romain.perier On Tue, Sep 14, 2021 at 07:04:12PM +0900, Daniel Palmer wrote: > In new SigmaStar SoCs they have moved away from having a few > interrupt capable GPIOs and instead have chained yet another > interrupt controller in to provide interrupt support for > all of the GPIOs. > > I'm hardly an IRQ expert so I expect I've made a total > mess of this. No one else was going to write this driver > so I had a go. Hi Daniel How are the GPIOs mapped to the interrupts? Is it a simple 1:1? The GPIO core has some support for the GPIO drivers to be also interrupt controllers. So if this interrupt control is dedicated to GPIO, you would be better to make it part of the GPIO driver. Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller 2021-09-14 15:59 ` Andrew Lunn @ 2021-09-15 9:06 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-15 9:06 UTC (permalink / raw) To: Andrew Lunn Cc: DTML, Rob Herring, Marc Zyngier, Thomas Gleixner, linux-arm-kernel, Romain Perier Hi Andrew, On Wed, 15 Sept 2021 at 00:59, Andrew Lunn <andrew@lunn.ch> wrote: > How are the GPIOs mapped to the interrupts? Is it a simple 1:1? Unfortunately, no. I wanted to add the GPIO controller part of this to this same series but there are some patches in flight for that so it would have been messy. You can see that here though: https://github.com/linux-chenxing/linux/commit/88345dc470bf07d36aa1ddab09551ed33a1cfb22 They've really made a mess of this. Their whole GPIO thing is a mess with no clear logic between the pin names and the register locations etc. This IRQ part is no exception. IRQ 0 from this thing isn't for the pin called GPIO0 or anything sane like that. > The GPIO core has some support for the GPIO drivers to be also > interrupt controllers. So if this interrupt control is dedicated to > GPIO, you would be better to make it part of the GPIO driver. I don't think so. One reason is the non-linear mapping stuff. A second reason is this GPIO interrupt controller might handle GPIO interrupts for multiple GPIO controller blocks. Finally, in newer chips they've replaced one of the GPIO blocks with a new IP which will need it's own driver. That GPIO controller still seems to use this same IRQ block to handle it's interrupts. So if this code is wrapped into the GPIO driver itself it would end up duplicated in two GPIO drivers. Cheers, Daniel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller @ 2021-09-15 9:06 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-15 9:06 UTC (permalink / raw) To: Andrew Lunn Cc: DTML, Rob Herring, Marc Zyngier, Thomas Gleixner, linux-arm-kernel, Romain Perier Hi Andrew, On Wed, 15 Sept 2021 at 00:59, Andrew Lunn <andrew@lunn.ch> wrote: > How are the GPIOs mapped to the interrupts? Is it a simple 1:1? Unfortunately, no. I wanted to add the GPIO controller part of this to this same series but there are some patches in flight for that so it would have been messy. You can see that here though: https://github.com/linux-chenxing/linux/commit/88345dc470bf07d36aa1ddab09551ed33a1cfb22 They've really made a mess of this. Their whole GPIO thing is a mess with no clear logic between the pin names and the register locations etc. This IRQ part is no exception. IRQ 0 from this thing isn't for the pin called GPIO0 or anything sane like that. > The GPIO core has some support for the GPIO drivers to be also > interrupt controllers. So if this interrupt control is dedicated to > GPIO, you would be better to make it part of the GPIO driver. I don't think so. One reason is the non-linear mapping stuff. A second reason is this GPIO interrupt controller might handle GPIO interrupts for multiple GPIO controller blocks. Finally, in newer chips they've replaced one of the GPIO blocks with a new IP which will need it's own driver. That GPIO controller still seems to use this same IRQ block to handle it's interrupts. So if this code is wrapped into the GPIO driver itself it would end up duplicated in two GPIO drivers. Cheers, Daniel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller 2021-09-15 9:06 ` Daniel Palmer @ 2021-09-15 20:34 ` Andrew Lunn -1 siblings, 0 replies; 56+ messages in thread From: Andrew Lunn @ 2021-09-15 20:34 UTC (permalink / raw) To: Daniel Palmer Cc: DTML, Rob Herring, Marc Zyngier, Thomas Gleixner, linux-arm-kernel, Romain Perier On Wed, Sep 15, 2021 at 06:06:52PM +0900, Daniel Palmer wrote: > Hi Andrew, > > On Wed, 15 Sept 2021 at 00:59, Andrew Lunn <andrew@lunn.ch> wrote: > > How are the GPIOs mapped to the interrupts? Is it a simple 1:1? > > Unfortunately, no. > I wanted to add the GPIO controller part of this to this same series > but there are some patches in flight for that so it would have been > messy. > You can see that here though: > https://github.com/linux-chenxing/linux/commit/88345dc470bf07d36aa1ddab09551ed33a1cfb22 > > They've really made a mess of this. Their whole GPIO thing is a mess > with no clear logic between the pin names and the register locations > etc. > This IRQ part is no exception. IRQ 0 from this thing isn't for the pin > called GPIO0 or anything sane like that. O.K. Then it sounds like splitting GPIO and the IRQ makes sense. This is the sort of thing which is good to put in the cover letter, explaining why you decided to do it this way. Andrew ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller @ 2021-09-15 20:34 ` Andrew Lunn 0 siblings, 0 replies; 56+ messages in thread From: Andrew Lunn @ 2021-09-15 20:34 UTC (permalink / raw) To: Daniel Palmer Cc: DTML, Rob Herring, Marc Zyngier, Thomas Gleixner, linux-arm-kernel, Romain Perier On Wed, Sep 15, 2021 at 06:06:52PM +0900, Daniel Palmer wrote: > Hi Andrew, > > On Wed, 15 Sept 2021 at 00:59, Andrew Lunn <andrew@lunn.ch> wrote: > > How are the GPIOs mapped to the interrupts? Is it a simple 1:1? > > Unfortunately, no. > I wanted to add the GPIO controller part of this to this same series > but there are some patches in flight for that so it would have been > messy. > You can see that here though: > https://github.com/linux-chenxing/linux/commit/88345dc470bf07d36aa1ddab09551ed33a1cfb22 > > They've really made a mess of this. Their whole GPIO thing is a mess > with no clear logic between the pin names and the register locations > etc. > This IRQ part is no exception. IRQ 0 from this thing isn't for the pin > called GPIO0 or anything sane like that. O.K. Then it sounds like splitting GPIO and the IRQ makes sense. This is the sort of thing which is good to put in the cover letter, explaining why you decided to do it this way. Andrew _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller 2021-09-15 20:34 ` Andrew Lunn @ 2021-09-20 8:36 ` Daniel Palmer -1 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-20 8:36 UTC (permalink / raw) To: Andrew Lunn Cc: DTML, Rob Herring, Marc Zyngier, Thomas Gleixner, linux-arm-kernel, Romain Perier Hi Andrew, On Thu, 16 Sept 2021 at 05:34, Andrew Lunn <andrew@lunn.ch> wrote: > O.K. Then it sounds like splitting GPIO and the IRQ makes sense. This > is the sort of thing which is good to put in the cover letter, > explaining why you decided to do it this way. Good point. I'll probably need to send a v2 at some point so I'll add it to the cover letter then. I have a working driver for the other GPIO IP block that uses this interrupt controller now so I can link that too. Thanks, Daniel ^ permalink raw reply [flat|nested] 56+ messages in thread
* Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller @ 2021-09-20 8:36 ` Daniel Palmer 0 siblings, 0 replies; 56+ messages in thread From: Daniel Palmer @ 2021-09-20 8:36 UTC (permalink / raw) To: Andrew Lunn Cc: DTML, Rob Herring, Marc Zyngier, Thomas Gleixner, linux-arm-kernel, Romain Perier Hi Andrew, On Thu, 16 Sept 2021 at 05:34, Andrew Lunn <andrew@lunn.ch> wrote: > O.K. Then it sounds like splitting GPIO and the IRQ makes sense. This > is the sort of thing which is good to put in the cover letter, > explaining why you decided to do it this way. Good point. I'll probably need to send a v2 at some point so I'll add it to the cover letter then. I have a working driver for the other GPIO IP block that uses this interrupt controller now so I can link that too. Thanks, Daniel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 56+ messages in thread
end of thread, other threads:[~2021-10-02 3:10 UTC | newest] Thread overview: 56+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-14 10:04 [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller Daniel Palmer 2021-09-14 10:04 ` Daniel Palmer 2021-09-14 10:04 ` [PATCH 1/3] dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi Daniel Palmer 2021-09-14 10:04 ` Daniel Palmer 2021-09-21 20:36 ` Rob Herring 2021-09-21 20:36 ` Rob Herring 2021-09-14 10:04 ` [PATCH 2/3] irqchip: " Daniel Palmer 2021-09-14 10:04 ` Daniel Palmer 2021-09-20 9:45 ` Marc Zyngier 2021-09-20 9:45 ` Marc Zyngier 2021-09-20 10:05 ` Daniel Palmer 2021-09-20 10:05 ` Daniel Palmer 2021-09-20 11:24 ` Marc Zyngier 2021-09-20 11:24 ` Marc Zyngier 2021-09-21 4:16 ` Daniel Palmer 2021-09-21 4:16 ` Daniel Palmer 2021-09-21 8:27 ` Marc Zyngier 2021-09-21 8:27 ` Marc Zyngier 2021-09-21 18:23 ` Linus Walleij 2021-09-21 18:23 ` Linus Walleij 2021-09-30 12:39 ` Daniel Palmer 2021-09-30 12:39 ` Daniel Palmer 2021-09-30 13:07 ` Marc Zyngier 2021-09-30 13:07 ` Marc Zyngier 2021-09-30 13:10 ` Daniel Palmer 2021-09-30 13:10 ` Daniel Palmer 2021-09-30 13:06 ` Marc Zyngier 2021-09-30 13:06 ` Marc Zyngier 2021-09-30 13:36 ` Daniel Palmer 2021-09-30 13:36 ` Daniel Palmer 2021-09-30 13:53 ` Marc Zyngier 2021-09-30 13:53 ` Marc Zyngier 2021-09-30 13:59 ` Daniel Palmer 2021-09-30 13:59 ` Daniel Palmer 2021-09-30 14:11 ` Marc Zyngier 2021-09-30 14:11 ` Marc Zyngier 2021-09-30 16:10 ` Linus Walleij 2021-09-30 16:10 ` Linus Walleij 2021-09-30 16:13 ` Linus Walleij 2021-09-30 16:13 ` Linus Walleij 2021-10-01 12:33 ` Daniel Palmer 2021-10-01 12:33 ` Daniel Palmer 2021-10-02 3:08 ` Daniel Palmer 2021-10-02 3:08 ` Daniel Palmer 2021-09-21 6:11 ` Daniel Palmer 2021-09-21 6:11 ` Daniel Palmer 2021-09-14 10:04 ` [PATCH 3/3] ARM: dts: mstar: Add gpi interrupt controller to i2m Daniel Palmer 2021-09-14 10:04 ` Daniel Palmer 2021-09-14 15:59 ` [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller Andrew Lunn 2021-09-14 15:59 ` Andrew Lunn 2021-09-15 9:06 ` Daniel Palmer 2021-09-15 9:06 ` Daniel Palmer 2021-09-15 20:34 ` Andrew Lunn 2021-09-15 20:34 ` Andrew Lunn 2021-09-20 8:36 ` Daniel Palmer 2021-09-20 8:36 ` Daniel Palmer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.