[PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[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

[PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[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

[PATCH 1/3] dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi

[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

[PATCH 1/3] dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi

[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

[PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[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

[PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[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

[PATCH 3/3] ARM: dts: mstar: Add gpi interrupt controller to i2m

[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

[PATCH 3/3] ARM: dts: mstar: Add gpi interrupt controller to i2m

[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

Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[Andrew Lunn]

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

Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[Andrew Lunn]

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

Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[Daniel Palmer]

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

Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[Daniel Palmer]

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

Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[Andrew Lunn]

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

Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[Andrew Lunn]

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

Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[Daniel Palmer]

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

Re: [PATCH 0/3] SigmaStar SSD20XD GPIO interrupt controller

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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.

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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.

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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.

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Linus Walleij]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Linus Walleij]

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

Re: [PATCH 1/3] dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi

[Rob Herring]

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>

Re: [PATCH 1/3] dt-bindings: interrupt-controller: Add SigmaStar SSD20xD gpi

[Rob Herring]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

[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.

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

[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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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.

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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.

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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.

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Marc Zyngier]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Linus Walleij]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Linus Walleij]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Linus Walleij]

On Thu, Sep 30, 2021 at 3:37 PM Daniel Palmer <daniel@0x0f.com> wrote:
> On Thu, 30 Sept 2021 at 22:06, Marc Zyngier <maz@kernel.org> wrote:
> > No, this is the right spot if you really need to set the handler. But
> > it should really be after the parent allocation (see below for
> > something totally untested).
>
> Your change resolves the null pointer difference when enabling the
> interrupt but when it triggers the below happens.
> This might just be my driver so I'll try to debug.

To me this looks like your IRQ handler is firing for unused IRQs, i.e.
you are getting spurious IRQs.

Are you missing to disable all IRQs as part of the set-up before
registering the GPIO chip? (Usually some registers need to
be written with zeroes.)

Yours,
Linus Walleij

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Linus Walleij]

On Thu, Sep 30, 2021 at 3:37 PM Daniel Palmer <daniel@0x0f.com> wrote:
> On Thu, 30 Sept 2021 at 22:06, Marc Zyngier <maz@kernel.org> wrote:
> > No, this is the right spot if you really need to set the handler. But
> > it should really be after the parent allocation (see below for
> > something totally untested).
>
> Your change resolves the null pointer difference when enabling the
> interrupt but when it triggers the below happens.
> This might just be my driver so I'll try to debug.

To me this looks like your IRQ handler is firing for unused IRQs, i.e.
you are getting spurious IRQs.

Are you missing to disable all IRQs as part of the set-up before
registering the GPIO chip? (Usually some registers need to
be written with zeroes.)

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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

Re: [PATCH 2/3] irqchip: SigmaStar SSD20xD gpi

[Daniel Palmer]

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