[PATCH 0/5] ARM: rskrza1: Add RZ/A1 IRQC and input switches

[PATCH 0/5] ARM: rskrza1: Add RZ/A1 IRQC and input switches

[Geert Uytterhoeven]

	Hi all,

Unlike on most other Renesas SoCs, the GPIO controller block on RZ/A1
SoCs lack interrupt functionality.  While the GPIOs can be routed to the
GIC as pin interrupts, this is of limited use, as the PL390 GIC supports
rising edge and high-level interrupts only.

Fortunately RZ/A1 SoCs contain a small front-end for the GIC, allowing
to use up to 8 external interrupts, with configurable sense select.

Hence this patch series adds DT bindings and a driver for this
front-end, adds a device node for it in the RZ/A1H DTS, and uses it to
enable support for the 3 input switches on the Renesas RSK+RZA1
development board.

I expect this driver to be reusable for RZ/A2, after adding a match
entry with .gic_spi_base = 4.
  - Should this information come from DT instead?
  - Originally I had interrupts properties in DT to define the mapping
    from external interrupts to GIC interrupts (cfr. "renesas,irqc",
    which also calls request_irq()), but other similar drivers seem to
    hardcode this information in the driver, so I went that route.

Dependencies:
  - Patch 3 depends on patch 2,
  - Patch 4 can be applied as soon as the DT bindings in patch 1 have
    been accepted,
  - Patch 5 depends on patch 4.

This has been tested on RSK+RZA1 with evtest and s2ram wake-up.
I have verified proper operation of low-level and rising/falling sense
select, too.

Thanks for your comments!

Geert Uytterhoeven (5):
  dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt
    Controller
  irqchip: Add Renesas RZ/A1 Interrupt Controller driver
  soc: renesas: Enable RZ/A1 IRQC on RZ/A1H
  ARM: dts: r7s72100: Add IRQC device node
  ARM: dts: rskrza1: Add input switches

 .../renesas,rza1-irqc.txt                     |  27 ++
 arch/arm/boot/dts/r7s72100-rskrza1.dts        |  38 +++
 arch/arm/boot/dts/r7s72100.dtsi               |   8 +
 drivers/irqchip/Kconfig                       |   4 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-renesas-rza1.c            | 238 ++++++++++++++++++
 drivers/soc/renesas/Kconfig                   |   1 +
 7 files changed, 317 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
 create mode 100644 drivers/irqchip/irq-renesas-rza1.c

-- 
2.17.1

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

[PATCH 1/5] dt-bindings: interrupt-controller: Add Renesas RZ/A1 Interrupt Controller

[Geert Uytterhoeven]

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

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 .../renesas,rza1-irqc.txt                     | 27 +++++++++++++++++++
 1 file changed, 27 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
new file mode 100644
index 0000000000000000..0914d3d216c3bdac
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/renesas,rza1-irqc.txt
@@ -0,0 +1,27 @@
+DT bindings for the Renesas RZ/A1 Interrupt Controller
+
+The RZ/A1 Interrupt Controller is a front-end for the GIC found on Renesas
+RZ/A1 SoCs:
+  - IRQ sense select for 8 external interrupts, 1:1-mapped to 8 GIC SPI
+    interrupts,
+  - NMI edge select.
+
+Required properties:
+  - compatible: Must be "renesas,<soctype>-irqc", and "renesas,rza1-irqc" as
+		fallback.
+		Examples with soctypes are:
+		  - "renesas,r7s72100-irqc" (RZ/A1H)
+  - #interrupt-cells: Must be 2 (an interrupt index and flags, as defined
+				 in interrupts.txt in this directory)
+  - interrupt-controller: Marks the device as an interrupt controller
+  - reg: Base address and length of the memory resource used by the interrupt
+         controller
+
+Example:
+
+	irqc: interrupt-controller@fcfef800 {
+		compatible = "renesas,r7s72100-irqc", "renesas,rza1-irqc";
+		#interrupt-cells = <2>;
+		interrupt-controller;
+		reg = <0xfcfef800 0x6>;
+	};
-- 
2.17.1

[PATCH 2/5] irqchip: Add Renesas RZ/A1 Interrupt Controller driver

[Geert Uytterhoeven]

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

This supports using up to 8 external interrupts on RZ/A1, with
configurable sense select.

NMI edge select is not yet supported.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/irqchip/Kconfig            |   4 +
 drivers/irqchip/Makefile           |   1 +
 drivers/irqchip/irq-renesas-rza1.c | 238 +++++++++++++++++++++++++++++
 3 files changed, 243 insertions(+)
 create mode 100644 drivers/irqchip/irq-renesas-rza1.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 5438abb1babaf042..cd5b9bbf6122afce 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -221,6 +221,10 @@ config RENESAS_IRQC
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 
+config RENESAS_RZA1_IRQC
+	bool
+	select IRQ_DOMAIN_HIERARCHY
+
 config ST_IRQCHIP
 	bool
 	select REGMAP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 85972ae1bd7f978b..c0e23ce9f2619de3 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -48,6 +48,7 @@ obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
 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_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-rza1.c b/drivers/irqchip/irq-renesas-rza1.c
new file mode 100644
index 0000000000000000..65110019adda5a8f
--- /dev/null
+++ b/drivers/irqchip/irq-renesas-rza1.c
@@ -0,0 +1,238 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Renesas RZ/A1 IRQC Driver
+ *
+ * Copyright (C) 2019 Glider bvba
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irqdomain.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <dt-bindings/interrupt-controller/arm-gic.h>
+
+#define IRQC_NUM_IRQ		8
+
+#define ICR0			0	/* Interrupt Control Register 0 */
+
+#define ICR0_NMIL		BIT(15)	/* NMI Input Level (0=low, 1=high) */
+#define ICR0_NMIE		BIT(8)	/* Edge Select (0=falling, 1=rising) */
+#define ICR0_NMIF		BIT(1)	/* NMI Interrupt Request */
+
+#define ICR1			2	/* Interrupt Control Register 1 */
+
+#define ICR1_IRQS(n, sense)	((sense) << ((n) * 2))	/* IRQ Sense Select */
+#define ICR1_IRQS_LEVEL_LOW	0
+#define ICR1_IRQS_EDGE_FALLING	1
+#define ICR1_IRQS_EDGE_RISING	2
+#define ICR1_IRQS_EDGE_BOTH	3
+#define ICR1_IRQS_MASK(n)	ICR1_IRQS((n), 3)
+
+#define IRQRR			4	/* IRQ Interrupt Request Register */
+
+
+struct rza1_irqc_info {
+	unsigned int gic_spi_base;
+};
+
+struct rza1_irqc_priv {
+	struct device *dev;
+	void __iomem *base;
+	struct irq_chip chip;
+	struct irq_domain *irq_domain;
+	unsigned int gic_spi_base;
+};
+
+static struct rza1_irqc_priv *irq_data_to_priv(struct irq_data *data)
+{
+	return data->domain->host_data;
+}
+
+static void rza1_irqc_eoi(struct irq_data *d)
+{
+	struct rza1_irqc_priv *priv = irq_data_to_priv(d);
+	unsigned int bit = BIT(irqd_to_hwirq(d));
+	u16 tmp;
+
+	tmp = readw(priv->base + IRQRR);
+	if (tmp & bit)
+		writew(GENMASK(IRQC_NUM_IRQ - 1, 0) & ~bit, priv->base + IRQRR);
+
+	irq_chip_eoi_parent(d);
+}
+
+static int rza1_irqc_set_type(struct irq_data *d, unsigned int type)
+{
+	struct rza1_irqc_priv *priv = irq_data_to_priv(d);
+	unsigned int hw_irq = irqd_to_hwirq(d);
+	u16 sense, tmp;
+
+	switch (type & IRQ_TYPE_SENSE_MASK) {
+	case IRQ_TYPE_LEVEL_LOW:
+		sense = ICR1_IRQS_LEVEL_LOW;
+		break;
+
+	case IRQ_TYPE_EDGE_FALLING:
+		sense = ICR1_IRQS_EDGE_FALLING;
+		break;
+
+	case IRQ_TYPE_EDGE_RISING:
+		sense = ICR1_IRQS_EDGE_RISING;
+		break;
+
+	case IRQ_TYPE_EDGE_BOTH:
+		sense = ICR1_IRQS_EDGE_BOTH;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	tmp = readw(priv->base + ICR1);
+	tmp &= ~ICR1_IRQS_MASK(hw_irq);
+	tmp |= ICR1_IRQS(hw_irq, sense);
+	writew(tmp, priv->base + ICR1);
+	return 0;
+}
+
+static int rza1_irqc_alloc(struct irq_domain *domain, unsigned int virq,
+			   unsigned int nr_irqs, void *arg)
+{
+	struct rza1_irqc_priv *priv = domain->host_data;
+	struct irq_fwspec *fwspec = arg;
+	struct irq_fwspec spec;
+	int ret;
+
+	ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
+					    &priv->chip, priv);
+	if (ret)
+		return ret;
+
+	spec.fwnode = &priv->dev->of_node->fwnode;
+	spec.param_count = 3;
+	spec.param[0] = GIC_SPI;
+	spec.param[1] = priv->gic_spi_base + fwspec->param[0];
+	spec.param[2] = IRQ_TYPE_LEVEL_HIGH;
+
+	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &spec);
+}
+
+static int rza1_irqc_translate(struct irq_domain *domain,
+			       struct irq_fwspec *fwspec, unsigned long *hwirq,
+			       unsigned int *type)
+{
+	if (fwspec->param_count != 2 || fwspec->param[0] >= IRQC_NUM_IRQ)
+		return -EINVAL;
+
+	*hwirq = fwspec->param[0];
+	*type = fwspec->param[1];
+	return 0;
+}
+
+static const struct irq_domain_ops rza1_irqc_domain_ops = {
+	.alloc = rza1_irqc_alloc,
+	.translate = rza1_irqc_translate,
+};
+
+static int rza1_irqc_probe(struct platform_device *pdev)
+{
+	const struct rza1_irqc_info *info;
+	struct irq_domain *parent = NULL;
+	struct device *dev = &pdev->dev;
+	struct device_node *gic_node;
+	struct rza1_irqc_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	info = of_device_get_match_data(dev);
+	priv->gic_spi_base = info->gic_spi_base;
+	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(dev->of_node);
+	if (gic_node) {
+		parent = irq_find_host(gic_node);
+		of_node_put(gic_node);
+	}
+
+	if (!parent) {
+		dev_err(dev, "cannot find parent domain\n");
+		return -ENODEV;
+	}
+
+	priv->chip.name = dev_name(dev);
+	priv->chip.irq_mask = irq_chip_mask_parent,
+	priv->chip.irq_unmask = irq_chip_unmask_parent,
+	priv->chip.irq_eoi = rza1_irqc_eoi,
+	priv->chip.irq_retrigger = irq_chip_retrigger_hierarchy,
+	priv->chip.irq_set_type = rza1_irqc_set_type,
+	priv->chip.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE;
+
+	priv->irq_domain = irq_domain_add_hierarchy(parent, 0, IRQC_NUM_IRQ,
+						    dev->of_node,
+						    &rza1_irqc_domain_ops,
+						    priv);
+	if (!priv->irq_domain) {
+		dev_err(dev, "cannot initialize irq domain\n");
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int rza1_irqc_remove(struct platform_device *pdev)
+{
+	struct rza1_irqc_priv *priv = platform_get_drvdata(pdev);
+
+	irq_domain_remove(priv->irq_domain);
+	return 0;
+}
+
+struct rza1_irqc_info rza1_irqc_info = {
+	.gic_spi_base = 0,
+};
+
+static const struct of_device_id rza1_irqc_dt_ids[] = {
+	{ .compatible = "renesas,rza1-irqc", &rza1_irqc_info },
+	{},
+};
+MODULE_DEVICE_TABLE(of, rza1_irqc_dt_ids);
+
+static struct platform_driver rza1_irqc_device_driver = {
+	.probe		= rza1_irqc_probe,
+	.remove		= rza1_irqc_remove,
+	.driver		= {
+		.name	= "renesas_rza1_irqc",
+		.of_match_table	= rza1_irqc_dt_ids,
+	}
+};
+
+static int __init rza1_irqc_init(void)
+{
+	return platform_driver_register(&rza1_irqc_device_driver);
+}
+postcore_initcall(rza1_irqc_init);
+
+static void __exit rza1_irqc_exit(void)
+{
+	platform_driver_unregister(&rza1_irqc_device_driver);
+}
+module_exit(rza1_irqc_exit);
+
+MODULE_AUTHOR("Geert Uytterhoeven <geert+renesas@glider.be>");
+MODULE_DESCRIPTION("Renesas RZ/A1 IRQC Driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH 3/5] soc: renesas: Enable RZ/A1 IRQC on RZ/A1H

[Geert Uytterhoeven]

Auto-enable the RZ/A1 Interrupt Controller when configuring a kernel
with support for RZ/A1H SoCs.

This is similar to how interrupt controllers for other Renesas SoCs are
enabled.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/soc/renesas/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig
index 68bfca6f20ddf8a7..1448b6dbcdb20bae 100644
--- a/drivers/soc/renesas/Kconfig
+++ b/drivers/soc/renesas/Kconfig
@@ -57,6 +57,7 @@ config ARCH_R7S72100
 	bool "RZ/A1H (R7S72100)"
 	select PM
 	select PM_GENERIC_DOMAINS
+	select RENESAS_RZA1_IRQC
 	select SYS_SUPPORTS_SH_MTU2
 	select RENESAS_OSTM
 
-- 
2.17.1

[PATCH 4/5] ARM: dts: r7s72100: Add IRQC device node

[Geert Uytterhoeven]

Enable support for the IRQC on RZ/A1H, which is a small front-end to the
GIC.  This allows to use up to 8 external interrupts with configurable
sense select.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r7s72100.dtsi | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index 2211f88ede2ad351..27bc7789f5437ef1 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -670,6 +670,14 @@
 			status = "disabled";
 		};
 
+		irqc: interrupt-controller@fcfef800 {
+			compatible = "renesas,r7s72100-irqc",
+				     "renesas,rza1-irqc";
+			#interrupt-cells = <2>;
+			interrupt-controller;
+			reg = <0xfcfef800 0x6>;
+		};
+
 		mtu2: timer@fcff0000 {
 			compatible = "renesas,mtu2-r7s72100", "renesas,mtu2";
 			reg = <0xfcff0000 0x400>;
-- 
2.17.1

[PATCH 5/5] ARM: dts: rskrza1: Add input switches

[Geert Uytterhoeven]

Add support for input switches SW1-3 on the Renesas RZ/A1 RSK+RZA1
development board.

Note that this uses the IRQ interrupts, as the RZ/A1 GPIO controller
does not include interrupt support.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Use rza1-irqc instead of gic.
---
 arch/arm/boot/dts/r7s72100-rskrza1.dts | 38 ++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-rskrza1.dts b/arch/arm/boot/dts/r7s72100-rskrza1.dts
index ff24301dc1be54de..99acfe4fe11aaed9 100644
--- a/arch/arm/boot/dts/r7s72100-rskrza1.dts
+++ b/arch/arm/boot/dts/r7s72100-rskrza1.dts
@@ -8,6 +8,7 @@
 /dts-v1/;
 #include "r7s72100.dtsi"
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
 #include <dt-bindings/pinctrl/r7s72100-pinctrl.h>
 
 / {
@@ -28,6 +29,37 @@
 		reg = <0x08000000 0x02000000>;
 	};
 
+	keyboard {
+		compatible = "gpio-keys";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&keyboard_pins>;
+
+		key-1 {
+			interrupt-parent = <&irqc>;
+			interrupts = <3 IRQ_TYPE_EDGE_BOTH>;
+			linux,code = <KEY_1>;
+			label = "SW1";
+			wakeup-source;
+		};
+
+		key-2 {
+			interrupt-parent = <&irqc>;
+			interrupts = <2 IRQ_TYPE_EDGE_BOTH>;
+			linux,code = <KEY_2>;
+			label = "SW2";
+			wakeup-source;
+		};
+
+		key-3 {
+			interrupt-parent = <&irqc>;
+			interrupts = <5 IRQ_TYPE_EDGE_BOTH>;
+			linux,code = <KEY_3>;
+			label = "SW3";
+			wakeup-source;
+		};
+	};
+
 	lbsc {
 		#address-cells = <1>;
 		#size-cells = <1>;
@@ -101,6 +133,12 @@
 			 <RZA1_PINMUX(1, 7, 1)>;	/* RIIC3SDA */
 	};
 
+	keyboard_pins: keyboard {
+		pinmux = <RZA1_PINMUX(1, 9, 3)>,	/* IRQ3 */
+			 <RZA1_PINMUX(1, 8, 3)>,	/* IRQ2 */
+			 <RZA1_PINMUX(1, 11, 3)>;	/* IRQ5 */
+	};
+
 	/* Serial Console */
 	scif2_pins: serial2 {
 		pinmux = <RZA1_PINMUX(3, 0, 6)>,	/* TxD2 */
-- 
2.17.1

Re: [PATCH 2/5] irqchip: Add Renesas RZ/A1 Interrupt Controller driver

[Marc Zyngier]

Hi Geert,

On 29/04/2019 10:36, Geert Uytterhoeven wrote:
> Add a driver for the Renesas RZ/A1 Interrupt Controller.
> 
> This supports using up to 8 external interrupts on RZ/A1, with
> configurable sense select.
> 
> NMI edge select is not yet supported.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/irqchip/Kconfig            |   4 +
>  drivers/irqchip/Makefile           |   1 +
>  drivers/irqchip/irq-renesas-rza1.c | 238 +++++++++++++++++++++++++++++
>  3 files changed, 243 insertions(+)
>  create mode 100644 drivers/irqchip/irq-renesas-rza1.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 5438abb1babaf042..cd5b9bbf6122afce 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -221,6 +221,10 @@ config RENESAS_IRQC
>  	select GENERIC_IRQ_CHIP
>  	select IRQ_DOMAIN
>  
> +config RENESAS_RZA1_IRQC
> +	bool
> +	select IRQ_DOMAIN_HIERARCHY
> +
>  config ST_IRQCHIP
>  	bool
>  	select REGMAP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 85972ae1bd7f978b..c0e23ce9f2619de3 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -48,6 +48,7 @@ obj-$(CONFIG_JCORE_AIC)			+= irq-jcore-aic.o
>  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_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-rza1.c b/drivers/irqchip/irq-renesas-rza1.c
> new file mode 100644
> index 0000000000000000..65110019adda5a8f
> --- /dev/null
> +++ b/drivers/irqchip/irq-renesas-rza1.c
> @@ -0,0 +1,238 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/A1 IRQC Driver
> + *
> + * Copyright (C) 2019 Glider bvba
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +#define IRQC_NUM_IRQ		8
> +
> +#define ICR0			0	/* Interrupt Control Register 0 */
> +
> +#define ICR0_NMIL		BIT(15)	/* NMI Input Level (0=low, 1=high) */
> +#define ICR0_NMIE		BIT(8)	/* Edge Select (0=falling, 1=rising) */
> +#define ICR0_NMIF		BIT(1)	/* NMI Interrupt Request */
> +
> +#define ICR1			2	/* Interrupt Control Register 1 */
> +
> +#define ICR1_IRQS(n, sense)	((sense) << ((n) * 2))	/* IRQ Sense Select */
> +#define ICR1_IRQS_LEVEL_LOW	0
> +#define ICR1_IRQS_EDGE_FALLING	1
> +#define ICR1_IRQS_EDGE_RISING	2
> +#define ICR1_IRQS_EDGE_BOTH	3
> +#define ICR1_IRQS_MASK(n)	ICR1_IRQS((n), 3)
> +
> +#define IRQRR			4	/* IRQ Interrupt Request Register */
> +
> +
> +struct rza1_irqc_info {
> +	unsigned int gic_spi_base;
> +};
> +
> +struct rza1_irqc_priv {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct irq_chip chip;
> +	struct irq_domain *irq_domain;
> +	unsigned int gic_spi_base;
> +};
> +
> +static struct rza1_irqc_priv *irq_data_to_priv(struct irq_data *data)
> +{
> +	return data->domain->host_data;
> +}
> +
> +static void rza1_irqc_eoi(struct irq_data *d)
> +{
> +	struct rza1_irqc_priv *priv = irq_data_to_priv(d);
> +	unsigned int bit = BIT(irqd_to_hwirq(d));

Please use u32 instead of "unsigned int" for something that operates on
HW registers.

> +	u16 tmp;
> +
> +	tmp = readw(priv->base + IRQRR);

Same thing here. It's less confusing to use a u32 and mask out the top
bits if needed rather than having this implicit cast (applies all over
the code).

> +	if (tmp & bit)
> +		writew(GENMASK(IRQC_NUM_IRQ - 1, 0) & ~bit, priv->base + IRQRR);

Please use the _relaxed accessors all over the driver, you really do not
need a DSB on each of these accesses.

> +
> +	irq_chip_eoi_parent(d);
> +}
> +
> +static int rza1_irqc_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct rza1_irqc_priv *priv = irq_data_to_priv(d);
> +	unsigned int hw_irq = irqd_to_hwirq(d);
> +	u16 sense, tmp;
> +
> +	switch (type & IRQ_TYPE_SENSE_MASK) {
> +	case IRQ_TYPE_LEVEL_LOW:
> +		sense = ICR1_IRQS_LEVEL_LOW;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_FALLING:
> +		sense = ICR1_IRQS_EDGE_FALLING;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_RISING:
> +		sense = ICR1_IRQS_EDGE_RISING;
> +		break;
> +
> +	case IRQ_TYPE_EDGE_BOTH:
> +		sense = ICR1_IRQS_EDGE_BOTH;
> +		break;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	tmp = readw(priv->base + ICR1);
> +	tmp &= ~ICR1_IRQS_MASK(hw_irq);
> +	tmp |= ICR1_IRQS(hw_irq, sense);
> +	writew(tmp, priv->base + ICR1);
> +	return 0;

Don't you need to propagate the trigger configuration to the parent irqchip?

> +}
> +
> +static int rza1_irqc_alloc(struct irq_domain *domain, unsigned int virq,
> +			   unsigned int nr_irqs, void *arg)
> +{
> +	struct rza1_irqc_priv *priv = domain->host_data;
> +	struct irq_fwspec *fwspec = arg;
> +	struct irq_fwspec spec;
> +	int ret;
> +
> +	ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
> +					    &priv->chip, priv);
> +	if (ret)
> +		return ret;
> +
> +	spec.fwnode = &priv->dev->of_node->fwnode;
> +	spec.param_count = 3;
> +	spec.param[0] = GIC_SPI;
> +	spec.param[1] = priv->gic_spi_base + fwspec->param[0];
> +	spec.param[2] = IRQ_TYPE_LEVEL_HIGH;

This is related to my earlier question: Does this block turn everything
into level interrupts?

> +
> +	return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, &spec);
> +}
> +
> +static int rza1_irqc_translate(struct irq_domain *domain,
> +			       struct irq_fwspec *fwspec, unsigned long *hwirq,
> +			       unsigned int *type)
> +{
> +	if (fwspec->param_count != 2 || fwspec->param[0] >= IRQC_NUM_IRQ)
> +		return -EINVAL;
> +
> +	*hwirq = fwspec->param[0];
> +	*type = fwspec->param[1];
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops rza1_irqc_domain_ops = {
> +	.alloc = rza1_irqc_alloc,
> +	.translate = rza1_irqc_translate,
> +};
> +
> +static int rza1_irqc_probe(struct platform_device *pdev)
> +{
> +	const struct rza1_irqc_info *info;
> +	struct irq_domain *parent = NULL;
> +	struct device *dev = &pdev->dev;
> +	struct device_node *gic_node;
> +	struct rza1_irqc_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	info = of_device_get_match_data(dev);
> +	priv->gic_spi_base = info->gic_spi_base;
> +	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(dev->of_node);
> +	if (gic_node) {
> +		parent = irq_find_host(gic_node);
> +		of_node_put(gic_node);
> +	}
> +
> +	if (!parent) {
> +		dev_err(dev, "cannot find parent domain\n");
> +		return -ENODEV;
> +	}
> +
> +	priv->chip.name = dev_name(dev);

name should normally be used to identify the overall "class" of
interrupt. .device is what should be used for the device itself.

> +	priv->chip.irq_mask = irq_chip_mask_parent,
> +	priv->chip.irq_unmask = irq_chip_unmask_parent,
> +	priv->chip.irq_eoi = rza1_irqc_eoi,
> +	priv->chip.irq_retrigger = irq_chip_retrigger_hierarchy,
> +	priv->chip.irq_set_type = rza1_irqc_set_type,
> +	priv->chip.flags = IRQCHIP_MASK_ON_SUSPEND | IRQCHIP_SKIP_SET_WAKE;
> +
> +	priv->irq_domain = irq_domain_add_hierarchy(parent, 0, IRQC_NUM_IRQ,
> +						    dev->of_node,
> +						    &rza1_irqc_domain_ops,
> +						    priv);
> +	if (!priv->irq_domain) {
> +		dev_err(dev, "cannot initialize irq domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int rza1_irqc_remove(struct platform_device *pdev)
> +{
> +	struct rza1_irqc_priv *priv = platform_get_drvdata(pdev);
> +
> +	irq_domain_remove(priv->irq_domain);
> +	return 0;
> +}
> +
> +struct rza1_irqc_info rza1_irqc_info = {
> +	.gic_spi_base = 0,
> +};

To answer your question in the cover letter, I'd rather this came from
DT. And otherwise, it should be be static.

> +
> +static const struct of_device_id rza1_irqc_dt_ids[] = {
> +	{ .compatible = "renesas,rza1-irqc", &rza1_irqc_info },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, rza1_irqc_dt_ids);
> +
> +static struct platform_driver rza1_irqc_device_driver = {
> +	.probe		= rza1_irqc_probe,
> +	.remove		= rza1_irqc_remove,
> +	.driver		= {
> +		.name	= "renesas_rza1_irqc",
> +		.of_match_table	= rza1_irqc_dt_ids,
> +	}
> +};
> +
> +static int __init rza1_irqc_init(void)
> +{
> +	return platform_driver_register(&rza1_irqc_device_driver);
> +}
> +postcore_initcall(rza1_irqc_init);
> +
> +static void __exit rza1_irqc_exit(void)
> +{
> +	platform_driver_unregister(&rza1_irqc_device_driver);
> +}
> +module_exit(rza1_irqc_exit);
> +
> +MODULE_AUTHOR("Geert Uytterhoeven <geert+renesas@glider.be>");
> +MODULE_DESCRIPTION("Renesas RZ/A1 IRQC Driver");
> +MODULE_LICENSE("GPL v2");
> 

It otherwise looks good to me. If you respin it quickly enough, I'm
happy to take it for 5.2.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/5] irqchip: Add Renesas RZ/A1 Interrupt Controller driver

[Geert Uytterhoeven]

Hi Marc,

On Mon, Apr 29, 2019 at 12:07 PM Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 29/04/2019 10:36, Geert Uytterhoeven wrote:
> > Add a driver for the Renesas RZ/A1 Interrupt Controller.
> >
> > This supports using up to 8 external interrupts on RZ/A1, with
> > configurable sense select.
> >
> > NMI edge select is not yet supported.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- /dev/null
> > +++ b/drivers/irqchip/irq-renesas-rza1.c

> > +static void rza1_irqc_eoi(struct irq_data *d)
> > +{
> > +     struct rza1_irqc_priv *priv = irq_data_to_priv(d);
> > +     unsigned int bit = BIT(irqd_to_hwirq(d));
>
> Please use u32 instead of "unsigned int" for something that operates on
> HW registers.

Even for 16-bit registers?

> > +     u16 tmp;
> > +
> > +     tmp = readw(priv->base + IRQRR);
>
> Same thing here. It's less confusing to use a u32 and mask out the top
> bits if needed rather than having this implicit cast (applies all over
> the code).

... so yes.

>
> > +     if (tmp & bit)
> > +             writew(GENMASK(IRQC_NUM_IRQ - 1, 0) & ~bit, priv->base + IRQRR);
>
> Please use the _relaxed accessors all over the driver, you really do not
> need a DSB on each of these accesses.

OK.

> > +static int rza1_irqc_set_type(struct irq_data *d, unsigned int type)
> > +{
> > +     struct rza1_irqc_priv *priv = irq_data_to_priv(d);
> > +     unsigned int hw_irq = irqd_to_hwirq(d);
> > +     u16 sense, tmp;
> > +
> > +     switch (type & IRQ_TYPE_SENSE_MASK) {
> > +     case IRQ_TYPE_LEVEL_LOW:
> > +             sense = ICR1_IRQS_LEVEL_LOW;
> > +             break;
> > +
> > +     case IRQ_TYPE_EDGE_FALLING:
> > +             sense = ICR1_IRQS_EDGE_FALLING;
> > +             break;
> > +
> > +     case IRQ_TYPE_EDGE_RISING:
> > +             sense = ICR1_IRQS_EDGE_RISING;
> > +             break;
> > +
> > +     case IRQ_TYPE_EDGE_BOTH:
> > +             sense = ICR1_IRQS_EDGE_BOTH;
> > +             break;
> > +
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     tmp = readw(priv->base + ICR1);
> > +     tmp &= ~ICR1_IRQS_MASK(hw_irq);
> > +     tmp |= ICR1_IRQS(hw_irq, sense);
> > +     writew(tmp, priv->base + ICR1);
> > +     return 0;
>
> Don't you need to propagate the trigger configuration to the parent irqchip?

No, the line to the parent GIC is always configured for high-level.

> > +static int rza1_irqc_alloc(struct irq_domain *domain, unsigned int virq,
> > +                        unsigned int nr_irqs, void *arg)
> > +{
> > +     struct rza1_irqc_priv *priv = domain->host_data;
> > +     struct irq_fwspec *fwspec = arg;
> > +     struct irq_fwspec spec;
> > +     int ret;
> > +
> > +     ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
> > +                                         &priv->chip, priv);
> > +     if (ret)
> > +             return ret;
> > +
> > +     spec.fwnode = &priv->dev->of_node->fwnode;
> > +     spec.param_count = 3;
> > +     spec.param[0] = GIC_SPI;
> > +     spec.param[1] = priv->gic_spi_base + fwspec->param[0];
> > +     spec.param[2] = IRQ_TYPE_LEVEL_HIGH;
>
> This is related to my earlier question: Does this block turn everything
> into level interrupts?

That is my understanding of the hardware:
  - Low-level interrupts are cleared when input becomes high again,
  - Rising/falling/both edge interrupts are cleared by reading+writing IRQRR.

FTR, the Hardware User Manual is available from
https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza1h.html#documents
(Section 7. Interrupt Controller).

> > +static int rza1_irqc_probe(struct platform_device *pdev)
> > +{

> > +     priv->chip.name = dev_name(dev);
>
> name should normally be used to identify the overall "class" of

OK, replacing by "rza1-irqc".

> interrupt. .device is what should be used for the device itself.

You mean .parent_device?
Been there, done that: if I fill that in with "dev", it fails with

    gpio-keys keyboard: Unable to claim irq 41; error -13
    gpio-keys: probe of keyboard failed with error -13

due to the call to pm_runtime_get_sync() in irq_chip_pm_get() failing.
This driver doesn't have (and doesn't need) Runtime PM.

> > +struct rza1_irqc_info rza1_irqc_info = {
> > +     .gic_spi_base = 0,
> > +};
>
> To answer your question in the cover letter, I'd rather this came from
> DT. And otherwise, it should be be static.

(Oops, forget the "static const")

Using a custom property, or derived from 8 interrupt specifiers in the
interrupts property?

> It otherwise looks good to me. If you respin it quickly enough, I'm
> happy to take it for 5.2.

Thanks, will do tomorrow, so Chris (in NC.US; let's hope he doesn't
celebrate Golden Week) has a chance to comment...

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

Re: [PATCH 2/5] irqchip: Add Renesas RZ/A1 Interrupt Controller driver

[Marc Zyngier]

On 29/04/2019 12:21, Geert Uytterhoeven wrote:
> Hi Marc,
> 
> On Mon, Apr 29, 2019 at 12:07 PM Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 29/04/2019 10:36, Geert Uytterhoeven wrote:
>>> Add a driver for the Renesas RZ/A1 Interrupt Controller.
>>>
>>> This supports using up to 8 external interrupts on RZ/A1, with
>>> configurable sense select.
>>>
>>> NMI edge select is not yet supported.
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
>>> --- /dev/null
>>> +++ b/drivers/irqchip/irq-renesas-rza1.c
> 
>>> +static void rza1_irqc_eoi(struct irq_data *d)
>>> +{
>>> +     struct rza1_irqc_priv *priv = irq_data_to_priv(d);
>>> +     unsigned int bit = BIT(irqd_to_hwirq(d));
>>
>> Please use u32 instead of "unsigned int" for something that operates on
>> HW registers.
> 
> Even for 16-bit registers?

Ah, I'm so used to see 32bit accessors everywhere that I missed the fact
that this is a 16bit MMIO. How bizarre! ;-)

In general, try to have types that do match the actual size of the MMIO
access. There are a few exceptions to this rule (using an unsigned long
to be able to use bitmap operations, for example), but that's the
general idea.

> 
>>> +     u16 tmp;
>>> +
>>> +     tmp = readw(priv->base + IRQRR);
>>
>> Same thing here. It's less confusing to use a u32 and mask out the top
>> bits if needed rather than having this implicit cast (applies all over
>> the code).
> 
> ... so yes.

To sum it up:

readw/writew -> u16
readl/writel -> u32

> 
>>
>>> +     if (tmp & bit)
>>> +             writew(GENMASK(IRQC_NUM_IRQ - 1, 0) & ~bit, priv->base + IRQRR);
>>
>> Please use the _relaxed accessors all over the driver, you really do not
>> need a DSB on each of these accesses.
> 
> OK.
> 
>>> +static int rza1_irqc_set_type(struct irq_data *d, unsigned int type)
>>> +{
>>> +     struct rza1_irqc_priv *priv = irq_data_to_priv(d);
>>> +     unsigned int hw_irq = irqd_to_hwirq(d);
>>> +     u16 sense, tmp;
>>> +
>>> +     switch (type & IRQ_TYPE_SENSE_MASK) {
>>> +     case IRQ_TYPE_LEVEL_LOW:
>>> +             sense = ICR1_IRQS_LEVEL_LOW;
>>> +             break;
>>> +
>>> +     case IRQ_TYPE_EDGE_FALLING:
>>> +             sense = ICR1_IRQS_EDGE_FALLING;
>>> +             break;
>>> +
>>> +     case IRQ_TYPE_EDGE_RISING:
>>> +             sense = ICR1_IRQS_EDGE_RISING;
>>> +             break;
>>> +
>>> +     case IRQ_TYPE_EDGE_BOTH:
>>> +             sense = ICR1_IRQS_EDGE_BOTH;
>>> +             break;
>>> +
>>> +     default:
>>> +             return -EINVAL;
>>> +     }
>>> +
>>> +     tmp = readw(priv->base + ICR1);
>>> +     tmp &= ~ICR1_IRQS_MASK(hw_irq);
>>> +     tmp |= ICR1_IRQS(hw_irq, sense);
>>> +     writew(tmp, priv->base + ICR1);
>>> +     return 0;
>>
>> Don't you need to propagate the trigger configuration to the parent irqchip?
> 
> No, the line to the parent GIC is always configured for high-level.
> 
>>> +static int rza1_irqc_alloc(struct irq_domain *domain, unsigned int virq,
>>> +                        unsigned int nr_irqs, void *arg)
>>> +{
>>> +     struct rza1_irqc_priv *priv = domain->host_data;
>>> +     struct irq_fwspec *fwspec = arg;
>>> +     struct irq_fwspec spec;
>>> +     int ret;
>>> +
>>> +     ret = irq_domain_set_hwirq_and_chip(domain, virq, fwspec->param[0],
>>> +                                         &priv->chip, priv);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     spec.fwnode = &priv->dev->of_node->fwnode;
>>> +     spec.param_count = 3;
>>> +     spec.param[0] = GIC_SPI;
>>> +     spec.param[1] = priv->gic_spi_base + fwspec->param[0];
>>> +     spec.param[2] = IRQ_TYPE_LEVEL_HIGH;
>>
>> This is related to my earlier question: Does this block turn everything
>> into level interrupts?
> 
> That is my understanding of the hardware:
>   - Low-level interrupts are cleared when input becomes high again,
>   - Rising/falling/both edge interrupts are cleared by reading+writing IRQRR.
> 
> FTR, the Hardware User Manual is available from
> https://www.renesas.com/eu/en/products/microcontrollers-microprocessors/rz/rza/rza1h.html#documents
> (Section 7. Interrupt Controller).

OK, thanks for the detailed explanation.

> 
>>> +static int rza1_irqc_probe(struct platform_device *pdev)
>>> +{
> 
>>> +     priv->chip.name = dev_name(dev);
>>
>> name should normally be used to identify the overall "class" of
> 
> OK, replacing by "rza1-irqc".
> 
>> interrupt. .device is what should be used for the device itself.
> 
> You mean .parent_device?
> Been there, done that: if I fill that in with "dev", it fails with
> 
>     gpio-keys keyboard: Unable to claim irq 41; error -13
>     gpio-keys: probe of keyboard failed with error -13
> 
> due to the call to pm_runtime_get_sync() in irq_chip_pm_get() failing.
> This driver doesn't have (and doesn't need) Runtime PM.

OK, fair enough. Who needs PM anyway? ;-)

> 
>>> +struct rza1_irqc_info rza1_irqc_info = {
>>> +     .gic_spi_base = 0,
>>> +};
>>
>> To answer your question in the cover letter, I'd rather this came from
>> DT. And otherwise, it should be be static.
> 
> (Oops, forget the "static const")
> 
> Using a custom property, or derived from 8 interrupt specifiers in the
> interrupts property?

A custom property is fine by me (everybody does that anyway).

> 
>> It otherwise looks good to me. If you respin it quickly enough, I'm
>> happy to take it for 5.2.
> 
> Thanks, will do tomorrow, so Chris (in NC.US; let's hope he doesn't
> celebrate Golden Week) has a chance to comment...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH 0/5] ARM: rskrza1: Add RZ/A1 IRQC and input switches

[Chris Brandt]

Hi Geert,

Thanks for this patch!

I've been hacking this support into the standard GIC driver in our BSPs 
for years now. :o

On Mon, Apr 29, 2019, Geert Uytterhoeven wrote:
> I expect this driver to be reusable for RZ/A2, after adding a match
> entry with .gic_spi_base = 4.

Yes, the same IP block is in RZ/A2.

So with that said, should we call this driver irq-renesas-rza1.c or just
irq-renesas-rza.c?
It doesn't really matter to me.
For an RZ/A3, we might just use the same IP again.

Side note, I've seen this interrupt pin HW in some older SH4A devices 
(like SH7724 and SH7757). So it's been around for a while.


Chris

RE: [PATCH 2/5] irqchip: Add Renesas RZ/A1 Interrupt Controller driver

[Chris Brandt]

Hi Geert,

On Mon, Apr 29, 2019, Geert Uytterhoeven wrote:
> > It otherwise looks good to me. If you respin it quickly enough, I'm
> > happy to take it for 5.2.
> 
> Thanks, will do tomorrow, so Chris (in NC.US; let's hope he doesn't
> celebrate Golden Week) has a chance to comment...

No Golden Week for me.

Except, I get 1 week a year when I wake up in the morning and my inbox 
is not already full...so it is a golden week for me. :)

I'm going to try these patches out on RZ/A1 and RZ/A2 today.


Chris

Re: [PATCH 0/5] ARM: rskrza1: Add RZ/A1 IRQC and input switches

[Geert Uytterhoeven]

Hi Chris,

On Mon, Apr 29, 2019 at 2:21 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> I've been hacking this support into the standard GIC driver in our BSPs
> for years now. :o

Yeah, and having that patch in your tree breaks all other GICs, as
I found out the hard way ;-)

> On Mon, Apr 29, 2019, Geert Uytterhoeven wrote:
> > I expect this driver to be reusable for RZ/A2, after adding a match
> > entry with .gic_spi_base = 4.
>
> Yes, the same IP block is in RZ/A2.
>
> So with that said, should we call this driver irq-renesas-rza1.c or just
> irq-renesas-rza.c?
> It doesn't really matter to me.
> For an RZ/A3, we might just use the same IP again.

I've learned to be reluctant to put too many wildcards in names, as it may
start to bite in the future. For driver names, it's not that bad (they can
be changed), but for DT, it's a no-go.

So for RZ/A2, I think it's best to use

    compatible = "renesas,r7s9210-irqc", "renesas,rza1-irqc";
    renesas,gic-spi-base = <4>;

(adding "renesas,gic-spi-base = <0>" to r7s72100.dtsi as I speak).

> Side note, I've seen this interrupt pin HW in some older SH4A devices
> (like SH7724 and SH7757). So it's been around for a while.

Right:

    arch/sh/kernel/cpu/sh4a/setup-sh7343.c: { 0xa4140024, 0, 8, /* INTREQ00 */
    arch/sh/kernel/cpu/sh4a/setup-sh7366.c: { 0xa4140024, 0, 8, /* INTREQ00 */
    arch/sh/kernel/cpu/sh4a/setup-sh7722.c: { 0xa4140024, 0, 8, /* INTREQ00 */
    arch/sh/kernel/cpu/sh4a/setup-sh7723.c: { 0xa4140024, 0, 8, /* INTREQ00 */
    arch/sh/kernel/cpu/sh4a/setup-sh7724.c: { 0xa4140024, 0, 8, /* INTREQ00 */

However, according to the sh7724 documentation, the register set is
slightly different, as is its sense configuration (no support for both
edges, but support for high-level interrupts).

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

Re: [PATCH 3/5] soc: renesas: Enable RZ/A1 IRQC on RZ/A1H

[Simon Horman]

On Mon, Apr 29, 2019 at 11:36:29AM +0200, Geert Uytterhoeven wrote:
> Auto-enable the RZ/A1 Interrupt Controller when configuring a kernel
> with support for RZ/A1H SoCs.
> 
> This is similar to how interrupt controllers for other Renesas SoCs are
> enabled.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks Geert,

this is fine by me but other parts of the series need respinning.

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>


> ---
>  drivers/soc/renesas/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig
> index 68bfca6f20ddf8a7..1448b6dbcdb20bae 100644
> --- a/drivers/soc/renesas/Kconfig
> +++ b/drivers/soc/renesas/Kconfig
> @@ -57,6 +57,7 @@ config ARCH_R7S72100
>  	bool "RZ/A1H (R7S72100)"
>  	select PM
>  	select PM_GENERIC_DOMAINS
> +	select RENESAS_RZA1_IRQC
>  	select SYS_SUPPORTS_SH_MTU2
>  	select RENESAS_OSTM
>  
> -- 
> 2.17.1
> 

Re: [PATCH 4/5] ARM: dts: r7s72100: Add IRQC device node

[Simon Horman]

On Mon, Apr 29, 2019 at 11:36:30AM +0200, Geert Uytterhoeven wrote:
> Enable support for the IRQC on RZ/A1H, which is a small front-end to the
> GIC.  This allows to use up to 8 external interrupts with configurable
> sense select.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks Geert,

this is fine by me but other parts of the series need respinning.

Reviewed-by: Simon Horman <horms+renesas@verge.net.au>

RE: [PATCH 0/5] ARM: rskrza1: Add RZ/A1 IRQC and input switches

[Chris Brandt]

Hi Geert,

On Mon, Apr 29, 2019, Geert Uytterhoeven wrote:
> On Mon, Apr 29, 2019 at 2:21 PM Chris Brandt <Chris.Brandt@renesas.com>
> wrote:
> > I've been hacking this support into the standard GIC driver in our BSPs
> > for years now. :o
> 
> Yeah, and having that patch in your tree breaks all other GICs, as
> I found out the hard way ;-)

I never said it was a good hack :)


> So for RZ/A2, I think it's best to use
> 
>     compatible = "renesas,r7s9210-irqc", "renesas,rza1-irqc";
>     renesas,gic-spi-base = <4>;

That seems to make sense. It's specific to r7s9210 (RZ/A2), but 
compatible with the original version which was for rza1. It explains the
history.


> > Side note, I've seen this interrupt pin HW in some older SH4A devices
> > (like SH7724 and SH7757). So it's been around for a while.
> 
> Right:
> 
>     arch/sh/kernel/cpu/sh4a/setup-sh7343.c: { 0xa4140024, 0, 8, /*
> INTREQ00 */
>     arch/sh/kernel/cpu/sh4a/setup-sh7366.c: { 0xa4140024, 0, 8, /*
> INTREQ00 */
>     arch/sh/kernel/cpu/sh4a/setup-sh7722.c: { 0xa4140024, 0, 8, /*
> INTREQ00 */
>     arch/sh/kernel/cpu/sh4a/setup-sh7723.c: { 0xa4140024, 0, 8, /*
> INTREQ00 */
>     arch/sh/kernel/cpu/sh4a/setup-sh7724.c: { 0xa4140024, 0, 8, /*
> INTREQ00 */
> 
> However, according to the sh7724 documentation, the register set is
> slightly different, as is its sense configuration (no support for both
> edges, but support for high-level interrupts).

If I remember correctly, I think the design engineers can choose the 
sense as they wire it up internally. The ones in the SH7757 were chosen 
based on a specific use case. So far, the ones chosen for the RZ/A1 seem to
make everyone happy, so I assume we'll keep them that way.


Chris

RE: [PATCH 2/5] irqchip: Add Renesas RZ/A1 Interrupt Controller driver

[Chris Brandt]

Hi Geert and Marc,

I tested Geert's patches on RZ/A1 and RZ/A2 boards (push buttons and LCD
touchscreen controller). Works great!

$ cat /proc/interrupts
 49:       5050  fcfef800.interrupt-controller   1 Level     ft5x06_ts
 50:         32  fcfef800.interrupt-controller   3 Edge      SW1
 51:         12  fcfef800.interrupt-controller   2 Edge      SW2
 52:         10  fcfef800.interrupt-controller   5 Edge      SW3


For RZ/A2, I patched the driver/Kconfig as follows:

----------------------------------------------------------------------
diff --git a/drivers/irqchip/irq-renesas-rza1.c b/drivers/irqchip/irq-renesas-rza1.c
index 65110019adda..89a721864ff9 100644
--- a/drivers/irqchip/irq-renesas-rza1.c
+++ b/drivers/irqchip/irq-renesas-rza1.c
@@ -206,7 +206,12 @@ struct rza1_irqc_info rza1_irqc_info = {
        .gic_spi_base = 0,
 };
 
+struct rza1_irqc_info rza2_irqc_info = {
+       .gic_spi_base = 4,
+};
+
 static const struct of_device_id rza1_irqc_dt_ids[] = {
+       { .compatible = "renesas,r7s9210-irqc", &rza2_irqc_info },
        { .compatible = "renesas,rza1-irqc", &rza1_irqc_info },
        {},
 };
diff --git a/drivers/soc/renesas/Kconfig b/drivers/soc/renesas/Kconfig
index 1448b6dbcdb2..4ab7dcfd6a2f 100644
--- a/drivers/soc/renesas/Kconfig
+++ b/drivers/soc/renesas/Kconfig
@@ -65,6 +65,7 @@ config ARCH_R7S9210
        bool "RZ/A2 (R7S9210)"
        select PM
        select PM_GENERIC_DOMAINS
+       select RENESAS_RZA1_IRQC
        select RENESAS_OSTM
 
 config ARCH_R8A73A4
----------------------------------------------------------------------


However, as per:


On Mon, Apr 29, 2019, Marc Zyngier wrote:
> >>> +struct rza1_irqc_info rza1_irqc_info = {
> >>> +     .gic_spi_base = 0,
> >>> +};
> >>
> >> To answer your question in the cover letter, I'd rather this came from
> >> DT. And otherwise, it should be be static.
> >
> > (Oops, forget the "static const")
> >
> > Using a custom property, or derived from 8 interrupt specifiers in the
> > interrupts property?
> 
> A custom property is fine by me (everybody does that anyway).


I'll re-test after v2 of the patch when gic_spi_base moves to DT.



If you want to add RZ/A2 into this series to get it all done at once, 
I'll test that as well.

Just note that on the RZ/A2M EVB, only SW3 is connected to an interrupt 
pin (IRQ0).

&pinctrl {
	/* SW3 = IRQ0 */
	keyboard_pins: keyboard {
		pinmux = <RZA2_PINMUX(PORTJ, 1, 6)>;	/* IRQ0 */
	};
};

/ {
	keyboard {
		compatible = "gpio-keys";

		pinctrl-names = "default";
		pinctrl-0 = <&keyboard_pins>;

		key-3 {
			interrupt-parent = <&irqc>;
			interrupts = <0 IRQ_TYPE_EDGE_BOTH>;
			linux,code = <KEY_3>;
			label = "SW3";
			wakeup-source;
		};
	};
};



Chris