All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH v2 0/4] Renesas RZ/G2L IRQC support
@ 2021-09-21 19:30 Lad Prabhakar
  2021-09-21 19:30 ` [RFC PATCH v2 1/4] dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller Lad Prabhakar
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Lad Prabhakar @ 2021-09-21 19:30 UTC (permalink / raw)
  To: Marc Zyngier, Thomas Gleixner, Geert Uytterhoeven, Rob Herring,
	Linus Walleij, Magnus Damm, linux-gpio, devicetree
  Cc: linux-kernel, linux-renesas-soc, Prabhakar, Biju Das, Lad Prabhakar

Hi All,

The RZ/G2L Interrupt Controller is a front-end for the GIC found on
Renesas RZ/G2L SoC's with below pins:
- IRQ sense select for 8 external interrupts, mapped to 8 GIC SPI interrupts
- GPIO pins used as external interrupt input pins out of GPIOINT0-122 a
  maximum of only 32 can be mapped to 32 GIC SPI interrupts,
- NMI edge select.

                                                                _____________
                                                                |    GIC     |
                                                                |  ________  |
                                         ____________           | |        | |
NMI ------------------------------------>|          |  SPI0-479 | | GIC-600| |
                _______                  |          |------------>|        | |
                |      |                 |          |  PPI16-31 | |        | |
                |      | IRQ0-IRQ8       |   IRQC   |------------>|        | |
P0_P48_4 ------>| GPIO |---------------->|          |           | |________| |
                |      |GPIOINT0-122     |          |           |            |
                |      |---------------->| TINT0-31 |           |            |
                |______|                 |__________|           |____________|

The proposed RFC patches, add the IRQ domains in GPIO (pinctrl driver) and the
IRQC driver. The IRQC domain handles the actual SPI interrupt and upon reception
of the interrupt it propagates to the GPIO IRQ domain to handle virq.
Out of GPIOINT0-122 only 32 can be mapped to GIC SPI, this mapping is handled by
the IRQC driver.

Current implementation only supports TINT interrupts.

Cheers,
Prabhakar

Changes for v2:
-> Re-structured the driver as a chained handler
-> Moved gpio verification to pinctrl driver
-> Added locks to protect read/write
-> Fixed propagating of irq (v1 queried the pinctrl irq domain to propagate)
-> To keep it simple dropped edge both support
-> Used relaxed accessors
-> Switched to use generic_handle_domain_irq()
-> Used irq_domain_translate_twocell() instead of custom callback

Lad Prabhakar (4):
  dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt
    Controller
  irqchip: Add RZ/G2L IA55 Interrupt Controller driver
  pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO
    interrupt
  arm64: dts: renesas: r9a07g044: Add IRQC node to SoC DTSI

 .../renesas,rzg2l-irqc.yaml                   | 130 ++++++
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi    |  58 +++
 drivers/irqchip/Kconfig                       |   9 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-renesas-rzg2l.c           | 393 ++++++++++++++++++
 drivers/pinctrl/renesas/pinctrl-rzg2l.c       | 214 ++++++++++
 drivers/soc/renesas/Kconfig                   |   1 +
 7 files changed, 806 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
 create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c

-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [RFC PATCH v2 1/4] dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller
  2021-09-21 19:30 [RFC PATCH v2 0/4] Renesas RZ/G2L IRQC support Lad Prabhakar
@ 2021-09-21 19:30 ` Lad Prabhakar
  2021-09-23 21:42   ` Linus Walleij
  2021-09-27 17:08   ` Rob Herring
  2021-09-21 19:30 ` [RFC PATCH v2 2/4] irqchip: Add RZ/G2L IA55 Interrupt Controller driver Lad Prabhakar
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 14+ messages in thread
From: Lad Prabhakar @ 2021-09-21 19:30 UTC (permalink / raw)
  To: Marc Zyngier, Thomas Gleixner, Geert Uytterhoeven, Rob Herring,
	Linus Walleij, Magnus Damm, linux-gpio, devicetree
  Cc: linux-kernel, linux-renesas-soc, Prabhakar, Biju Das, Lad Prabhakar

Add DT bindings for the Renesas RZ/G2L Interrupt Controller.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 .../renesas,rzg2l-irqc.yaml                   | 130 ++++++++++++++++++
 1 file changed, 130 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
new file mode 100644
index 000000000000..ab1848d537aa
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
@@ -0,0 +1,130 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/renesas,rzg2l-irqc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Renesas RZ/G2L Interrupt Controller
+
+maintainers:
+  - Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
+  - Geert Uytterhoeven <geert+renesas@glider.be>
+
+description: |
+  The RZ/G2L Interrupt Controller is a front-end for the GIC found on Renesas RZ/G2L SoC's
+    - IRQ sense select for 8 external interrupts, mapped to 8 GIC SPI interrupts
+    - GPIO pins used as external interrupt input pins, mapped to 32 GIC SPI interrupts
+    - NMI edge select (NMI is not treated as NMI exception and supports fall edge and
+      stand-up edge detection interrupts)
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - renesas,r9a07g044-irqc # RZ/G2L
+      - const: renesas,rzg2l-irqc
+
+  '#interrupt-cells':
+    const: 2
+
+  '#address-cells':
+    const: 0
+
+  interrupt-controller: true
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    description: Specifies the GIC interrupts.
+    maxItems: 41
+
+  clocks:
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: clk
+      - const: pclk
+
+  power-domains:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+required:
+  - compatible
+  - '#interrupt-cells'
+  - '#address-cells'
+  - interrupt-controller
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - power-domains
+  - resets
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/r9a07g044-cpg.h>
+
+    irqc: interrupt-controller@110a0000 {
+            compatible = "renesas,r9a07g044-irqc", "renesas,rzg2l-irqc";
+            #interrupt-cells = <2>;
+            #address-cells = <0>;
+            interrupt-controller;
+            reg = <0x110a0000 0x10000>;
+            interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
+                         <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;
+                        clocks = <&cpg CPG_MOD R9A07G044_IA55_CLK>,
+                                 <&cpg CPG_MOD R9A07G044_IA55_PCLK>;
+                        clock-names = "clk", "pclk";
+                        power-domains = <&cpg>;
+                        resets = <&cpg R9A07G044_IA55_RESETN>;
+    };
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH v2 2/4] irqchip: Add RZ/G2L IA55 Interrupt Controller driver
  2021-09-21 19:30 [RFC PATCH v2 0/4] Renesas RZ/G2L IRQC support Lad Prabhakar
  2021-09-21 19:30 ` [RFC PATCH v2 1/4] dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller Lad Prabhakar
@ 2021-09-21 19:30 ` Lad Prabhakar
  2021-09-24 19:01   ` Marc Zyngier
  2021-09-21 19:30 ` [RFC PATCH v2 3/4] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt Lad Prabhakar
  2021-09-21 19:30 ` [RFC PATCH v2 4/4] arm64: dts: renesas: r9a07g044: Add IRQC node to SoC DTSI Lad Prabhakar
  3 siblings, 1 reply; 14+ messages in thread
From: Lad Prabhakar @ 2021-09-21 19:30 UTC (permalink / raw)
  To: Marc Zyngier, Thomas Gleixner, Geert Uytterhoeven, Rob Herring,
	Linus Walleij, Magnus Damm, linux-gpio, devicetree
  Cc: linux-kernel, linux-renesas-soc, Prabhakar, Biju Das, Lad Prabhakar

Add a driver for the Renesas RZ/G2L Interrupt Controller.

This supports external pins being used as interrupts. It supports
one line for NMI, 8 external pins and 32 GPIO pins (out of 123)
to be used as IRQ lines.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/irqchip/Kconfig             |   9 +
 drivers/irqchip/Makefile            |   1 +
 drivers/irqchip/irq-renesas-rzg2l.c | 393 ++++++++++++++++++++++++++++
 drivers/soc/renesas/Kconfig         |   1 +
 4 files changed, 404 insertions(+)
 create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 4d5924e9f766..b61dceac8628 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -236,6 +236,15 @@ config RENESAS_RZA1_IRQC
 	  Enable support for the Renesas RZ/A1 Interrupt Controller, to use up
 	  to 8 external interrupts with configurable sense select.
 
+config RENESAS_RZG2L_IRQC
+	bool "Renesas RZ/G2L IRQC support" if COMPILE_TEST
+	select GENERIC_IRQ_CHIP
+	select IRQ_DOMAIN
+	select IRQ_DOMAIN_HIERARCHY
+	help
+	  Enable support for the Renesas RZ/G2L Interrupt Controller for external
+	  devices.
+
 config SL28CPLD_INTC
 	bool "Kontron sl28cpld IRQ controller"
 	depends on MFD_SL28CPLD=y || COMPILE_TEST
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index f88cbf36a9d2..8017786fbdac 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_RDA_INTC)			+= irq-rda-intc.o
 obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
 obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
 obj-$(CONFIG_RENESAS_RZA1_IRQC)		+= irq-renesas-rza1.o
+obj-$(CONFIG_RENESAS_RZG2L_IRQC)	+= irq-renesas-rzg2l.o
 obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
 obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
 obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
new file mode 100644
index 000000000000..8057fdf6781b
--- /dev/null
+++ b/drivers/irqchip/irq-renesas-rzg2l.c
@@ -0,0 +1,393 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/G2L IRQC Driver
+ *
+ * Copyright (C) 2021 Renesas Electronics Corporation.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdesc.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+#define IRQC_IRQ_START			1
+#define IRQC_IRQ_COUNT			8
+#define IRQC_TINT_START			9
+#define IRQC_TINT_COUNT			32
+#define IRQC_NUM_IRQ			41
+
+#define ISCR				0x10
+#define IITSR				0x14
+#define TSCR				0x20
+#define TITSR0				0x24
+#define TITSR1				0x28
+#define TITSR0_MAX_INT			16
+#define TITSEL_WIDTH			0x2
+#define TSSR(n)				(0x30 + ((n) * 4))
+#define TIEN				BIT(7)
+#define TSSEL_SHIFT(n)			(8 * (n))
+#define TSSEL_MASK			GENMASK(7, 0)
+#define IRQ_MASK			0x3
+
+#define TSSR_OFFSET(n)			((n) % 4)
+#define TSSR_INDEX(n)			((n) / 4)
+
+#define TITSR_TITSEL_EDGE_RISING	0
+#define TITSR_TITSEL_EDGE_FALLING	1
+#define TITSR_TITSEL_LEVEL_HIGH		2
+#define TITSR_TITSEL_LEVEL_LOW		3
+
+struct rzg2l_irqc_priv {
+	void __iomem *base;
+	struct device *dev;
+	struct irq_chip chip;
+	struct irq_domain *irq_domain;
+	struct resource *irq[IRQC_NUM_IRQ];
+	struct mutex irqc_mutex;
+	struct mutex tint_mutex; /* Mutex to protect tint_slot bitmap */
+	DECLARE_BITMAP(tint_slot, IRQC_TINT_COUNT);
+};
+
+struct rzg2l_irqc_chip_data {
+	struct rzg2l_irqc_priv *priv;
+	int tint;
+	int hwirq;
+};
+
+static int rzg2l_tint_set_edge(struct rzg2l_irqc_priv *priv,
+			       unsigned int hwirq, unsigned int type)
+{
+	u32 port = hwirq - IRQC_TINT_START;
+	u8 sense;
+	u32 reg;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_EDGE_RISING:
+		sense = TITSR_TITSEL_EDGE_RISING;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		sense = TITSR_TITSEL_EDGE_FALLING;
+		break;
+
+	case IRQ_TYPE_LEVEL_HIGH:
+		sense = TITSR_TITSEL_LEVEL_HIGH;
+		break;
+
+	case IRQ_TYPE_LEVEL_LOW:
+		sense = TITSR_TITSEL_LEVEL_LOW;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	mutex_lock(&priv->irqc_mutex);
+	if (port < TITSR0_MAX_INT) {
+		reg = readl_relaxed(priv->base + TITSR0);
+		reg &= ~(IRQ_MASK << (port * TITSEL_WIDTH));
+		reg |= sense << (port * TITSEL_WIDTH);
+		writel_relaxed(reg, priv->base + TITSR0);
+	} else {
+		port = port / TITSEL_WIDTH;
+		reg = readl_relaxed(priv->base + TITSR1);
+		reg &= ~(IRQ_MASK << (port * TITSEL_WIDTH));
+		reg |= sense << (port * TITSEL_WIDTH);
+		writel_relaxed(reg, priv->base + TITSR1);
+	}
+	mutex_unlock(&priv->irqc_mutex);
+
+	return 0;
+}
+
+static void rzg2l_irqc_tint_irq_handler(struct irq_desc *desc)
+{
+	unsigned int irq = irq_desc_get_irq(desc);
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	struct rzg2l_irqc_chip_data *chip_data = irq_get_handler_data(irq);
+	struct rzg2l_irqc_priv *priv = chip_data->priv;
+	unsigned int offset;
+	u32 reg;
+
+	chained_irq_enter(chip, desc);
+	offset = chip_data->hwirq - IRQC_TINT_START;
+	generic_handle_domain_irq(priv->irq_domain, chip_data->hwirq);
+	reg = readl_relaxed(priv->base + TSCR) & ~BIT(offset);
+	writel_relaxed(reg, priv->base + TSCR);
+	chained_irq_exit(chip, desc);
+}
+
+static void rzg2l_irqc_irq_disable(struct irq_data *d)
+{
+	struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
+	struct rzg2l_irqc_priv *priv = chip_data->priv;
+
+	if (chip_data->tint != -EINVAL) {
+		u32 offset = chip_data->hwirq - IRQC_TINT_START;
+		u32 tssr_offset = TSSR_OFFSET(offset);
+		u8 tssr_index = TSSR_INDEX(offset);
+		u32 reg;
+
+		irq_set_chained_handler_and_data(priv->irq[chip_data->hwirq]->start,
+						 NULL, NULL);
+
+		mutex_lock(&priv->irqc_mutex);
+		reg = readl_relaxed(priv->base + TSSR(tssr_index));
+		reg &= ~(TSSEL_MASK << tssr_offset);
+		writel_relaxed(reg, priv->base + TSSR(tssr_index));
+		mutex_unlock(&priv->irqc_mutex);
+	}
+}
+
+static void rzg2l_irqc_irq_enable(struct irq_data *d)
+{
+	struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
+	struct rzg2l_irqc_priv *priv = chip_data->priv;
+
+	if (chip_data->tint != -EINVAL) {
+		u32 offset = chip_data->hwirq - IRQC_TINT_START;
+		u32 tssr_offset = TSSR_OFFSET(offset);
+		u8 tssr_index = TSSR_INDEX(offset);
+		u32 reg;
+
+		irq_set_chained_handler_and_data(priv->irq[chip_data->hwirq]->start,
+						 rzg2l_irqc_tint_irq_handler,
+						 chip_data);
+
+		mutex_lock(&priv->irqc_mutex);
+		reg = readl_relaxed(priv->base + TSSR(tssr_index));
+		reg |= (TIEN | chip_data->tint) << TSSEL_SHIFT(tssr_offset);
+		writel_relaxed(reg, priv->base + TSSR(tssr_index));
+		mutex_unlock(&priv->irqc_mutex);
+	}
+}
+
+static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type)
+{
+	struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
+	struct rzg2l_irqc_priv *priv = chip_data->priv;
+
+	if (chip_data->tint != EINVAL)
+		return rzg2l_tint_set_edge(priv, chip_data->hwirq, type);
+
+	return -EINVAL;
+}
+
+static int rzg2l_irqc_domain_alloc(struct irq_domain *domain, unsigned int virq,
+				   unsigned int nr_irqs, void *arg)
+{
+	struct rzg2l_irqc_priv *priv = domain->host_data;
+	struct rzg2l_irqc_chip_data *chip_data = NULL;
+	struct irq_fwspec parent_fwspec;
+	struct irq_fwspec *fwspec = arg;
+	int tint = -EINVAL;
+	irq_hw_number_t hwirq;
+	int irq = -EINVAL;
+	unsigned int type;
+	int ret;
+
+	chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
+	if (!chip_data)
+		return -ENOMEM;
+
+	ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	/*
+	 * When alloc has landed from pinctrl driver:
+	 * fwspec->param_count = 3
+	 * fwspec->param[0] = tint
+	 * fwspec->param[1] = type
+	 * fwspec->param[2] = 1
+	 */
+	if (fwspec->param_count == 3 && fwspec->param[2]) {
+		mutex_lock(&priv->tint_mutex);
+		irq = bitmap_find_free_region(priv->tint_slot, IRQC_TINT_COUNT, get_order(1));
+		if (irq < 0) {
+			mutex_unlock(&priv->tint_mutex);
+			return -ENOSPC;
+		}
+		mutex_unlock(&priv->tint_mutex);
+		tint = hwirq;
+		hwirq = irq + IRQC_TINT_START;
+	}
+
+	chip_data->priv = priv;
+	chip_data->tint = tint;
+	chip_data->hwirq = hwirq;
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &priv->chip, chip_data);
+	if (ret)
+		goto release_bitmap;
+
+	/* parent is GIC */
+	parent_fwspec.fwnode = domain->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0] = 0; /* SPI */
+	parent_fwspec.param[1] = priv->irq[hwirq]->start;
+	parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
+	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_fwspec);
+	if (ret)
+		goto release_bitmap;
+
+	return 0;
+
+release_bitmap:
+	if (tint != -EINVAL) {
+		mutex_lock(&priv->tint_mutex);
+		bitmap_release_region(priv->tint_slot, irq, get_order(1));
+		mutex_unlock(&priv->tint_mutex);
+	}
+	kfree(chip_data);
+	return ret;
+}
+
+static void rzg2l_irqc_domain_free(struct irq_domain *domain, unsigned int virq,
+				   unsigned int nr_irqs)
+{
+	struct rzg2l_irqc_chip_data *chip_data;
+	struct rzg2l_irqc_priv *priv;
+	struct irq_data *d;
+
+	d = irq_domain_get_irq_data(domain, virq);
+	if (d) {
+		chip_data = d->chip_data;
+		priv = chip_data->priv;
+		if (chip_data) {
+			if (chip_data->tint != -EINVAL) {
+				mutex_lock(&priv->tint_mutex);
+				bitmap_release_region(priv->tint_slot,
+						      chip_data->hwirq - IRQC_TINT_START,
+						      get_order(1));
+				mutex_unlock(&priv->tint_mutex);
+			}
+			kfree(chip_data);
+		}
+	}
+
+	irq_domain_free_irqs_common(domain, virq, nr_irqs);
+}
+
+static const struct irq_domain_ops rzg2l_irqc_domain_ops = {
+	.alloc = rzg2l_irqc_domain_alloc,
+	.free = rzg2l_irqc_domain_free,
+	.translate = irq_domain_translate_twocell,
+};
+
+static int rzg2l_irqc_probe(struct platform_device *pdev)
+{
+	struct irq_domain *parent_irq_domain;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *gic_node;
+	struct rzg2l_irqc_priv *priv;
+	unsigned int i;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, priv);
+	priv->dev = dev;
+
+	priv->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(priv->base))
+		return PTR_ERR(priv->base);
+
+	gic_node = of_irq_find_parent(np);
+	if (gic_node)
+		parent_irq_domain = irq_find_host(gic_node);
+
+	if (!parent_irq_domain) {
+		dev_err(dev, "cannot find parent domain\n");
+		ret = -ENODEV;
+		goto out_put_node;
+	}
+
+	for (i = 0; i < IRQC_NUM_IRQ; i++) {
+		priv->irq[i] = platform_get_resource(pdev, IORESOURCE_IRQ, i);
+		if (!priv->irq[i]) {
+			ret = -ENOENT;
+			dev_err(dev, "failed to get irq resource(%u)", i);
+			goto out_put_node;
+		}
+	}
+
+	mutex_init(&priv->irqc_mutex);
+	mutex_init(&priv->tint_mutex);
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_resume_and_get(&pdev->dev);
+
+	priv->chip.name = "rzg2l-irqc";
+	priv->chip.irq_mask = irq_chip_mask_parent;
+	priv->chip.irq_unmask = irq_chip_unmask_parent;
+	priv->chip.irq_enable = rzg2l_irqc_irq_enable;
+	priv->chip.irq_disable = rzg2l_irqc_irq_disable;
+	priv->chip.irq_retrigger = irq_chip_retrigger_hierarchy;
+	priv->chip.irq_set_type = rzg2l_irqc_set_type;
+	priv->chip.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE;
+	priv->irq_domain = irq_domain_add_hierarchy(parent_irq_domain, 0,
+						    IRQC_NUM_IRQ, np,
+						    &rzg2l_irqc_domain_ops,
+						    priv);
+	if (!priv->irq_domain) {
+		dev_err(dev, "cannot initialize irq domain\n");
+		ret = -ENOMEM;
+		pm_runtime_put(&pdev->dev);
+		pm_runtime_disable(&pdev->dev);
+		goto out_put_node;
+	}
+
+out_put_node:
+	of_node_put(gic_node);
+	return ret;
+}
+
+static int rzg2l_irqc_remove(struct platform_device *pdev)
+{
+	struct rzg2l_irqc_priv *priv = platform_get_drvdata(pdev);
+
+	irq_domain_remove(priv->irq_domain);
+	pm_runtime_put(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	return 0;
+}
+
+static const struct of_device_id rzg2l_irqc_dt_ids[] = {
+	{ .compatible = "renesas,rzg2l-irqc" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, rzg2l_irqc_dt_ids);
+
+static struct platform_driver rzg2l_irqc_device_driver = {
+	.probe		= rzg2l_irqc_probe,
+	.remove		= rzg2l_irqc_remove,
+	.driver		= {
+		.name	= "renesas_rzg2l_irqc",
+		.of_match_table	= rzg2l_irqc_dt_ids,
+	}
+};
+
+static int __init rzg2l_irqc_init(void)
+{
+	return platform_driver_register(&rzg2l_irqc_device_driver);
+}
+postcore_initcall(rzg2l_irqc_init);
+
+static void __exit rzg2l_irqc_exit(void)
+{
+	platform_driver_unregister(&rzg2l_irqc_device_driver);
+}
+module_exit(rzg2l_irqc_exit);
+
+MODULE_DESCRIPTION("Renesas RZ/G2L IRQC Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig
index ce16ef5c939c..fec93883c3b8 100644
--- a/drivers/soc/renesas/Kconfig
+++ b/drivers/soc/renesas/Kconfig
@@ -286,6 +286,7 @@ config ARCH_R8A774B1
 
 config ARCH_R9A07G044
 	bool "ARM64 Platform support for RZ/G2L"
+	select RENESAS_RZG2L_IRQC
 	help
 	  This enables support for the Renesas RZ/G2L SoC variants.
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH v2 3/4] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt
  2021-09-21 19:30 [RFC PATCH v2 0/4] Renesas RZ/G2L IRQC support Lad Prabhakar
  2021-09-21 19:30 ` [RFC PATCH v2 1/4] dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller Lad Prabhakar
  2021-09-21 19:30 ` [RFC PATCH v2 2/4] irqchip: Add RZ/G2L IA55 Interrupt Controller driver Lad Prabhakar
@ 2021-09-21 19:30 ` Lad Prabhakar
  2021-09-23 21:37   ` Linus Walleij
  2021-09-21 19:30 ` [RFC PATCH v2 4/4] arm64: dts: renesas: r9a07g044: Add IRQC node to SoC DTSI Lad Prabhakar
  3 siblings, 1 reply; 14+ messages in thread
From: Lad Prabhakar @ 2021-09-21 19:30 UTC (permalink / raw)
  To: Marc Zyngier, Thomas Gleixner, Geert Uytterhoeven, Rob Herring,
	Linus Walleij, Magnus Damm, linux-gpio, devicetree
  Cc: linux-kernel, linux-renesas-soc, Prabhakar, Biju Das, Lad Prabhakar

Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.

GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
used as IRQ lines at given time. Selection of pins as IRQ lines
is handled by IA55 (which is the IRQC block) which sits in between the
GPIO and GIC.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 214 ++++++++++++++++++++++++
 1 file changed, 214 insertions(+)

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index dbf2f521bb27..222fbcebecde 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -9,8 +9,10 @@
 #include <linux/clk.h>
 #include <linux/gpio/driver.h>
 #include <linux/io.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/pinctrl/pinconf-generic.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinctrl.h>
@@ -87,6 +89,7 @@
 #define PFC(n)			(0x0400 + 0x40 + (n) * 4)
 #define PIN(n)			(0x0800 + 0x10 + (n))
 #define IEN(n)			(0x1800 + (n) * 8)
+#define ISEL(n)			(0x2C80 + (n) * 8)
 #define PWPR			(0x3014)
 #define SD_CH(n)		(0x3000 + (n) * 4)
 #define QSPI			(0x3008)
@@ -108,6 +111,9 @@
 #define RZG2L_PIN_ID_TO_PORT(id)	((id) / RZG2L_PINS_PER_PORT)
 #define RZG2L_PIN_ID_TO_PIN(id)		((id) % RZG2L_PINS_PER_PORT)
 
+#define RZG2L_TINT_MAX_INTERRUPT	32
+#define RZG2L_TINT_IRQ_START_INDEX	9
+
 struct rzg2l_dedicated_configs {
 	const char *name;
 	u32 config;
@@ -133,10 +139,17 @@ struct rzg2l_pinctrl {
 
 	struct gpio_chip		gpio_chip;
 	struct pinctrl_gpio_range	gpio_range;
+	struct irq_chip			irq_chip;
+	struct irq_domain		*domain;
 
 	spinlock_t			lock;
 };
 
+struct rzg2l_pinctrl_irq_chip_data {
+	struct rzg2l_pinctrl *pctrl;
+	unsigned int irq;
+};
+
 static void rzg2l_pinctrl_set_pfc_mode(struct rzg2l_pinctrl *pctrl,
 				       u8 port, u8 pin, u8 func)
 {
@@ -782,6 +795,22 @@ static void rzg2l_gpio_free(struct gpio_chip *chip, unsigned int offset)
 	rzg2l_gpio_direction_input(chip, offset);
 }
 
+static int rzg2l_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct irq_fwspec fwspec;
+
+	fwspec.fwnode = of_node_to_fwnode(chip->parent->of_node);
+	fwspec.param_count = 2;
+	fwspec.param[0] = offset;
+	/*
+	 * IRQ_TYPE_NONE is rejected by the parent irq domain. Set EDGE_RISING
+	 * temporarily. Anyway, ->irq_set_type() will override it later.
+	 */
+	fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
+
+	return irq_create_fwspec_mapping(&fwspec);
+}
+
 static const char * const rzg2l_gpio_names[] = {
 	"P0_0", "P0_1", "P0_2", "P0_3", "P0_4", "P0_5", "P0_6", "P0_7",
 	"P1_0", "P1_1", "P1_2", "P1_3", "P1_4", "P1_5", "P1_6", "P1_7",
@@ -965,14 +994,183 @@ static  struct rzg2l_dedicated_configs rzg2l_dedicated_pins[] = {
 	{ "RIIC1_SCL", RZG2L_SINGLE_PIN_PACK(0xe, 3, PIN_CFG_IEN) },
 };
 
+static int rzg2l_gpio_get_gpioint(unsigned int virq)
+{
+	unsigned int gpioint = 0;
+	unsigned int i = 0;
+	u32 port, bit;
+
+	port = virq / 8;
+	bit = virq % 8;
+
+	if (port >= ARRAY_SIZE(rzg2l_gpio_configs))
+		return -EINVAL;
+
+	for (i = 0; i < port; i++)
+		gpioint += RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]);
+
+	if (bit >= RZG2L_GPIO_PORT_GET_PINCNT(rzg2l_gpio_configs[i]))
+		return -EINVAL;
+
+	gpioint += bit;
+
+	return gpioint;
+}
+
+static int rzg2l_gpio_irq_domain_alloc(struct irq_domain *domain,
+				       unsigned int virq,
+				       unsigned int nr_irqs, void *arg)
+{
+	struct rzg2l_pinctrl_irq_chip_data *chip_data = NULL;
+	struct rzg2l_pinctrl *pctrl = domain->host_data;
+	struct irq_fwspec parent_fwspec;
+	irq_hw_number_t hwirq;
+	unsigned int type;
+	int irq, ret;
+	int gpioint;
+
+	if (WARN_ON(nr_irqs != 1))
+		return -EINVAL;
+
+	ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type);
+	if (ret)
+		return ret;
+
+	gpioint = rzg2l_gpio_get_gpioint(hwirq);
+	if (gpioint < 0)
+		return ret;
+
+	chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
+	if (!chip_data)
+		return -ENOMEM;
+
+	chip_data->pctrl = pctrl;
+	chip_data->irq = irq;
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+					    &pctrl->irq_chip, chip_data);
+	if (ret)
+		goto free_data;
+
+	/* parent is IRQC */
+	parent_fwspec.fwnode = domain->parent->fwnode;
+	parent_fwspec.param_count = 3;
+	parent_fwspec.param[0] = gpioint;
+	parent_fwspec.param[1] = type;
+	parent_fwspec.param[2] = 1;
+	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &parent_fwspec);
+	if (ret)
+		goto free_data;
+
+	return 0;
+
+free_data:
+	kfree(chip_data);
+	return ret;
+}
+
+static void rzg2l_gpio_irq_domain_free(struct irq_domain *domain, unsigned int virq,
+				       unsigned int nr_irqs)
+{
+	struct rzg2l_pinctrl_irq_chip_data *chip_data;
+	struct irq_data *d;
+
+	d = irq_domain_get_irq_data(domain, virq);
+	if (d) {
+		chip_data = d->chip_data;
+		kfree(chip_data);
+	}
+
+	irq_domain_free_irqs_common(domain, virq, nr_irqs);
+}
+
+static const struct irq_domain_ops rzg2l_gpio_irq_domain_ops = {
+	.alloc = rzg2l_gpio_irq_domain_alloc,
+	.free = rzg2l_gpio_irq_domain_free,
+	.translate = irq_domain_translate_twocell,
+};
+
+static void rzg2l_gpio_irq_disable(struct irq_data *d)
+{
+	struct rzg2l_pinctrl_irq_chip_data *chip_data = d->chip_data;
+	struct rzg2l_pinctrl *pctrl = chip_data->pctrl;
+	int hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	void __iomem *addr;
+	u32 port;
+	u8 bit;
+
+	port = RZG2L_PIN_ID_TO_PORT(hwirq);
+	bit = RZG2L_PIN_ID_TO_PIN(hwirq);
+
+	addr = pctrl->base + ISEL(port);
+	if (bit >= 4) {
+		bit -= 4;
+		addr += 4;
+	}
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	writel(readl(addr) & ~BIT(bit * 8), addr);
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	irq_chip_disable_parent(d);
+}
+
+static void rzg2l_gpio_irq_enable(struct irq_data *d)
+{
+	struct rzg2l_pinctrl_irq_chip_data *chip_data = d->chip_data;
+	struct rzg2l_pinctrl *pctrl = chip_data->pctrl;
+	int hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	void __iomem *addr;
+	u32 port;
+	u8 bit;
+
+	port = RZG2L_PIN_ID_TO_PORT(hwirq);
+	bit = RZG2L_PIN_ID_TO_PIN(hwirq);
+
+	addr = pctrl->base + ISEL(port);
+	if (bit >= 4) {
+		bit -= 4;
+		addr += 4;
+	}
+
+	spin_lock_irqsave(&pctrl->lock, flags);
+	writel(readl(addr) | BIT(bit * 8), addr);
+	spin_unlock_irqrestore(&pctrl->lock, flags);
+
+	irq_chip_enable_parent(d);
+}
+
+static int rzg2l_gpio_irq_set_type(struct irq_data *d, unsigned int type)
+{
+	return irq_chip_set_type_parent(d, type);
+}
+
+static void rzg2l_gpio_irqc_eoi(struct irq_data *d)
+{
+	/* we have nothing to do here */
+}
+
 static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
 {
+	struct irq_chip *irq_chip = &pctrl->irq_chip;
 	struct device_node *np = pctrl->dev->of_node;
 	struct gpio_chip *chip = &pctrl->gpio_chip;
 	const char *name = dev_name(pctrl->dev);
+	struct irq_domain *parent_domain;
 	struct of_phandle_args of_args;
+	struct device_node *parent_np;
 	int ret;
 
+	parent_np = of_irq_find_parent(np);
+	if (!parent_np)
+		return -ENXIO;
+
+	parent_domain = irq_find_host(parent_np);
+	of_node_put(parent_np);
+	if (!parent_domain)
+		return -EPROBE_DEFER;
+
 	ret = of_parse_phandle_with_fixed_args(np, "gpio-ranges", 3, 0, &of_args);
 	if (ret) {
 		dev_err(pctrl->dev, "Unable to parse gpio-ranges\n");
@@ -991,6 +1189,7 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
 	chip->get_direction = rzg2l_gpio_get_direction;
 	chip->direction_input = rzg2l_gpio_direction_input;
 	chip->direction_output = rzg2l_gpio_direction_output;
+	chip->to_irq = rzg2l_gpio_to_irq;
 	chip->get = rzg2l_gpio_get;
 	chip->set = rzg2l_gpio_set;
 	chip->label = name;
@@ -999,6 +1198,13 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
 	chip->base = -1;
 	chip->ngpio = of_args.args[2];
 
+	irq_chip->name = chip->label;
+	irq_chip->irq_disable = rzg2l_gpio_irq_disable;
+	irq_chip->irq_enable = rzg2l_gpio_irq_enable;
+	irq_chip->irq_set_type = rzg2l_gpio_irq_set_type;
+	irq_chip->irq_eoi = rzg2l_gpio_irqc_eoi;
+	irq_chip->flags = IRQCHIP_SET_TYPE_MASKED;
+
 	pctrl->gpio_range.id = 0;
 	pctrl->gpio_range.pin_base = 0;
 	pctrl->gpio_range.base = 0;
@@ -1011,6 +1217,14 @@ static int rzg2l_gpio_register(struct rzg2l_pinctrl *pctrl)
 		return ret;
 	}
 
+	pctrl->domain = irq_domain_create_hierarchy(parent_domain, 0,
+						    RZG2L_TINT_MAX_INTERRUPT,
+						    of_node_to_fwnode(np),
+						    &rzg2l_gpio_irq_domain_ops,
+						    pctrl);
+	if (!pctrl->domain)
+		return -ENOMEM;
+
 	dev_dbg(pctrl->dev, "Registered gpio controller\n");
 
 	return 0;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [RFC PATCH v2 4/4] arm64: dts: renesas: r9a07g044: Add IRQC node to SoC DTSI
  2021-09-21 19:30 [RFC PATCH v2 0/4] Renesas RZ/G2L IRQC support Lad Prabhakar
                   ` (2 preceding siblings ...)
  2021-09-21 19:30 ` [RFC PATCH v2 3/4] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt Lad Prabhakar
@ 2021-09-21 19:30 ` Lad Prabhakar
  3 siblings, 0 replies; 14+ messages in thread
From: Lad Prabhakar @ 2021-09-21 19:30 UTC (permalink / raw)
  To: Marc Zyngier, Thomas Gleixner, Geert Uytterhoeven, Rob Herring,
	Linus Walleij, Magnus Damm, linux-gpio, devicetree
  Cc: linux-kernel, linux-renesas-soc, Prabhakar, Biju Das, Lad Prabhakar

Add IRQC node to R9A07G044 (RZ/G2L) SoC DTSI.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 58 ++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
index 9c170db544d2..d10cedeb5880 100644
--- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
+++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi
@@ -388,6 +388,8 @@
 			reg = <0 0x11030000 0 0x10000>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			interrupt-parent = <&irqc>;
+			interrupt-controller;
 			gpio-ranges = <&pinctrl 0 0 392>;
 			clocks = <&cpg CPG_MOD R9A07G044_GPIO_HCLK>;
 			power-domains = <&cpg>;
@@ -396,6 +398,62 @@
 				 <&cpg R9A07G044_GPIO_SPARE_RESETN>;
 		};
 
+		irqc: interrupt-controller@110a0000 {
+			compatible = "renesas,r9a07g044-irqc",
+				     "renesas,rzg2l-irqc";
+			#interrupt-cells = <2>;
+			#address-cells = <0>;
+			interrupt-controller;
+			reg = <0 0x110a0000 0 0x10000>;
+			interrupts =
+				<GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
+				<GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&cpg CPG_MOD R9A07G044_IA55_CLK>,
+				 <&cpg CPG_MOD R9A07G044_IA55_PCLK>;
+			clock-names = "clk", "pclk";
+			power-domains = <&cpg>;
+			resets = <&cpg R9A07G044_IA55_RESETN>;
+		};
+
 		dmac: dma-controller@11820000 {
 			compatible = "renesas,r9a07g044-dmac",
 				     "renesas,rz-dmac";
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH v2 3/4] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt
  2021-09-21 19:30 ` [RFC PATCH v2 3/4] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt Lad Prabhakar
@ 2021-09-23 21:37   ` Linus Walleij
  2021-09-24 21:48     ` Lad, Prabhakar
  2021-10-05  9:56     ` Geert Uytterhoeven
  0 siblings, 2 replies; 14+ messages in thread
From: Linus Walleij @ 2021-09-23 21:37 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: Marc Zyngier, Thomas Gleixner, Geert Uytterhoeven, Rob Herring,
	Magnus Damm, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, Linux-Renesas, Prabhakar, Biju Das

On Tue, Sep 21, 2021 at 9:30 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

> Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
>
> GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> used as IRQ lines at given time. Selection of pins as IRQ lines
> is handled by IA55 (which is the IRQC block) which sits in between the
> GPIO and GIC.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Why can't you just use the hierarchical IRQ domain handling inside
gpiolib?

See for example drivers/gpio/gpio-ixp4xx.c for an example of how this
is used.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH v2 1/4] dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller
  2021-09-21 19:30 ` [RFC PATCH v2 1/4] dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller Lad Prabhakar
@ 2021-09-23 21:42   ` Linus Walleij
  2021-09-27 17:08   ` Rob Herring
  1 sibling, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2021-09-23 21:42 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: Marc Zyngier, Thomas Gleixner, Geert Uytterhoeven, Rob Herring,
	Magnus Damm, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, Linux-Renesas, Prabhakar, Biju Das

On Tue, Sep 21, 2021 at 9:30 PM Lad Prabhakar
<prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:

> Add DT bindings for the Renesas RZ/G2L Interrupt Controller.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
(...)
> +            interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 444 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 445 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 446 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 447 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 448 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 449 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 450 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 451 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 452 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 453 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 454 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 455 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 456 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 457 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 458 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 459 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 460 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 461 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 462 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 463 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 464 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 465 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 466 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 467 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 468 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 469 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 470 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 471 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 472 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 473 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 474 IRQ_TYPE_LEVEL_HIGH>,
> +                         <GIC_SPI 475 IRQ_TYPE_LEVEL_HIGH>;

It is not custom to code all the interrupts into the device tree if this
a one-to-one mapping, instead the hardware driver is supposed to
know which IRQs to pick in the 1-to-1 map based on the compatible
string, which should be unique per-SoC.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH v2 2/4] irqchip: Add RZ/G2L IA55 Interrupt Controller driver
  2021-09-21 19:30 ` [RFC PATCH v2 2/4] irqchip: Add RZ/G2L IA55 Interrupt Controller driver Lad Prabhakar
@ 2021-09-24 19:01   ` Marc Zyngier
  2021-09-24 22:27     ` Lad, Prabhakar
  0 siblings, 1 reply; 14+ messages in thread
From: Marc Zyngier @ 2021-09-24 19:01 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: Thomas Gleixner, Geert Uytterhoeven, Rob Herring, Linus Walleij,
	Magnus Damm, linux-gpio, devicetree, linux-kernel,
	linux-renesas-soc, Prabhakar, Biju Das

On Tue, 21 Sep 2021 20:30:26 +0100,
Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> 
> Add a driver for the Renesas RZ/G2L Interrupt Controller.
> 
> This supports external pins being used as interrupts. It supports
> one line for NMI, 8 external pins and 32 GPIO pins (out of 123)
> to be used as IRQ lines.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/irqchip/Kconfig             |   9 +
>  drivers/irqchip/Makefile            |   1 +
>  drivers/irqchip/irq-renesas-rzg2l.c | 393 ++++++++++++++++++++++++++++
>  drivers/soc/renesas/Kconfig         |   1 +
>  4 files changed, 404 insertions(+)
>  create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 4d5924e9f766..b61dceac8628 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -236,6 +236,15 @@ config RENESAS_RZA1_IRQC
>  	  Enable support for the Renesas RZ/A1 Interrupt Controller, to use up
>  	  to 8 external interrupts with configurable sense select.
>  
> +config RENESAS_RZG2L_IRQC
> +	bool "Renesas RZ/G2L IRQC support" if COMPILE_TEST
> +	select GENERIC_IRQ_CHIP
> +	select IRQ_DOMAIN
> +	select IRQ_DOMAIN_HIERARCHY
> +	help
> +	  Enable support for the Renesas RZ/G2L Interrupt Controller for external
> +	  devices.
> +
>  config SL28CPLD_INTC
>  	bool "Kontron sl28cpld IRQ controller"
>  	depends on MFD_SL28CPLD=y || COMPILE_TEST
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index f88cbf36a9d2..8017786fbdac 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_RDA_INTC)			+= irq-rda-intc.o
>  obj-$(CONFIG_RENESAS_INTC_IRQPIN)	+= irq-renesas-intc-irqpin.o
>  obj-$(CONFIG_RENESAS_IRQC)		+= irq-renesas-irqc.o
>  obj-$(CONFIG_RENESAS_RZA1_IRQC)		+= irq-renesas-rza1.o
> +obj-$(CONFIG_RENESAS_RZG2L_IRQC)	+= irq-renesas-rzg2l.o
>  obj-$(CONFIG_VERSATILE_FPGA_IRQ)	+= irq-versatile-fpga.o
>  obj-$(CONFIG_ARCH_NSPIRE)		+= irq-zevio.o
>  obj-$(CONFIG_ARCH_VT8500)		+= irq-vt8500.o
> diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> new file mode 100644
> index 000000000000..8057fdf6781b
> --- /dev/null
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -0,0 +1,393 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/G2L IRQC Driver
> + *
> + * Copyright (C) 2021 Renesas Electronics Corporation.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdesc.h>
> +#include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#define IRQC_IRQ_START			1
> +#define IRQC_IRQ_COUNT			8
> +#define IRQC_TINT_START			9
> +#define IRQC_TINT_COUNT			32
> +#define IRQC_NUM_IRQ			41
> +
> +#define ISCR				0x10
> +#define IITSR				0x14
> +#define TSCR				0x20
> +#define TITSR0				0x24
> +#define TITSR1				0x28
> +#define TITSR0_MAX_INT			16
> +#define TITSEL_WIDTH			0x2
> +#define TSSR(n)				(0x30 + ((n) * 4))
> +#define TIEN				BIT(7)
> +#define TSSEL_SHIFT(n)			(8 * (n))
> +#define TSSEL_MASK			GENMASK(7, 0)
> +#define IRQ_MASK			0x3
> +
> +#define TSSR_OFFSET(n)			((n) % 4)
> +#define TSSR_INDEX(n)			((n) / 4)
> +
> +#define TITSR_TITSEL_EDGE_RISING	0
> +#define TITSR_TITSEL_EDGE_FALLING	1
> +#define TITSR_TITSEL_LEVEL_HIGH		2
> +#define TITSR_TITSEL_LEVEL_LOW		3
> +
> +struct rzg2l_irqc_priv {
> +	void __iomem *base;
> +	struct device *dev;
> +	struct irq_chip chip;
> +	struct irq_domain *irq_domain;
> +	struct resource *irq[IRQC_NUM_IRQ];
> +	struct mutex irqc_mutex;
> +	struct mutex tint_mutex; /* Mutex to protect tint_slot bitmap */
> +	DECLARE_BITMAP(tint_slot, IRQC_TINT_COUNT);
> +};
> +
> +struct rzg2l_irqc_chip_data {
> +	struct rzg2l_irqc_priv *priv;
> +	int tint;
> +	int hwirq;
> +};
> +
> +static int rzg2l_tint_set_edge(struct rzg2l_irqc_priv *priv,
> +			       unsigned int hwirq, unsigned int type)
> +{
> +	u32 port = hwirq - IRQC_TINT_START;
> +	u8 sense;
> +	u32 reg;
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_EDGE_RISING:
> +		sense = TITSR_TITSEL_EDGE_RISING;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_FALLING:
> +		sense = TITSR_TITSEL_EDGE_FALLING;
> +		break;
> +
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		sense = TITSR_TITSEL_LEVEL_HIGH;
> +		break;
> +
> +	case IRQ_TYPE_LEVEL_LOW:
> +		sense = TITSR_TITSEL_LEVEL_LOW;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&priv->irqc_mutex);

Have you tested this code with lockdep? This cannot possibly work,
since all the irqchip callbacks are running under a per-irq raw
spinlock.

> +	if (port < TITSR0_MAX_INT) {
> +		reg = readl_relaxed(priv->base + TITSR0);
> +		reg &= ~(IRQ_MASK << (port * TITSEL_WIDTH));
> +		reg |= sense << (port * TITSEL_WIDTH);
> +		writel_relaxed(reg, priv->base + TITSR0);
> +	} else {
> +		port = port / TITSEL_WIDTH;
> +		reg = readl_relaxed(priv->base + TITSR1);
> +		reg &= ~(IRQ_MASK << (port * TITSEL_WIDTH));
> +		reg |= sense << (port * TITSEL_WIDTH);
> +		writel_relaxed(reg, priv->base + TITSR1);
> +	}
> +	mutex_unlock(&priv->irqc_mutex);
> +
> +	return 0;
> +}
> +
> +static void rzg2l_irqc_tint_irq_handler(struct irq_desc *desc)
> +{
> +	unsigned int irq = irq_desc_get_irq(desc);
> +	struct irq_chip *chip = irq_desc_get_chip(desc);
> +	struct rzg2l_irqc_chip_data *chip_data = irq_get_handler_data(irq);
> +	struct rzg2l_irqc_priv *priv = chip_data->priv;
> +	unsigned int offset;
> +	u32 reg;
> +
> +	chained_irq_enter(chip, desc);
> +	offset = chip_data->hwirq - IRQC_TINT_START;
> +	generic_handle_domain_irq(priv->irq_domain, chip_data->hwirq);
> +	reg = readl_relaxed(priv->base + TSCR) & ~BIT(offset);
> +	writel_relaxed(reg, priv->base + TSCR);

What is this operation? If that's an ack, it is way too late. And
whether it is an ack or an eoi, it must be in a callback.

> +	chained_irq_exit(chip, desc);
> +}
> +
> +static void rzg2l_irqc_irq_disable(struct irq_data *d)
> +{
> +	struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
> +	struct rzg2l_irqc_priv *priv = chip_data->priv;
> +
> +	if (chip_data->tint != -EINVAL) {
> +		u32 offset = chip_data->hwirq - IRQC_TINT_START;
> +		u32 tssr_offset = TSSR_OFFSET(offset);
> +		u8 tssr_index = TSSR_INDEX(offset);
> +		u32 reg;
> +
> +		irq_set_chained_handler_and_data(priv->irq[chip_data->hwirq]->start,
> +						 NULL, NULL);
> +
> +		mutex_lock(&priv->irqc_mutex);
> +		reg = readl_relaxed(priv->base + TSSR(tssr_index));
> +		reg &= ~(TSSEL_MASK << tssr_offset);
> +		writel_relaxed(reg, priv->base + TSSR(tssr_index));
> +		mutex_unlock(&priv->irqc_mutex);
> +	}
> +}
> +
> +static void rzg2l_irqc_irq_enable(struct irq_data *d)
> +{
> +	struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
> +	struct rzg2l_irqc_priv *priv = chip_data->priv;
> +
> +	if (chip_data->tint != -EINVAL) {
> +		u32 offset = chip_data->hwirq - IRQC_TINT_START;
> +		u32 tssr_offset = TSSR_OFFSET(offset);
> +		u8 tssr_index = TSSR_INDEX(offset);
> +		u32 reg;
> +
> +		irq_set_chained_handler_and_data(priv->irq[chip_data->hwirq]->start,
> +						 rzg2l_irqc_tint_irq_handler,
> +						 chip_data);
> +
> +		mutex_lock(&priv->irqc_mutex);
> +		reg = readl_relaxed(priv->base + TSSR(tssr_index));
> +		reg |= (TIEN | chip_data->tint) << TSSEL_SHIFT(tssr_offset);
> +		writel_relaxed(reg, priv->base + TSSR(tssr_index));
> +		mutex_unlock(&priv->irqc_mutex);
> +	}
> +}

These two function make little sense. Why isn't the chained handler
wired once and for all?

> +
> +static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
> +	struct rzg2l_irqc_priv *priv = chip_data->priv;
> +
> +	if (chip_data->tint != EINVAL)
> +		return rzg2l_tint_set_edge(priv, chip_data->hwirq, type);

Inline this function here. There is no point in having this helper on
its own with a single call site.

> +
> +	return -EINVAL;
> +}
> +
> +static int rzg2l_irqc_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				   unsigned int nr_irqs, void *arg)
> +{
> +	struct rzg2l_irqc_priv *priv = domain->host_data;
> +	struct rzg2l_irqc_chip_data *chip_data = NULL;
> +	struct irq_fwspec parent_fwspec;
> +	struct irq_fwspec *fwspec = arg;
> +	int tint = -EINVAL;
> +	irq_hw_number_t hwirq;
> +	int irq = -EINVAL;
> +	unsigned int type;
> +	int ret;
> +
> +	chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
> +	if (!chip_data)
> +		return -ENOMEM;

Why do you need to allocate per interrupt chip-specific data?

> +
> +	ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * When alloc has landed from pinctrl driver:
> +	 * fwspec->param_count = 3
> +	 * fwspec->param[0] = tint
> +	 * fwspec->param[1] = type
> +	 * fwspec->param[2] = 1
> +	 */
> +	if (fwspec->param_count == 3 && fwspec->param[2]) {
> +		mutex_lock(&priv->tint_mutex);
> +		irq = bitmap_find_free_region(priv->tint_slot, IRQC_TINT_COUNT, get_order(1));

I think you can replace this get_order() with the result.

> +		if (irq < 0) {
> +			mutex_unlock(&priv->tint_mutex);
> +			return -ENOSPC;
> +		}
> +		mutex_unlock(&priv->tint_mutex);
> +		tint = hwirq;
> +		hwirq = irq + IRQC_TINT_START;
> +	}
> +
> +	chip_data->priv = priv;
> +	chip_data->tint = tint;
> +	chip_data->hwirq = hwirq;

Really?

> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &priv->chip, chip_data);
> +	if (ret)
> +		goto release_bitmap;
> +
> +	/* parent is GIC */
> +	parent_fwspec.fwnode = domain->parent->fwnode;
> +	parent_fwspec.param_count = 3;
> +	parent_fwspec.param[0] = 0; /* SPI */
> +	parent_fwspec.param[1] = priv->irq[hwirq]->start;
> +	parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
> +	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_fwspec);
> +	if (ret)
> +		goto release_bitmap;
> +
> +	return 0;
> +
> +release_bitmap:
> +	if (tint != -EINVAL) {
> +		mutex_lock(&priv->tint_mutex);
> +		bitmap_release_region(priv->tint_slot, irq, get_order(1));
> +		mutex_unlock(&priv->tint_mutex);
> +	}
> +	kfree(chip_data);
> +	return ret;
> +}
> +
> +static void rzg2l_irqc_domain_free(struct irq_domain *domain, unsigned int virq,
> +				   unsigned int nr_irqs)
> +{
> +	struct rzg2l_irqc_chip_data *chip_data;
> +	struct rzg2l_irqc_priv *priv;
> +	struct irq_data *d;
> +
> +	d = irq_domain_get_irq_data(domain, virq);
> +	if (d) {
> +		chip_data = d->chip_data;
> +		priv = chip_data->priv;
> +		if (chip_data) {
> +			if (chip_data->tint != -EINVAL) {
> +				mutex_lock(&priv->tint_mutex);
> +				bitmap_release_region(priv->tint_slot,
> +						      chip_data->hwirq - IRQC_TINT_START,
> +						      get_order(1));
> +				mutex_unlock(&priv->tint_mutex);
> +			}
> +			kfree(chip_data);
> +		}
> +	}
> +
> +	irq_domain_free_irqs_common(domain, virq, nr_irqs);
> +}
> +
> +static const struct irq_domain_ops rzg2l_irqc_domain_ops = {
> +	.alloc = rzg2l_irqc_domain_alloc,
> +	.free = rzg2l_irqc_domain_free,
> +	.translate = irq_domain_translate_twocell,
> +};
> +
> +static int rzg2l_irqc_probe(struct platform_device *pdev)
> +{
> +	struct irq_domain *parent_irq_domain;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *gic_node;
> +	struct rzg2l_irqc_priv *priv;
> +	unsigned int i;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, priv);
> +	priv->dev = dev;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +
> +	gic_node = of_irq_find_parent(np);
> +	if (gic_node)
> +		parent_irq_domain = irq_find_host(gic_node);
> +
> +	if (!parent_irq_domain) {
> +		dev_err(dev, "cannot find parent domain\n");
> +		ret = -ENODEV;
> +		goto out_put_node;
> +	}
> +
> +	for (i = 0; i < IRQC_NUM_IRQ; i++) {
> +		priv->irq[i] = platform_get_resource(pdev, IORESOURCE_IRQ, i);
> +		if (!priv->irq[i]) {
> +			ret = -ENOENT;
> +			dev_err(dev, "failed to get irq resource(%u)", i);
> +			goto out_put_node;
> +		}
> +	}
> +
> +	mutex_init(&priv->irqc_mutex);
> +	mutex_init(&priv->tint_mutex);
> +	pm_runtime_enable(&pdev->dev);
> +	pm_runtime_resume_and_get(&pdev->dev);
> +
> +	priv->chip.name = "rzg2l-irqc";
> +	priv->chip.irq_mask = irq_chip_mask_parent;
> +	priv->chip.irq_unmask = irq_chip_unmask_parent;
> +	priv->chip.irq_enable = rzg2l_irqc_irq_enable;
> +	priv->chip.irq_disable = rzg2l_irqc_irq_disable;
> +	priv->chip.irq_retrigger = irq_chip_retrigger_hierarchy;
> +	priv->chip.irq_set_type = rzg2l_irqc_set_type;

Move the irqchip structure out of rzg2l_irqc_priv, make it
static. There is no reason why it should be dynamically allocated and
not shared across all instances.

> +	priv->chip.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE;
> +	priv->irq_domain = irq_domain_add_hierarchy(parent_irq_domain, 0,
> +						    IRQC_NUM_IRQ, np,
> +						    &rzg2l_irqc_domain_ops,
> +						    priv);

Why a hierarchy? This isn't a hierarchical setup at all.

> +	if (!priv->irq_domain) {
> +		dev_err(dev, "cannot initialize irq domain\n");
> +		ret = -ENOMEM;
> +		pm_runtime_put(&pdev->dev);
> +		pm_runtime_disable(&pdev->dev);
> +		goto out_put_node;
> +	}
> +
> +out_put_node:
> +	of_node_put(gic_node);
> +	return ret;
> +}
> +
> +static int rzg2l_irqc_remove(struct platform_device *pdev)
> +{
> +	struct rzg2l_irqc_priv *priv = platform_get_drvdata(pdev);
> +
> +	irq_domain_remove(priv->irq_domain);
> +	pm_runtime_put(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	return 0;
> +}

No. interrupt controllers cannot be removed. Once in, they don't
leave.

> +
> +static const struct of_device_id rzg2l_irqc_dt_ids[] = {
> +	{ .compatible = "renesas,rzg2l-irqc" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, rzg2l_irqc_dt_ids);
> +
> +static struct platform_driver rzg2l_irqc_device_driver = {
> +	.probe		= rzg2l_irqc_probe,
> +	.remove		= rzg2l_irqc_remove,
> +	.driver		= {
> +		.name	= "renesas_rzg2l_irqc",
> +		.of_match_table	= rzg2l_irqc_dt_ids,
> +	}
> +};
> +
> +static int __init rzg2l_irqc_init(void)
> +{
> +	return platform_driver_register(&rzg2l_irqc_device_driver);
> +}
> +postcore_initcall(rzg2l_irqc_init);
> +
> +static void __exit rzg2l_irqc_exit(void)
> +{
> +	platform_driver_unregister(&rzg2l_irqc_device_driver);
> +}
> +module_exit(rzg2l_irqc_exit);

Please use the relevant irqchip helpers to declare an irqchip module:
IRQCHIP_PLATFORM_DRIVER_BEGIN, IRQCHIP_MATCH,
IRQCHIP_PLATFORM_DRIVER_END.

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH v2 3/4] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt
  2021-09-23 21:37   ` Linus Walleij
@ 2021-09-24 21:48     ` Lad, Prabhakar
  2021-10-05  9:56     ` Geert Uytterhoeven
  1 sibling, 0 replies; 14+ messages in thread
From: Lad, Prabhakar @ 2021-09-24 21:48 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Lad Prabhakar, Marc Zyngier, Thomas Gleixner, Geert Uytterhoeven,
	Rob Herring, Magnus Damm, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, Linux-Renesas, Biju Das

Hi Linus,

Thank you for the review.

On Thu, Sep 23, 2021 at 10:38 PM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Tue, Sep 21, 2021 at 9:30 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
>
> > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> >
> > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > used as IRQ lines at given time. Selection of pins as IRQ lines
> > is handled by IA55 (which is the IRQC block) which sits in between the
> > GPIO and GIC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Why can't you just use the hierarchical IRQ domain handling inside
> gpiolib?
>
Will do that in the next version.

> See for example drivers/gpio/gpio-ixp4xx.c for an example of how this
> is used.
>
Thank you for the pointer.

Cheers,
Prabhakar

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH v2 2/4] irqchip: Add RZ/G2L IA55 Interrupt Controller driver
  2021-09-24 19:01   ` Marc Zyngier
@ 2021-09-24 22:27     ` Lad, Prabhakar
  2021-09-25  9:31       ` Marc Zyngier
  0 siblings, 1 reply; 14+ messages in thread
From: Lad, Prabhakar @ 2021-09-24 22:27 UTC (permalink / raw)
  To: Marc Zyngier
  Cc: Lad Prabhakar, Thomas Gleixner, Geert Uytterhoeven, Rob Herring,
	Linus Walleij, Magnus Damm, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Linux-Renesas, Biju Das

Hi Marc,

Thank you for the review.

On Fri, Sep 24, 2021 at 8:01 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Tue, 21 Sep 2021 20:30:26 +0100,
> Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> >
> > Add a driver for the Renesas RZ/G2L Interrupt Controller.
> >
> > This supports external pins being used as interrupts. It supports
> > one line for NMI, 8 external pins and 32 GPIO pins (out of 123)
> > to be used as IRQ lines.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/irqchip/Kconfig             |   9 +
> >  drivers/irqchip/Makefile            |   1 +
> >  drivers/irqchip/irq-renesas-rzg2l.c | 393 ++++++++++++++++++++++++++++
> >  drivers/soc/renesas/Kconfig         |   1 +
> >  4 files changed, 404 insertions(+)
> >  create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > index 4d5924e9f766..b61dceac8628 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -236,6 +236,15 @@ config RENESAS_RZA1_IRQC
> >         Enable support for the Renesas RZ/A1 Interrupt Controller, to use up
> >         to 8 external interrupts with configurable sense select.
> >
> > +config RENESAS_RZG2L_IRQC
> > +     bool "Renesas RZ/G2L IRQC support" if COMPILE_TEST
> > +     select GENERIC_IRQ_CHIP
> > +     select IRQ_DOMAIN
> > +     select IRQ_DOMAIN_HIERARCHY
> > +     help
> > +       Enable support for the Renesas RZ/G2L Interrupt Controller for external
> > +       devices.
> > +
> >  config SL28CPLD_INTC
> >       bool "Kontron sl28cpld IRQ controller"
> >       depends on MFD_SL28CPLD=y || COMPILE_TEST
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > index f88cbf36a9d2..8017786fbdac 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_RDA_INTC)                      += irq-rda-intc.o
> >  obj-$(CONFIG_RENESAS_INTC_IRQPIN)    += irq-renesas-intc-irqpin.o
> >  obj-$(CONFIG_RENESAS_IRQC)           += irq-renesas-irqc.o
> >  obj-$(CONFIG_RENESAS_RZA1_IRQC)              += irq-renesas-rza1.o
> > +obj-$(CONFIG_RENESAS_RZG2L_IRQC)     += irq-renesas-rzg2l.o
> >  obj-$(CONFIG_VERSATILE_FPGA_IRQ)     += irq-versatile-fpga.o
> >  obj-$(CONFIG_ARCH_NSPIRE)            += irq-zevio.o
> >  obj-$(CONFIG_ARCH_VT8500)            += irq-vt8500.o
> > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> > new file mode 100644
> > index 000000000000..8057fdf6781b
> > --- /dev/null
> > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > @@ -0,0 +1,393 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Renesas RZ/G2L IRQC Driver
> > + *
> > + * Copyright (C) 2021 Renesas Electronics Corporation.
> > + */
> > +
> > +#include <linux/err.h>
> > +#include <linux/init.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqchip/chained_irq.h>
> > +#include <linux/irqdesc.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/module.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> > +
> > +#define IRQC_IRQ_START                       1
> > +#define IRQC_IRQ_COUNT                       8
> > +#define IRQC_TINT_START                      9
> > +#define IRQC_TINT_COUNT                      32
> > +#define IRQC_NUM_IRQ                 41
> > +
> > +#define ISCR                         0x10
> > +#define IITSR                                0x14
> > +#define TSCR                         0x20
> > +#define TITSR0                               0x24
> > +#define TITSR1                               0x28
> > +#define TITSR0_MAX_INT                       16
> > +#define TITSEL_WIDTH                 0x2
> > +#define TSSR(n)                              (0x30 + ((n) * 4))
> > +#define TIEN                         BIT(7)
> > +#define TSSEL_SHIFT(n)                       (8 * (n))
> > +#define TSSEL_MASK                   GENMASK(7, 0)
> > +#define IRQ_MASK                     0x3
> > +
> > +#define TSSR_OFFSET(n)                       ((n) % 4)
> > +#define TSSR_INDEX(n)                        ((n) / 4)
> > +
> > +#define TITSR_TITSEL_EDGE_RISING     0
> > +#define TITSR_TITSEL_EDGE_FALLING    1
> > +#define TITSR_TITSEL_LEVEL_HIGH              2
> > +#define TITSR_TITSEL_LEVEL_LOW               3
> > +
> > +struct rzg2l_irqc_priv {
> > +     void __iomem *base;
> > +     struct device *dev;
> > +     struct irq_chip chip;
> > +     struct irq_domain *irq_domain;
> > +     struct resource *irq[IRQC_NUM_IRQ];
> > +     struct mutex irqc_mutex;
> > +     struct mutex tint_mutex; /* Mutex to protect tint_slot bitmap */
> > +     DECLARE_BITMAP(tint_slot, IRQC_TINT_COUNT);
> > +};
> > +
> > +struct rzg2l_irqc_chip_data {
> > +     struct rzg2l_irqc_priv *priv;
> > +     int tint;
> > +     int hwirq;
> > +};
> > +
> > +static int rzg2l_tint_set_edge(struct rzg2l_irqc_priv *priv,
> > +                            unsigned int hwirq, unsigned int type)
> > +{
> > +     u32 port = hwirq - IRQC_TINT_START;
> > +     u8 sense;
> > +     u32 reg;
> > +
> > +     switch (type & IRQ_TYPE_SENSE_MASK) {
> > +     case IRQ_TYPE_EDGE_RISING:
> > +             sense = TITSR_TITSEL_EDGE_RISING;
> > +             break;
> > +
> > +     case IRQ_TYPE_EDGE_FALLING:
> > +             sense = TITSR_TITSEL_EDGE_FALLING;
> > +             break;
> > +
> > +     case IRQ_TYPE_LEVEL_HIGH:
> > +             sense = TITSR_TITSEL_LEVEL_HIGH;
> > +             break;
> > +
> > +     case IRQ_TYPE_LEVEL_LOW:
> > +             sense = TITSR_TITSEL_LEVEL_LOW;
> > +             break;
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     mutex_lock(&priv->irqc_mutex);
>
> Have you tested this code with lockdep? This cannot possibly work,
> since all the irqchip callbacks are running under a per-irq raw
> spinlock.
>
Yes I have tested with a GPIO pin being set as an interrupt.

root@smarc-rzg2l:~# cat /proc/interrupts | grep SW3
127:          1          0  11030000.pin-controller  72 Edge      SW3
root@smarc-rzg2l:~# cd /sys/kernel/debug/irq/irqs/
root@smarc-rzg2l:/sys/kernel/debug/irq/irqs# cat 127
handler:  handle_fasteoi_irq
device:   (null)
status:   0x00000002
istate:   0x00000000
ddepth:   0
wdepth:   0
dstate:   0x13400202
            IRQ_TYPE_EDGE_FALLING
            IRQD_ACTIVATED
            IRQD_IRQ_STARTED
            IRQD_SINGLE_TARGET
            IRQD_DEFAULT_TRIGGER_SET
            IRQD_HANDLE_ENFORCE_IRQCTX
node:     -1
affinity: 0-1
effectiv:
domain:  :soc:pin-controller@11030000
 hwirq:   0x48
 chip:    11030000.pin-controller
  flags:   0x1
             IRQCHIP_SET_TYPE_MASKED
 parent:
    domain:  :soc:interrupt-controller@110a0000
     hwirq:   0x9
     chip:    rzg2l-irqc
      flags:   0x14
                 IRQCHIP_MASK_ON_SUSPEND
                 IRQCHIP_SKIP_SET_WAKE
     parent:
        domain:  :soc:interrupt-controller@11900000-1
         hwirq:   0x60
         chip:    GICv3
          flags:   0x15
                     IRQCHIP_SET_TYPE_MASKED
                     IRQCHIP_MASK_ON_SUSPEND
                     IRQCHIP_SKIP_SET_WAKE
root@smarc-rzg2l:/sys/kernel/debug/irq/irqs#

> > +     if (port < TITSR0_MAX_INT) {
> > +             reg = readl_relaxed(priv->base + TITSR0);
> > +             reg &= ~(IRQ_MASK << (port * TITSEL_WIDTH));
> > +             reg |= sense << (port * TITSEL_WIDTH);
> > +             writel_relaxed(reg, priv->base + TITSR0);
> > +     } else {
> > +             port = port / TITSEL_WIDTH;
> > +             reg = readl_relaxed(priv->base + TITSR1);
> > +             reg &= ~(IRQ_MASK << (port * TITSEL_WIDTH));
> > +             reg |= sense << (port * TITSEL_WIDTH);
> > +             writel_relaxed(reg, priv->base + TITSR1);
> > +     }
> > +     mutex_unlock(&priv->irqc_mutex);
> > +
> > +     return 0;
> > +}
> > +
> > +static void rzg2l_irqc_tint_irq_handler(struct irq_desc *desc)
> > +{
> > +     unsigned int irq = irq_desc_get_irq(desc);
> > +     struct irq_chip *chip = irq_desc_get_chip(desc);
> > +     struct rzg2l_irqc_chip_data *chip_data = irq_get_handler_data(irq);
> > +     struct rzg2l_irqc_priv *priv = chip_data->priv;
> > +     unsigned int offset;
> > +     u32 reg;
> > +
> > +     chained_irq_enter(chip, desc);
> > +     offset = chip_data->hwirq - IRQC_TINT_START;
> > +     generic_handle_domain_irq(priv->irq_domain, chip_data->hwirq);
> > +     reg = readl_relaxed(priv->base + TSCR) & ~BIT(offset);
> > +     writel_relaxed(reg, priv->base + TSCR);
>
> What is this operation? If that's an ack, it is way too late. And
> whether it is an ack or an eoi, it must be in a callback.
>
Ok, I will move this to the eoi callback.

> > +     chained_irq_exit(chip, desc);
> > +}
> > +
> > +static void rzg2l_irqc_irq_disable(struct irq_data *d)
> > +{
> > +     struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
> > +     struct rzg2l_irqc_priv *priv = chip_data->priv;
> > +
> > +     if (chip_data->tint != -EINVAL) {
> > +             u32 offset = chip_data->hwirq - IRQC_TINT_START;
> > +             u32 tssr_offset = TSSR_OFFSET(offset);
> > +             u8 tssr_index = TSSR_INDEX(offset);
> > +             u32 reg;
> > +
> > +             irq_set_chained_handler_and_data(priv->irq[chip_data->hwirq]->start,
> > +                                              NULL, NULL);
> > +
> > +             mutex_lock(&priv->irqc_mutex);
> > +             reg = readl_relaxed(priv->base + TSSR(tssr_index));
> > +             reg &= ~(TSSEL_MASK << tssr_offset);
> > +             writel_relaxed(reg, priv->base + TSSR(tssr_index));
> > +             mutex_unlock(&priv->irqc_mutex);
> > +     }
> > +}
> > +
> > +static void rzg2l_irqc_irq_enable(struct irq_data *d)
> > +{
> > +     struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
> > +     struct rzg2l_irqc_priv *priv = chip_data->priv;
> > +
> > +     if (chip_data->tint != -EINVAL) {
> > +             u32 offset = chip_data->hwirq - IRQC_TINT_START;
> > +             u32 tssr_offset = TSSR_OFFSET(offset);
> > +             u8 tssr_index = TSSR_INDEX(offset);
> > +             u32 reg;
> > +
> > +             irq_set_chained_handler_and_data(priv->irq[chip_data->hwirq]->start,
> > +                                              rzg2l_irqc_tint_irq_handler,
> > +                                              chip_data);
> > +
> > +             mutex_lock(&priv->irqc_mutex);
> > +             reg = readl_relaxed(priv->base + TSSR(tssr_index));
> > +             reg |= (TIEN | chip_data->tint) << TSSEL_SHIFT(tssr_offset);
> > +             writel_relaxed(reg, priv->base + TSSR(tssr_index));
> > +             mutex_unlock(&priv->irqc_mutex);
> > +     }
> > +}
>
> These two function make little sense. Why isn't the chained handler
> wired once and for all?
>
you mean to move this during alloc callback? chained handler is
registered only when an GPIO line is requested as interrupt as a
result I am wiring only upon requests.

> > +
> > +static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +     struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
> > +     struct rzg2l_irqc_priv *priv = chip_data->priv;
> > +
> > +     if (chip_data->tint != EINVAL)
> > +             return rzg2l_tint_set_edge(priv, chip_data->hwirq, type);
>
> Inline this function here. There is no point in having this helper on
> its own with a single call site.
>
Ok, I will make this inline.

> > +
> > +     return -EINVAL;
> > +}
> > +
> > +static int rzg2l_irqc_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > +                                unsigned int nr_irqs, void *arg)
> > +{
> > +     struct rzg2l_irqc_priv *priv = domain->host_data;
> > +     struct rzg2l_irqc_chip_data *chip_data = NULL;
> > +     struct irq_fwspec parent_fwspec;
> > +     struct irq_fwspec *fwspec = arg;
> > +     int tint = -EINVAL;
> > +     irq_hw_number_t hwirq;
> > +     int irq = -EINVAL;
> > +     unsigned int type;
> > +     int ret;
> > +
> > +     chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
> > +     if (!chip_data)
> > +             return -ENOMEM;
>
> Why do you need to allocate per interrupt chip-specific data?
>
To ack and trigger the virq in the domain. Let me know if there is a
much better way of doing this.

> > +
> > +     ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * When alloc has landed from pinctrl driver:
> > +      * fwspec->param_count = 3
> > +      * fwspec->param[0] = tint
> > +      * fwspec->param[1] = type
> > +      * fwspec->param[2] = 1
> > +      */
> > +     if (fwspec->param_count == 3 && fwspec->param[2]) {
> > +             mutex_lock(&priv->tint_mutex);
> > +             irq = bitmap_find_free_region(priv->tint_slot, IRQC_TINT_COUNT, get_order(1));
>
> I think you can replace this get_order() with the result.
>
> > +             if (irq < 0) {
> > +                     mutex_unlock(&priv->tint_mutex);
> > +                     return -ENOSPC;
> > +             }
> > +             mutex_unlock(&priv->tint_mutex);
> > +             tint = hwirq;
> > +             hwirq = irq + IRQC_TINT_START;
> > +     }
> > +
> > +     chip_data->priv = priv;
> > +     chip_data->tint = tint;
> > +     chip_data->hwirq = hwirq;
>
> Really?
Could you please elaborate here.

As per the current implementation here
pinctrl->irqc->GIC
The SoC supports 123 GPIO pins out of which a max of 32 can be
configured as an interrupt line into the irqc.

keyboard {
    compatible = "gpio-keys";
    status = "okay";

   key-3 {
       gpios = <&pinctrl RZG2L_GPIO(9, 0) GPIO_ACTIVE_HIGH>;
       linux,code = <KEY_3>;
       label = "SW3";
        wakeup-source;
    };
};
For example with the above the pinctrl driver passes interrupt number
to be programmed into irqc corresponding to RZG2L_GPIO(9, 0),  type
and third parameter tint indicating this is coming from pinctrl (as
irqc supports gpio/irq/nmi). When this lands in the irqc domain with
the above we find the first available slot from 32 interrupts and
hwirq is set to  irq + IRQC_TINT_START if its first slot it would be
0+9.

>
> > +     ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &priv->chip, chip_data);
> > +     if (ret)
> > +             goto release_bitmap;
> > +
> > +     /* parent is GIC */
> > +     parent_fwspec.fwnode = domain->parent->fwnode;
> > +     parent_fwspec.param_count = 3;
> > +     parent_fwspec.param[0] = 0; /* SPI */
> > +     parent_fwspec.param[1] = priv->irq[hwirq]->start;
> > +     parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
> > +     ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_fwspec);
This is the bit which is I am unclear about as I am requesting a
chained handler I believe this should be done automatically and is not
required. Is my understanding correct here?

> > +     if (ret)
> > +             goto release_bitmap;
> > +
> > +     return 0;
> > +
> > +release_bitmap:
> > +     if (tint != -EINVAL) {
> > +             mutex_lock(&priv->tint_mutex);
> > +             bitmap_release_region(priv->tint_slot, irq, get_order(1));
> > +             mutex_unlock(&priv->tint_mutex);
> > +     }
> > +     kfree(chip_data);
> > +     return ret;
> > +}
> > +
> > +static void rzg2l_irqc_domain_free(struct irq_domain *domain, unsigned int virq,
> > +                                unsigned int nr_irqs)
> > +{
> > +     struct rzg2l_irqc_chip_data *chip_data;
> > +     struct rzg2l_irqc_priv *priv;
> > +     struct irq_data *d;
> > +
> > +     d = irq_domain_get_irq_data(domain, virq);
> > +     if (d) {
> > +             chip_data = d->chip_data;
> > +             priv = chip_data->priv;
> > +             if (chip_data) {
> > +                     if (chip_data->tint != -EINVAL) {
> > +                             mutex_lock(&priv->tint_mutex);
> > +                             bitmap_release_region(priv->tint_slot,
> > +                                                   chip_data->hwirq - IRQC_TINT_START,
> > +                                                   get_order(1));
> > +                             mutex_unlock(&priv->tint_mutex);
> > +                     }
> > +                     kfree(chip_data);
> > +             }
> > +     }
> > +
> > +     irq_domain_free_irqs_common(domain, virq, nr_irqs);
> > +}
> > +
> > +static const struct irq_domain_ops rzg2l_irqc_domain_ops = {
> > +     .alloc = rzg2l_irqc_domain_alloc,
> > +     .free = rzg2l_irqc_domain_free,
> > +     .translate = irq_domain_translate_twocell,
> > +};
> > +
> > +static int rzg2l_irqc_probe(struct platform_device *pdev)
> > +{
> > +     struct irq_domain *parent_irq_domain;
> > +     struct device *dev = &pdev->dev;
> > +     struct device_node *np = dev->of_node;
> > +     struct device_node *gic_node;
> > +     struct rzg2l_irqc_priv *priv;
> > +     unsigned int i;
> > +     int ret;
> > +
> > +     priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +     if (!priv)
> > +             return -ENOMEM;
> > +
> > +     platform_set_drvdata(pdev, priv);
> > +     priv->dev = dev;
> > +
> > +     priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR(priv->base))
> > +             return PTR_ERR(priv->base);
> > +
> > +     gic_node = of_irq_find_parent(np);
> > +     if (gic_node)
> > +             parent_irq_domain = irq_find_host(gic_node);
> > +
> > +     if (!parent_irq_domain) {
> > +             dev_err(dev, "cannot find parent domain\n");
> > +             ret = -ENODEV;
> > +             goto out_put_node;
> > +     }
> > +
> > +     for (i = 0; i < IRQC_NUM_IRQ; i++) {
> > +             priv->irq[i] = platform_get_resource(pdev, IORESOURCE_IRQ, i);
> > +             if (!priv->irq[i]) {
> > +                     ret = -ENOENT;
> > +                     dev_err(dev, "failed to get irq resource(%u)", i);
> > +                     goto out_put_node;
> > +             }
> > +     }
> > +
> > +     mutex_init(&priv->irqc_mutex);
> > +     mutex_init(&priv->tint_mutex);
> > +     pm_runtime_enable(&pdev->dev);
> > +     pm_runtime_resume_and_get(&pdev->dev);
> > +
> > +     priv->chip.name = "rzg2l-irqc";
> > +     priv->chip.irq_mask = irq_chip_mask_parent;
> > +     priv->chip.irq_unmask = irq_chip_unmask_parent;
> > +     priv->chip.irq_enable = rzg2l_irqc_irq_enable;
> > +     priv->chip.irq_disable = rzg2l_irqc_irq_disable;
> > +     priv->chip.irq_retrigger = irq_chip_retrigger_hierarchy;
> > +     priv->chip.irq_set_type = rzg2l_irqc_set_type;
>
> Move the irqchip structure out of rzg2l_irqc_priv, make it
> static. There is no reason why it should be dynamically allocated and
> not shared across all instances.
>
OK will move that outside.

> > +     priv->chip.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE;
> > +     priv->irq_domain = irq_domain_add_hierarchy(parent_irq_domain, 0,
> > +                                                 IRQC_NUM_IRQ, np,
> > +                                                 &rzg2l_irqc_domain_ops,
> > +                                                 priv);
>
> Why a hierarchy? This isn't a hierarchical setup at all.
>
Chained handlers should be irq_domain_add_linear()?

> > +     if (!priv->irq_domain) {
> > +             dev_err(dev, "cannot initialize irq domain\n");
> > +             ret = -ENOMEM;
> > +             pm_runtime_put(&pdev->dev);
> > +             pm_runtime_disable(&pdev->dev);
> > +             goto out_put_node;
> > +     }
> > +
> > +out_put_node:
> > +     of_node_put(gic_node);
> > +     return ret;
> > +}
> > +
> > +static int rzg2l_irqc_remove(struct platform_device *pdev)
> > +{
> > +     struct rzg2l_irqc_priv *priv = platform_get_drvdata(pdev);
> > +
> > +     irq_domain_remove(priv->irq_domain);
> > +     pm_runtime_put(&pdev->dev);
> > +     pm_runtime_disable(&pdev->dev);
> > +     return 0;
> > +}
>
> No. interrupt controllers cannot be removed. Once in, they don't
> leave.
>
Ok, I will drop this.

> > +
> > +static const struct of_device_id rzg2l_irqc_dt_ids[] = {
> > +     { .compatible = "renesas,rzg2l-irqc" },
> > +     {}
> > +};
> > +MODULE_DEVICE_TABLE(of, rzg2l_irqc_dt_ids);
> > +
> > +static struct platform_driver rzg2l_irqc_device_driver = {
> > +     .probe          = rzg2l_irqc_probe,
> > +     .remove         = rzg2l_irqc_remove,
> > +     .driver         = {
> > +             .name   = "renesas_rzg2l_irqc",
> > +             .of_match_table = rzg2l_irqc_dt_ids,
> > +     }
> > +};
> > +
> > +static int __init rzg2l_irqc_init(void)
> > +{
> > +     return platform_driver_register(&rzg2l_irqc_device_driver);
> > +}
> > +postcore_initcall(rzg2l_irqc_init);
> > +
> > +static void __exit rzg2l_irqc_exit(void)
> > +{
> > +     platform_driver_unregister(&rzg2l_irqc_device_driver);
> > +}
> > +module_exit(rzg2l_irqc_exit);
>
> Please use the relevant irqchip helpers to declare an irqchip module:
> IRQCHIP_PLATFORM_DRIVER_BEGIN, IRQCHIP_MATCH,
> IRQCHIP_PLATFORM_DRIVER_END.
>
I will use them in the next version.

Cheers,
Prabhakar

>         M.
>
> --
> Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH v2 2/4] irqchip: Add RZ/G2L IA55 Interrupt Controller driver
  2021-09-24 22:27     ` Lad, Prabhakar
@ 2021-09-25  9:31       ` Marc Zyngier
  0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2021-09-25  9:31 UTC (permalink / raw)
  To: Lad, Prabhakar
  Cc: Lad Prabhakar, Thomas Gleixner, Geert Uytterhoeven, Rob Herring,
	Linus Walleij, Magnus Damm, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, LKML,
	Linux-Renesas, Biju Das

On Fri, 24 Sep 2021 23:27:21 +0100,
"Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> 
> Hi Marc,
> 
> Thank you for the review.
> 
> On Fri, Sep 24, 2021 at 8:01 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Tue, 21 Sep 2021 20:30:26 +0100,
> > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > >
> > > Add a driver for the Renesas RZ/G2L Interrupt Controller.
> > >
> > > This supports external pins being used as interrupts. It supports
> > > one line for NMI, 8 external pins and 32 GPIO pins (out of 123)
> > > to be used as IRQ lines.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > >  drivers/irqchip/Kconfig             |   9 +
> > >  drivers/irqchip/Makefile            |   1 +
> > >  drivers/irqchip/irq-renesas-rzg2l.c | 393 ++++++++++++++++++++++++++++
> > >  drivers/soc/renesas/Kconfig         |   1 +
> > >  4 files changed, 404 insertions(+)
> > >  create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c
> > >
> > > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> > > index 4d5924e9f766..b61dceac8628 100644
> > > --- a/drivers/irqchip/Kconfig
> > > +++ b/drivers/irqchip/Kconfig
> > > @@ -236,6 +236,15 @@ config RENESAS_RZA1_IRQC
> > >         Enable support for the Renesas RZ/A1 Interrupt Controller, to use up
> > >         to 8 external interrupts with configurable sense select.
> > >
> > > +config RENESAS_RZG2L_IRQC
> > > +     bool "Renesas RZ/G2L IRQC support" if COMPILE_TEST
> > > +     select GENERIC_IRQ_CHIP
> > > +     select IRQ_DOMAIN
> > > +     select IRQ_DOMAIN_HIERARCHY
> > > +     help
> > > +       Enable support for the Renesas RZ/G2L Interrupt Controller for external
> > > +       devices.
> > > +
> > >  config SL28CPLD_INTC
> > >       bool "Kontron sl28cpld IRQ controller"
> > >       depends on MFD_SL28CPLD=y || COMPILE_TEST
> > > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> > > index f88cbf36a9d2..8017786fbdac 100644
> > > --- a/drivers/irqchip/Makefile
> > > +++ b/drivers/irqchip/Makefile
> > > @@ -51,6 +51,7 @@ obj-$(CONFIG_RDA_INTC)                      += irq-rda-intc.o
> > >  obj-$(CONFIG_RENESAS_INTC_IRQPIN)    += irq-renesas-intc-irqpin.o
> > >  obj-$(CONFIG_RENESAS_IRQC)           += irq-renesas-irqc.o
> > >  obj-$(CONFIG_RENESAS_RZA1_IRQC)              += irq-renesas-rza1.o
> > > +obj-$(CONFIG_RENESAS_RZG2L_IRQC)     += irq-renesas-rzg2l.o
> > >  obj-$(CONFIG_VERSATILE_FPGA_IRQ)     += irq-versatile-fpga.o
> > >  obj-$(CONFIG_ARCH_NSPIRE)            += irq-zevio.o
> > >  obj-$(CONFIG_ARCH_VT8500)            += irq-vt8500.o
> > > diff --git a/drivers/irqchip/irq-renesas-rzg2l.c b/drivers/irqchip/irq-renesas-rzg2l.c
> > > new file mode 100644
> > > index 000000000000..8057fdf6781b
> > > --- /dev/null
> > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> > > @@ -0,0 +1,393 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Renesas RZ/G2L IRQC Driver
> > > + *
> > > + * Copyright (C) 2021 Renesas Electronics Corporation.
> > > + */
> > > +
> > > +#include <linux/err.h>
> > > +#include <linux/init.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/io.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/irqchip/chained_irq.h>
> > > +#include <linux/irqdesc.h>
> > > +#include <linux/irqdomain.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of_irq.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > > +
> > > +#define IRQC_IRQ_START                       1
> > > +#define IRQC_IRQ_COUNT                       8
> > > +#define IRQC_TINT_START                      9
> > > +#define IRQC_TINT_COUNT                      32
> > > +#define IRQC_NUM_IRQ                 41
> > > +
> > > +#define ISCR                         0x10
> > > +#define IITSR                                0x14
> > > +#define TSCR                         0x20
> > > +#define TITSR0                               0x24
> > > +#define TITSR1                               0x28
> > > +#define TITSR0_MAX_INT                       16
> > > +#define TITSEL_WIDTH                 0x2
> > > +#define TSSR(n)                              (0x30 + ((n) * 4))
> > > +#define TIEN                         BIT(7)
> > > +#define TSSEL_SHIFT(n)                       (8 * (n))
> > > +#define TSSEL_MASK                   GENMASK(7, 0)
> > > +#define IRQ_MASK                     0x3
> > > +
> > > +#define TSSR_OFFSET(n)                       ((n) % 4)
> > > +#define TSSR_INDEX(n)                        ((n) / 4)
> > > +
> > > +#define TITSR_TITSEL_EDGE_RISING     0
> > > +#define TITSR_TITSEL_EDGE_FALLING    1
> > > +#define TITSR_TITSEL_LEVEL_HIGH              2
> > > +#define TITSR_TITSEL_LEVEL_LOW               3
> > > +
> > > +struct rzg2l_irqc_priv {
> > > +     void __iomem *base;
> > > +     struct device *dev;
> > > +     struct irq_chip chip;
> > > +     struct irq_domain *irq_domain;
> > > +     struct resource *irq[IRQC_NUM_IRQ];
> > > +     struct mutex irqc_mutex;
> > > +     struct mutex tint_mutex; /* Mutex to protect tint_slot bitmap */
> > > +     DECLARE_BITMAP(tint_slot, IRQC_TINT_COUNT);
> > > +};
> > > +
> > > +struct rzg2l_irqc_chip_data {
> > > +     struct rzg2l_irqc_priv *priv;
> > > +     int tint;
> > > +     int hwirq;
> > > +};
> > > +
> > > +static int rzg2l_tint_set_edge(struct rzg2l_irqc_priv *priv,
> > > +                            unsigned int hwirq, unsigned int type)
> > > +{
> > > +     u32 port = hwirq - IRQC_TINT_START;
> > > +     u8 sense;
> > > +     u32 reg;
> > > +
> > > +     switch (type & IRQ_TYPE_SENSE_MASK) {
> > > +     case IRQ_TYPE_EDGE_RISING:
> > > +             sense = TITSR_TITSEL_EDGE_RISING;
> > > +             break;
> > > +
> > > +     case IRQ_TYPE_EDGE_FALLING:
> > > +             sense = TITSR_TITSEL_EDGE_FALLING;
> > > +             break;
> > > +
> > > +     case IRQ_TYPE_LEVEL_HIGH:
> > > +             sense = TITSR_TITSEL_LEVEL_HIGH;
> > > +             break;
> > > +
> > > +     case IRQ_TYPE_LEVEL_LOW:
> > > +             sense = TITSR_TITSEL_LEVEL_LOW;
> > > +             break;
> > > +
> > > +     default:
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     mutex_lock(&priv->irqc_mutex);
> >
> > Have you tested this code with lockdep? This cannot possibly work,
> > since all the irqchip callbacks are running under a per-irq raw
> > spinlock.
> >
> Yes I have tested with a GPIO pin being set as an interrupt.

[...]

You seem to have missed the key element above. *lockdep* is the
crucial part (and more specifically CONFIG_PROVE_LOCKING). My bet is
that you will get a huge splat telling you that your locking is bogus.

> > > +static void rzg2l_irqc_irq_enable(struct irq_data *d)
> > > +{
> > > +     struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
> > > +     struct rzg2l_irqc_priv *priv = chip_data->priv;
> > > +
> > > +     if (chip_data->tint != -EINVAL) {
> > > +             u32 offset = chip_data->hwirq - IRQC_TINT_START;
> > > +             u32 tssr_offset = TSSR_OFFSET(offset);
> > > +             u8 tssr_index = TSSR_INDEX(offset);
> > > +             u32 reg;
> > > +
> > > +             irq_set_chained_handler_and_data(priv->irq[chip_data->hwirq]->start,
> > > +                                              rzg2l_irqc_tint_irq_handler,
> > > +                                              chip_data);
> > > +
> > > +             mutex_lock(&priv->irqc_mutex);
> > > +             reg = readl_relaxed(priv->base + TSSR(tssr_index));
> > > +             reg |= (TIEN | chip_data->tint) << TSSEL_SHIFT(tssr_offset);
> > > +             writel_relaxed(reg, priv->base + TSSR(tssr_index));
> > > +             mutex_unlock(&priv->irqc_mutex);
> > > +     }
> > > +}
> >
> > These two function make little sense. Why isn't the chained handler
> > wired once and for all?
> >
> you mean to move this during alloc callback? chained handler is
> registered only when an GPIO line is requested as interrupt as a
> result I am wiring only upon requests.

Why? Wiring a chained handler can be done as soon as the output
interrupt is known (which is probe time). There is no need to do that
on demand. However, this is the wrong model anyway, see below.

> 
> > > +
> > > +static int rzg2l_irqc_set_type(struct irq_data *d, unsigned int type)
> > > +{
> > > +     struct rzg2l_irqc_chip_data *chip_data = d->chip_data;
> > > +     struct rzg2l_irqc_priv *priv = chip_data->priv;
> > > +
> > > +     if (chip_data->tint != EINVAL)
> > > +             return rzg2l_tint_set_edge(priv, chip_data->hwirq, type);
> >
> > Inline this function here. There is no point in having this helper on
> > its own with a single call site.
> >
> Ok, I will make this inline.
> 
> > > +
> > > +     return -EINVAL;
> > > +}
> > > +
> > > +static int rzg2l_irqc_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > > +                                unsigned int nr_irqs, void *arg)
> > > +{
> > > +     struct rzg2l_irqc_priv *priv = domain->host_data;
> > > +     struct rzg2l_irqc_chip_data *chip_data = NULL;
> > > +     struct irq_fwspec parent_fwspec;
> > > +     struct irq_fwspec *fwspec = arg;
> > > +     int tint = -EINVAL;
> > > +     irq_hw_number_t hwirq;
> > > +     int irq = -EINVAL;
> > > +     unsigned int type;
> > > +     int ret;
> > > +
> > > +     chip_data = kzalloc(sizeof(*chip_data), GFP_KERNEL);
> > > +     if (!chip_data)
> > > +             return -ENOMEM;
> >
> > Why do you need to allocate per interrupt chip-specific data?
> >
> To ack and trigger the virq in the domain. Let me know if there is a
> much better way of doing this.

This makes zero sense. The data structure associated with the irq_desc
should already give you everything you need.

And what is this 'virq'? If you are talking about the hwirq of the
triggering interrupt, you totally have the wrong end of the stick. A
chained interrupt controller shouldn't need to know that.

It really looks like you are abusing a chained interrupt controller,
while this really should be a hierarchical controller which maps a set
of input pins to a set of output pins, as you never seem to map
multiple inputs to a single output.

A hierarchical interrupt controller would give you the correct context
by construction, without having to add any extra data structure.

> 
> > > +
> > > +     ret = irq_domain_translate_twocell(domain, arg, &hwirq, &type);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     /*
> > > +      * When alloc has landed from pinctrl driver:
> > > +      * fwspec->param_count = 3
> > > +      * fwspec->param[0] = tint
> > > +      * fwspec->param[1] = type
> > > +      * fwspec->param[2] = 1
> > > +      */
> > > +     if (fwspec->param_count == 3 && fwspec->param[2]) {
> > > +             mutex_lock(&priv->tint_mutex);
> > > +             irq = bitmap_find_free_region(priv->tint_slot, IRQC_TINT_COUNT, get_order(1));
> >
> > I think you can replace this get_order() with the result.
> >
> > > +             if (irq < 0) {
> > > +                     mutex_unlock(&priv->tint_mutex);
> > > +                     return -ENOSPC;
> > > +             }
> > > +             mutex_unlock(&priv->tint_mutex);
> > > +             tint = hwirq;
> > > +             hwirq = irq + IRQC_TINT_START;
> > > +     }
> > > +
> > > +     chip_data->priv = priv;
> > > +     chip_data->tint = tint;
> > > +     chip_data->hwirq = hwirq;
> >
> > Really?
> Could you please elaborate here.
> 
> As per the current implementation here
> pinctrl->irqc->GIC
> The SoC supports 123 GPIO pins out of which a max of 32 can be
> configured as an interrupt line into the irqc.
> 
> keyboard {
>     compatible = "gpio-keys";
>     status = "okay";
> 
>    key-3 {
>        gpios = <&pinctrl RZG2L_GPIO(9, 0) GPIO_ACTIVE_HIGH>;
>        linux,code = <KEY_3>;
>        label = "SW3";
>         wakeup-source;
>     };
> };
> For example with the above the pinctrl driver passes interrupt number
> to be programmed into irqc corresponding to RZG2L_GPIO(9, 0),  type
> and third parameter tint indicating this is coming from pinctrl (as
> irqc supports gpio/irq/nmi). When this lands in the irqc domain with
> the above we find the first available slot from 32 interrupts and
> hwirq is set to  irq + IRQC_TINT_START if its first slot it would be
> 0+9.

Which proves my point. This isn't a chained interrupt controller *at
all*. this is just a mapper between 8 (out of 32) inputs to 8 outputs.

> 
> >
> > > +     ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq, &priv->chip, chip_data);
> > > +     if (ret)
> > > +             goto release_bitmap;
> > > +
> > > +     /* parent is GIC */
> > > +     parent_fwspec.fwnode = domain->parent->fwnode;
> > > +     parent_fwspec.param_count = 3;
> > > +     parent_fwspec.param[0] = 0; /* SPI */
> > > +     parent_fwspec.param[1] = priv->irq[hwirq]->start;
> > > +     parent_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH;
> > > +     ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &parent_fwspec);
> This is the bit which is I am unclear about as I am requesting a
> chained handler I believe this should be done automatically and is not
> required. Is my understanding correct here?

Because you are mixing chained and hierarchical models. I even missed
the use of irq_domain_alloc_irqs_parent here, as the whole logic is
completely messed up.

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH v2 1/4] dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller
  2021-09-21 19:30 ` [RFC PATCH v2 1/4] dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller Lad Prabhakar
  2021-09-23 21:42   ` Linus Walleij
@ 2021-09-27 17:08   ` Rob Herring
  1 sibling, 0 replies; 14+ messages in thread
From: Rob Herring @ 2021-09-27 17:08 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: devicetree, Prabhakar, Linus Walleij, Magnus Damm,
	Thomas Gleixner, Rob Herring, Biju Das, linux-gpio,
	Geert Uytterhoeven, linux-kernel, linux-renesas-soc,
	Marc Zyngier

On Tue, 21 Sep 2021 20:30:25 +0100, Lad Prabhakar wrote:
> Add DT bindings for the Renesas RZ/G2L Interrupt Controller.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  .../renesas,rzg2l-irqc.yaml                   | 130 ++++++++++++++++++
>  1 file changed, 130 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rzg2l-irqc.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH v2 3/4] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt
  2021-09-23 21:37   ` Linus Walleij
  2021-09-24 21:48     ` Lad, Prabhakar
@ 2021-10-05  9:56     ` Geert Uytterhoeven
  2021-10-13  0:07       ` Linus Walleij
  1 sibling, 1 reply; 14+ messages in thread
From: Geert Uytterhoeven @ 2021-10-05  9:56 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Lad Prabhakar, Marc Zyngier, Thomas Gleixner, Geert Uytterhoeven,
	Rob Herring, Magnus Damm, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, Linux-Renesas, Prabhakar, Biju Das

Hi Linus,

On Thu, Sep 23, 2021 at 11:38 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Sep 21, 2021 at 9:30 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > Add IRQ domian to RZ/G2L pinctrl driver to handle GPIO interrupt.
> >
> > GPIO0-GPIO122 pins can be used as IRQ lines but only 32 pins can be
> > used as IRQ lines at given time. Selection of pins as IRQ lines
> > is handled by IA55 (which is the IRQC block) which sits in between the
> > GPIO and GIC.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Why can't you just use the hierarchical IRQ domain handling inside
> gpiolib?

Out of interest (not related to this patch), does this support multiple
parent domains?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [RFC PATCH v2 3/4] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt
  2021-10-05  9:56     ` Geert Uytterhoeven
@ 2021-10-13  0:07       ` Linus Walleij
  0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2021-10-13  0:07 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Lad Prabhakar, Marc Zyngier, Thomas Gleixner, Geert Uytterhoeven,
	Rob Herring, Magnus Damm, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, Linux-Renesas, Prabhakar, Biju Das

On Tue, Oct 5, 2021 at 11:56 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> > Why can't you just use the hierarchical IRQ domain handling inside
> > gpiolib?
>
> Out of interest (not related to this patch), does this support multiple
> parent domains?

Not currently, but I might have seen a patch adding it?
Now I can't find it...

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2021-10-13  0:07 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-21 19:30 [RFC PATCH v2 0/4] Renesas RZ/G2L IRQC support Lad Prabhakar
2021-09-21 19:30 ` [RFC PATCH v2 1/4] dt-bindings: interrupt-controller: Add Renesas RZ/G2L Interrupt Controller Lad Prabhakar
2021-09-23 21:42   ` Linus Walleij
2021-09-27 17:08   ` Rob Herring
2021-09-21 19:30 ` [RFC PATCH v2 2/4] irqchip: Add RZ/G2L IA55 Interrupt Controller driver Lad Prabhakar
2021-09-24 19:01   ` Marc Zyngier
2021-09-24 22:27     ` Lad, Prabhakar
2021-09-25  9:31       ` Marc Zyngier
2021-09-21 19:30 ` [RFC PATCH v2 3/4] pinctrl: renesas: pinctrl-rzg2l: Add IRQ domain to handle GPIO interrupt Lad Prabhakar
2021-09-23 21:37   ` Linus Walleij
2021-09-24 21:48     ` Lad, Prabhakar
2021-10-05  9:56     ` Geert Uytterhoeven
2021-10-13  0:07       ` Linus Walleij
2021-09-21 19:30 ` [RFC PATCH v2 4/4] arm64: dts: renesas: r9a07g044: Add IRQC node to SoC DTSI Lad Prabhakar

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.