[PATCH v4 0/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[PATCH v4 0/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

On RZ/N1 devices, there are lots of GPIO interrupts that are multiplexed before
getting to the GIC interrupt controller. Other than the multiplexing, there is
no other logic applied to the signals.

The multiplexing cannot be handled dynamically because there is another CPU that
runs firmware. It's likely that the firmware will use some of these GPIO
interrupts and so we don't want them to move around.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v4:
 - Fix DT binding nits
v3:
 - Use 'interrupt-map' DT property to map the interrupts, this is very similar
   to PCIe MSI. The only difference is that we need to get hold of the interrupt
   specifier for the interupts coming into the irqmux.
   I had completely messed up the use of 'interrupt-map' in v2... oops.
 - Do not use a chained interrupt controller.
v2:
 - Split DT bindings into separate patch.
 - Use interrupt-map to allow the GPIO controller info to be specified
   as part of the irq.
 - Don't show status in binding examples.
 - Don't show the soc/board split in binding doc.
 - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.

Phil Edworthy (2):
  dt-bindings/interrupt-controller: rzn1: Add RZ/N1 gpio irq mux binding
  irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

 .../interrupt-controller/renesas,rzn1-mux.txt |  73 +++++++
 drivers/irqchip/Kconfig                       |   9 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/rzn1-irq-mux.c                | 205 ++++++++++++++++++
 4 files changed, 288 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/renesas,rzn1-mux.txt
 create mode 100644 drivers/irqchip/rzn1-irq-mux.c

-- 
2.17.1

[PATCH v4 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
configured to have 32 interrupt outputs, so we have a total of 96 GPIO
interrupts. All of these are passed to the GPIO IRQ Muxer, which selects
8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
aren't latched, so there is nothing to do in this driver when an interrupt
is received, other than tell the corresponding GPIO block.

Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
---
v4:
 - No change.
v3:
 - Use 'interrupt-map' DT property to map the interrupts, this is very similar
   to PCIe MSI. The only difference is that we need to get hold of the interrupt
   specifier for the interupts coming into the irqmux.
 - Do not use a chained interrupt controller.
v2:
 - Use interrupt-map to allow the GPIO controller info to be specified
   as part of the irq.
 - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.
---
 drivers/irqchip/Kconfig        |   9 ++
 drivers/irqchip/Makefile       |   1 +
 drivers/irqchip/rzn1-irq-mux.c | 205 +++++++++++++++++++++++++++++++++
 3 files changed, 215 insertions(+)
 create mode 100644 drivers/irqchip/rzn1-irq-mux.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 5dcb5456cd14..369f78d6b521 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -211,6 +211,15 @@ config RENESAS_IRQC
 	select GENERIC_IRQ_CHIP
 	select IRQ_DOMAIN
 
+config RENESAS_RZN1_IRQ_MUX
+	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
+	depends on ARCH_RZN1
+	select IRQ_DOMAIN
+	help
+	  Say yes here to add support for the GPIO IRQ multiplexer embedded
+	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
+	  the interrupts coming from the GPIO controllers are used.
+
 config ST_IRQCHIP
 	bool
 	select REGMAP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 7acd0e36d0b4..2edd42ef2182 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -46,6 +46,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_RZN1_IRQ_MUX)	+= rzn1-irq-mux.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/rzn1-irq-mux.c b/drivers/irqchip/rzn1-irq-mux.c
new file mode 100644
index 000000000000..ee7810b9b3f3
--- /dev/null
+++ b/drivers/irqchip/rzn1-irq-mux.c
@@ -0,0 +1,205 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RZ/N1 GPIO Interrupt Multiplexer
+ *
+ * Copyright (C) 2018 Renesas Electronics Europe Limited
+ *
+ * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each configured
+ * to have 32 interrupt outputs, so we have a total of 96 GPIO interrupts.
+ * All of these are passed to the GPIO IRQ Muxer, which selects 8 of the GPIO
+ * interrupts to pass onto the GIC.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+
+#define MAX_NR_INPUT_IRQS	96
+#define MAX_NR_OUTPUT_IRQS	8
+
+/*
+ * "interrupt-map" consists of 1 interrupt cell, 0 address cells, phandle to
+ * interrupt parent, and parent interrupt specifier (3 cells for GIC), giving
+ * a total of 5 cells.
+ */
+#define IMAP_LENGTH		5
+
+struct irqmux_priv;
+struct irqmux_one {
+	unsigned int irq;
+	unsigned int src_hwirq;
+	struct irqmux_priv *priv;
+};
+
+struct irqmux_priv {
+	struct device *dev;
+	struct irq_domain *irq_domain;
+	unsigned int nr_irqs;
+	struct irqmux_one mux[MAX_NR_OUTPUT_IRQS];
+};
+
+static irqreturn_t irqmux_handler(int irq, void *data)
+{
+	struct irqmux_one *mux = data;
+	struct irqmux_priv *priv = mux->priv;
+	unsigned int virq;
+
+	virq = irq_find_mapping(priv->irq_domain, mux->src_hwirq);
+
+	generic_handle_irq(virq);
+
+	return IRQ_HANDLED;
+}
+
+static int irqmux_domain_map(struct irq_domain *h, unsigned int irq,
+			     irq_hw_number_t hwirq)
+{
+	irq_set_chip_data(irq, h->host_data);
+	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+
+	return 0;
+}
+
+static const struct irq_domain_ops irqmux_domain_ops = {
+	.map = irqmux_domain_map,
+};
+
+static int irqmux_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct resource *res;
+	u32 __iomem *regs;
+	struct irqmux_priv *priv;
+	unsigned int i;
+	int nr_irqs;
+	int ret;
+	const __be32 *imap;
+	int imaplen;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	platform_set_drvdata(pdev, priv);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	regs = devm_ioremap_resource(dev, res);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
+
+	nr_irqs = of_irq_count(np);
+	if (nr_irqs < 0)
+		return nr_irqs;
+
+	if (nr_irqs > MAX_NR_OUTPUT_IRQS) {
+		dev_err(dev, "too many output interrupts\n");
+		return -ENOENT;
+	}
+
+	priv->nr_irqs = nr_irqs;
+
+	/* Look for the interrupt-map */
+	imap = of_get_property(np, "interrupt-map", &imaplen);
+	if (!imap)
+		return -ENOENT;
+	imaplen /= IMAP_LENGTH * sizeof(__be32);
+
+	/* Sometimes not all muxs are used */
+	if (imaplen < priv->nr_irqs)
+		priv->nr_irqs = imaplen;
+
+	/* Create IRQ domain for the interrupts coming from the GPIO blocks */
+	priv->irq_domain = irq_domain_add_linear(np, MAX_NR_INPUT_IRQS,
+						 &irqmux_domain_ops, priv);
+	if (!priv->irq_domain)
+		return -ENOMEM;
+
+	for (i = 0; i < MAX_NR_INPUT_IRQS; i++)
+		irq_create_mapping(priv->irq_domain, i);
+
+	for (i = 0; i < priv->nr_irqs; i++) {
+		struct irqmux_one *mux = &priv->mux[i];
+
+		ret = irq_of_parse_and_map(np, i);
+		if (ret < 0) {
+			ret = -ENOENT;
+			goto err;
+		}
+
+		mux->irq = ret;
+		mux->priv = priv;
+
+		/*
+		 * We need the first cell of the interrupt-map to configure
+		 * the hardware.
+		 */
+		mux->src_hwirq = be32_to_cpu(*imap);
+		imap += IMAP_LENGTH;
+
+		dev_info(dev, "%u: %u mapped irq %u\n", i,  mux->src_hwirq,
+			 mux->irq);
+
+		ret = devm_request_irq(dev, mux->irq, irqmux_handler,
+				       IRQF_SHARED | IRQF_NO_THREAD,
+				       "irqmux", mux);
+		if (ret < 0) {
+			dev_err(dev, "failed to request IRQ: %d\n", ret);
+			goto err;
+		}
+
+		/* Set up the hardware to pass the interrupt through */
+		writel(mux->src_hwirq, &regs[i]);
+	}
+
+	dev_info(dev, "probed, %d gpio interrupts\n", priv->nr_irqs);
+
+	return 0;
+
+err:
+	while (i--)
+		irq_dispose_mapping(priv->mux[i].irq);
+	irq_domain_remove(priv->irq_domain);
+
+	return ret;
+}
+
+static int irqmux_remove(struct platform_device *pdev)
+{
+	struct irqmux_priv *priv = platform_get_drvdata(pdev);
+	unsigned int i;
+
+	for (i = 0; i < priv->nr_irqs; i++)
+		irq_dispose_mapping(priv->mux[i].irq);
+	irq_domain_remove(priv->irq_domain);
+
+	return 0;
+}
+
+static const struct of_device_id irqmux_match[] = {
+	{ .compatible = "renesas,rzn1-gpioirqmux", },
+	{ /* sentinel */ },
+};
+
+MODULE_DEVICE_TABLE(of, irqmux_match);
+
+static struct platform_driver irqmux_driver = {
+	.driver = {
+		.name = "gpio_irq_mux",
+		.owner = THIS_MODULE,
+		.of_match_table = irqmux_match,
+	},
+	.probe = irqmux_probe,
+	.remove = irqmux_remove,
+};
+
+module_platform_driver(irqmux_driver);
+
+MODULE_DESCRIPTION("Renesas RZ/N1 GPIO IRQ Multiplexer Driver");
+MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH v4 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Marc Zyngier]

On Tue, 19 Feb 2019 15:55:11 +0000
Phil Edworthy <phil.edworthy@renesas.com> wrote:

+ LinusW, who seem to have taken an interest in irqchip hierarchies...

> On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> configured to have 32 interrupt outputs, so we have a total of 96 GPIO
> interrupts. All of these are passed to the GPIO IRQ Muxer, which selects
> 8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
> aren't latched, so there is nothing to do in this driver when an interrupt
> is received, other than tell the corresponding GPIO block.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> ---
> v4:
>  - No change.
> v3:
>  - Use 'interrupt-map' DT property to map the interrupts, this is very similar
>    to PCIe MSI. The only difference is that we need to get hold of the interrupt
>    specifier for the interupts coming into the irqmux.
>  - Do not use a chained interrupt controller.
> v2:
>  - Use interrupt-map to allow the GPIO controller info to be specified
>    as part of the irq.
>  - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.
> ---
>  drivers/irqchip/Kconfig        |   9 ++
>  drivers/irqchip/Makefile       |   1 +
>  drivers/irqchip/rzn1-irq-mux.c | 205 +++++++++++++++++++++++++++++++++
>  3 files changed, 215 insertions(+)
>  create mode 100644 drivers/irqchip/rzn1-irq-mux.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 5dcb5456cd14..369f78d6b521 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -211,6 +211,15 @@ config RENESAS_IRQC
>  	select GENERIC_IRQ_CHIP
>  	select IRQ_DOMAIN
>  
> +config RENESAS_RZN1_IRQ_MUX
> +	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
> +	depends on ARCH_RZN1
> +	select IRQ_DOMAIN
> +	help
> +	  Say yes here to add support for the GPIO IRQ multiplexer embedded
> +	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
> +	  the interrupts coming from the GPIO controllers are used.
> +
>  config ST_IRQCHIP
>  	bool
>  	select REGMAP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 7acd0e36d0b4..2edd42ef2182 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -46,6 +46,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_RZN1_IRQ_MUX)	+= rzn1-irq-mux.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/rzn1-irq-mux.c b/drivers/irqchip/rzn1-irq-mux.c
> new file mode 100644
> index 000000000000..ee7810b9b3f3
> --- /dev/null
> +++ b/drivers/irqchip/rzn1-irq-mux.c
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RZ/N1 GPIO Interrupt Multiplexer
> + *
> + * Copyright (C) 2018 Renesas Electronics Europe Limited
> + *
> + * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each configured
> + * to have 32 interrupt outputs, so we have a total of 96 GPIO interrupts.
> + * All of these are passed to the GPIO IRQ Muxer, which selects 8 of the GPIO
> + * interrupts to pass onto the GIC.

So that I get it right:

- 96 inputs
- 8 outputs

This driver picks 8 of the inputs (and at most 8), and pass then down
to the GIC. Do I understand that correctly?

> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +
> +#define MAX_NR_INPUT_IRQS	96
> +#define MAX_NR_OUTPUT_IRQS	8
> +
> +/*
> + * "interrupt-map" consists of 1 interrupt cell, 0 address cells, phandle to
> + * interrupt parent, and parent interrupt specifier (3 cells for GIC), giving
> + * a total of 5 cells.
> + */
> +#define IMAP_LENGTH		5

Although the maths do check out, I find it rather dangerous. The
versatile nature of DT makes me say that sooner or later, someone is
going to punt this in front of something that will need more than 3
interrupt cells for the parent irqchip, and things will be...
interesting. Even GICv3 in some configuration requires 4 cells.

You've been warned! ;-)

> +
> +struct irqmux_priv;
> +struct irqmux_one {
> +	unsigned int irq;
> +	unsigned int src_hwirq;
> +	struct irqmux_priv *priv;
> +};
> +
> +struct irqmux_priv {
> +	struct device *dev;
> +	struct irq_domain *irq_domain;
> +	unsigned int nr_irqs;
> +	struct irqmux_one mux[MAX_NR_OUTPUT_IRQS];
> +};
> +
> +static irqreturn_t irqmux_handler(int irq, void *data)
> +{
> +	struct irqmux_one *mux = data;
> +	struct irqmux_priv *priv = mux->priv;
> +	unsigned int virq;
> +
> +	virq = irq_find_mapping(priv->irq_domain, mux->src_hwirq);
> +
> +	generic_handle_irq(virq);
> +
> +	return IRQ_HANDLED;
> +}

Oh $DEITY. No, really not. I don't know where to start.

- This type of handler should almost never appear in an interrupt
  controller driver. Under the right circumstances, it will deadlock.

- If I understood the above correctly, this should be modelled as a
  hierarchy sitting on top of the GIC, and not as a mux. A good way to
  notice that this is not a mux is that there is no register to read
  and find out what fired.

- The fact that you have to invent src_hwirq instead of directly using
  data->hwirq is a clear sign that something is amiss.

> +
> +static int irqmux_domain_map(struct irq_domain *h, unsigned int irq,
> +			     irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_data(irq, h->host_data);
> +	irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);

Another sure sign that this is wrong, the flow handler. In 99% of the
case, using handle_simple_irq is wrong. I'd expect something that
matches the underlying interrupt controller (a fasteoi handler).

> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops irqmux_domain_ops = {
> +	.map = irqmux_domain_map,
> +};
> +
> +static int irqmux_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct resource *res;
> +	u32 __iomem *regs;
> +	struct irqmux_priv *priv;
> +	unsigned int i;
> +	int nr_irqs;
> +	int ret;
> +	const __be32 *imap;
> +	int imaplen;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	platform_set_drvdata(pdev, priv);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
> +
> +	nr_irqs = of_irq_count(np);
> +	if (nr_irqs < 0)
> +		return nr_irqs;
> +
> +	if (nr_irqs > MAX_NR_OUTPUT_IRQS) {
> +		dev_err(dev, "too many output interrupts\n");
> +		return -ENOENT;
> +	}
> +
> +	priv->nr_irqs = nr_irqs;
> +
> +	/* Look for the interrupt-map */
> +	imap = of_get_property(np, "interrupt-map", &imaplen);
> +	if (!imap)
> +		return -ENOENT;
> +	imaplen /= IMAP_LENGTH * sizeof(__be32);
> +
> +	/* Sometimes not all muxs are used */
> +	if (imaplen < priv->nr_irqs)
> +		priv->nr_irqs = imaplen;
> +
> +	/* Create IRQ domain for the interrupts coming from the GPIO blocks */
> +	priv->irq_domain = irq_domain_add_linear(np, MAX_NR_INPUT_IRQS,
> +						 &irqmux_domain_ops, priv);
> +	if (!priv->irq_domain)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < MAX_NR_INPUT_IRQS; i++)
> +		irq_create_mapping(priv->irq_domain, i);

This should never happen. Mappings should be created from discovering
the interrupt specifiers for devices in the DT, and not eagerly at
probe time.

> +
> +	for (i = 0; i < priv->nr_irqs; i++) {
> +		struct irqmux_one *mux = &priv->mux[i];
> +
> +		ret = irq_of_parse_and_map(np, i);
> +		if (ret < 0) {
> +			ret = -ENOENT;
> +			goto err;
> +		}
> +
> +		mux->irq = ret;
> +		mux->priv = priv;
> +
> +		/*
> +		 * We need the first cell of the interrupt-map to configure
> +		 * the hardware.
> +		 */
> +		mux->src_hwirq = be32_to_cpu(*imap);
> +		imap += IMAP_LENGTH;
> +
> +		dev_info(dev, "%u: %u mapped irq %u\n", i,  mux->src_hwirq,
> +			 mux->irq);
> +
> +		ret = devm_request_irq(dev, mux->irq, irqmux_handler,
> +				       IRQF_SHARED | IRQF_NO_THREAD,
> +				       "irqmux", mux);

And this is a result of using an interrupt handler in the driver. Bad.

> +		if (ret < 0) {
> +			dev_err(dev, "failed to request IRQ: %d\n", ret);
> +			goto err;
> +		}
> +
> +		/* Set up the hardware to pass the interrupt through */
> +		writel(mux->src_hwirq, &regs[i]);

It feels very odd that you configure the routing at probe time, and not
at runtime. This should really happen when the client devices are
probed...

> +	}
> +
> +	dev_info(dev, "probed, %d gpio interrupts\n", priv->nr_irqs);
> +
> +	return 0;
> +
> +err:
> +	while (i--)
> +		irq_dispose_mapping(priv->mux[i].irq);
> +	irq_domain_remove(priv->irq_domain);
> +
> +	return ret;
> +}
> +
> +static int irqmux_remove(struct platform_device *pdev)
> +{
> +	struct irqmux_priv *priv = platform_get_drvdata(pdev);
> +	unsigned int i;
> +
> +	for (i = 0; i < priv->nr_irqs; i++)
> +		irq_dispose_mapping(priv->mux[i].irq);
> +	irq_domain_remove(priv->irq_domain);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id irqmux_match[] = {
> +	{ .compatible = "renesas,rzn1-gpioirqmux", },
> +	{ /* sentinel */ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, irqmux_match);
> +
> +static struct platform_driver irqmux_driver = {
> +	.driver = {
> +		.name = "gpio_irq_mux",
> +		.owner = THIS_MODULE,
> +		.of_match_table = irqmux_match,
> +	},
> +	.probe = irqmux_probe,
> +	.remove = irqmux_remove,
> +};
> +
> +module_platform_driver(irqmux_driver);
> +
> +MODULE_DESCRIPTION("Renesas RZ/N1 GPIO IRQ Multiplexer Driver");
> +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
> +MODULE_LICENSE("GPL v2");

Can you please confirm that my understanding of how the HW works is
correct? If so, we can help you turning this around.

Thanks,

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

RE: [PATCH v4 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hi Marc

On 19 February 2019 20:29 Marc Zyngier wrote:
> On Tue, 19 Feb 2019 15:55:11 +0000 Phil Edworthy wrote:
> 
> + LinusW, who seem to have taken an interest in irqchip hierarchies...
> 
> > On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> > configured to have 32 interrupt outputs, so we have a total of 96 GPIO
> > interrupts. All of these are passed to the GPIO IRQ Muxer, which
> > selects
> > 8 of the GPIO interrupts to pass onto the GIC. The interrupt signals
> > aren't latched, so there is nothing to do in this driver when an
> > interrupt is received, other than tell the corresponding GPIO block.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy@renesas.com>
> > ---
> > v4:
> >  - No change.
> > v3:
> >  - Use 'interrupt-map' DT property to map the interrupts, this is very similar
> >    to PCIe MSI. The only difference is that we need to get hold of the
> interrupt
> >    specifier for the interupts coming into the irqmux.
> >  - Do not use a chained interrupt controller.
> > v2:
> >  - Use interrupt-map to allow the GPIO controller info to be specified
> >    as part of the irq.
> >  - Renamed struct and funcs from 'girq' to a more comprehenisble 'irqmux'.
> > ---
> >  drivers/irqchip/Kconfig        |   9 ++
> >  drivers/irqchip/Makefile       |   1 +
> >  drivers/irqchip/rzn1-irq-mux.c | 205
> > +++++++++++++++++++++++++++++++++
> >  3 files changed, 215 insertions(+)
> >  create mode 100644 drivers/irqchip/rzn1-irq-mux.c
> >
> > diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig index
> > 5dcb5456cd14..369f78d6b521 100644
> > --- a/drivers/irqchip/Kconfig
> > +++ b/drivers/irqchip/Kconfig
> > @@ -211,6 +211,15 @@ config RENESAS_IRQC
> >  	select GENERIC_IRQ_CHIP
> >  	select IRQ_DOMAIN
> >
> > +config RENESAS_RZN1_IRQ_MUX
> > +	bool "Renesas RZ/N1 GPIO IRQ multiplexer support"
> > +	depends on ARCH_RZN1
> > +	select IRQ_DOMAIN
> > +	help
> > +	  Say yes here to add support for the GPIO IRQ multiplexer
> embedded
> > +	  in Renesas RZ/N1 SoC devices. The GPIO IRQ Muxer selects which of
> > +	  the interrupts coming from the GPIO controllers are used.
> > +
> >  config ST_IRQCHIP
> >  	bool
> >  	select REGMAP
> > diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile index
> > 7acd0e36d0b4..2edd42ef2182 100644
> > --- a/drivers/irqchip/Makefile
> > +++ b/drivers/irqchip/Makefile
> > @@ -46,6 +46,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_RZN1_IRQ_MUX)	+= rzn1-irq-mux.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/rzn1-irq-mux.c
> > b/drivers/irqchip/rzn1-irq-mux.c new file mode 100644 index
> > 000000000000..ee7810b9b3f3
> > --- /dev/null
> > +++ b/drivers/irqchip/rzn1-irq-mux.c
> > @@ -0,0 +1,205 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * RZ/N1 GPIO Interrupt Multiplexer
> > + *
> > + * Copyright (C) 2018 Renesas Electronics Europe Limited
> > + *
> > + * On RZ/N1 devices, there are 3 Synopsys DesignWare GPIO blocks each
> > +configured
> > + * to have 32 interrupt outputs, so we have a total of 96 GPIO interrupts.
> > + * All of these are passed to the GPIO IRQ Muxer, which selects 8 of
> > +the GPIO
> > + * interrupts to pass onto the GIC.
> 
> So that I get it right:
> 
> - 96 inputs
> - 8 outputs
> 
> This driver picks 8 of the inputs (and at most 8), and pass then down to the
> GIC. Do I understand that correctly?
Correct.

> > + */
> > +
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/irqdomain.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +
> > +#define MAX_NR_INPUT_IRQS	96
> > +#define MAX_NR_OUTPUT_IRQS	8
> > +
> > +/*
> > + * "interrupt-map" consists of 1 interrupt cell, 0 address cells,
> > +phandle to
> > + * interrupt parent, and parent interrupt specifier (3 cells for
> > +GIC), giving
> > + * a total of 5 cells.
> > + */
> > +#define IMAP_LENGTH		5
> 
> Although the maths do check out, I find it rather dangerous. The versatile
> nature of DT makes me say that sooner or later, someone is going to punt
> this in front of something that will need more than 3 interrupt cells for the
> parent irqchip, and things will be...
> interesting. Even GICv3 in some configuration requires 4 cells.
> 
> You've been warned! ;-)
Warning noted! I would like to think that no one does this in HW again,
though history suggests otherwise.

> > +
> > +struct irqmux_priv;
> > +struct irqmux_one {
> > +	unsigned int irq;
> > +	unsigned int src_hwirq;
> > +	struct irqmux_priv *priv;
> > +};
> > +
> > +struct irqmux_priv {
> > +	struct device *dev;
> > +	struct irq_domain *irq_domain;
> > +	unsigned int nr_irqs;
> > +	struct irqmux_one mux[MAX_NR_OUTPUT_IRQS]; };
> > +
> > +static irqreturn_t irqmux_handler(int irq, void *data) {
> > +	struct irqmux_one *mux = data;
> > +	struct irqmux_priv *priv = mux->priv;
> > +	unsigned int virq;
> > +
> > +	virq = irq_find_mapping(priv->irq_domain, mux->src_hwirq);
> > +
> > +	generic_handle_irq(virq);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> Oh $DEITY. No, really not. I don't know where to start.
Oh dear... not a good opening.

> - This type of handler should almost never appear in an interrupt
>   controller driver. Under the right circumstances, it will deadlock.
> 
> - If I understood the above correctly, this should be modelled as a
>   hierarchy sitting on top of the GIC, and not as a mux. A good way to
>   notice that this is not a mux is that there is no register to read
>   and find out what fired.
> 
> - The fact that you have to invent src_hwirq instead of directly using
>   data->hwirq is a clear sign that something is amiss.
> 
> > +
> > +static int irqmux_domain_map(struct irq_domain *h, unsigned int irq,
> > +			     irq_hw_number_t hwirq)
> > +{
> > +	irq_set_chip_data(irq, h->host_data);
> > +	irq_set_chip_and_handler(irq, &dummy_irq_chip,
> handle_simple_irq);
> 
> Another sure sign that this is wrong, the flow handler. In 99% of the case,
> using handle_simple_irq is wrong. I'd expect something that matches the
> underlying interrupt controller (a fasteoi handler).
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct irq_domain_ops irqmux_domain_ops = {
> > +	.map = irqmux_domain_map,
> > +};
> > +
> > +static int irqmux_probe(struct platform_device *pdev) {
> > +	struct device *dev = &pdev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	struct resource *res;
> > +	u32 __iomem *regs;
> > +	struct irqmux_priv *priv;
> > +	unsigned int i;
> > +	int nr_irqs;
> > +	int ret;
> > +	const __be32 *imap;
> > +	int imaplen;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->dev = dev;
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	regs = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(regs))
> > +		return PTR_ERR(regs);
> > +
> > +	nr_irqs = of_irq_count(np);
> > +	if (nr_irqs < 0)
> > +		return nr_irqs;
> > +
> > +	if (nr_irqs > MAX_NR_OUTPUT_IRQS) {
> > +		dev_err(dev, "too many output interrupts\n");
> > +		return -ENOENT;
> > +	}
> > +
> > +	priv->nr_irqs = nr_irqs;
> > +
> > +	/* Look for the interrupt-map */
> > +	imap = of_get_property(np, "interrupt-map", &imaplen);
> > +	if (!imap)
> > +		return -ENOENT;
> > +	imaplen /= IMAP_LENGTH * sizeof(__be32);
> > +
> > +	/* Sometimes not all muxs are used */
> > +	if (imaplen < priv->nr_irqs)
> > +		priv->nr_irqs = imaplen;
> > +
> > +	/* Create IRQ domain for the interrupts coming from the GPIO blocks
> */
> > +	priv->irq_domain = irq_domain_add_linear(np,
> MAX_NR_INPUT_IRQS,
> > +						 &irqmux_domain_ops, priv);
> > +	if (!priv->irq_domain)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < MAX_NR_INPUT_IRQS; i++)
> > +		irq_create_mapping(priv->irq_domain, i);
> 
> This should never happen. Mappings should be created from discovering the
> interrupt specifiers for devices in the DT, and not eagerly at probe time.
The key issue here is that the mappings should not be dynamically allocated.
On the device that has this hardware, there is a Cortex M3 that is likely to use
some of these GPIO interrupts.
Maybe it would be better to limit the number of GPIO irqs that Linux can
configure dynamically.

> > +
> > +	for (i = 0; i < priv->nr_irqs; i++) {
> > +		struct irqmux_one *mux = &priv->mux[i];
> > +
> > +		ret = irq_of_parse_and_map(np, i);
> > +		if (ret < 0) {
> > +			ret = -ENOENT;
> > +			goto err;
> > +		}
> > +
> > +		mux->irq = ret;
> > +		mux->priv = priv;
> > +
> > +		/*
> > +		 * We need the first cell of the interrupt-map to configure
> > +		 * the hardware.
> > +		 */
> > +		mux->src_hwirq = be32_to_cpu(*imap);
> > +		imap += IMAP_LENGTH;
> > +
> > +		dev_info(dev, "%u: %u mapped irq %u\n", i,  mux-
> >src_hwirq,
> > +			 mux->irq);
> > +
> > +		ret = devm_request_irq(dev, mux->irq, irqmux_handler,
> > +				       IRQF_SHARED | IRQF_NO_THREAD,
> > +				       "irqmux", mux);
> 
> And this is a result of using an interrupt handler in the driver. Bad.
> 
> > +		if (ret < 0) {
> > +			dev_err(dev, "failed to request IRQ: %d\n", ret);
> > +			goto err;
> > +		}
> > +
> > +		/* Set up the hardware to pass the interrupt through */
> > +		writel(mux->src_hwirq, &regs[i]);
> 
> It feels very odd that you configure the routing at probe time, and not at
> runtime. This should really happen when the client devices are probed...
> 
> > +	}
> > +
> > +	dev_info(dev, "probed, %d gpio interrupts\n", priv->nr_irqs);
> > +
> > +	return 0;
> > +
> > +err:
> > +	while (i--)
> > +		irq_dispose_mapping(priv->mux[i].irq);
> > +	irq_domain_remove(priv->irq_domain);
> > +
> > +	return ret;
> > +}
> > +
> > +static int irqmux_remove(struct platform_device *pdev) {
> > +	struct irqmux_priv *priv = platform_get_drvdata(pdev);
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < priv->nr_irqs; i++)
> > +		irq_dispose_mapping(priv->mux[i].irq);
> > +	irq_domain_remove(priv->irq_domain);
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id irqmux_match[] = {
> > +	{ .compatible = "renesas,rzn1-gpioirqmux", },
> > +	{ /* sentinel */ },
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, irqmux_match);
> > +
> > +static struct platform_driver irqmux_driver = {
> > +	.driver = {
> > +		.name = "gpio_irq_mux",
> > +		.owner = THIS_MODULE,
> > +		.of_match_table = irqmux_match,
> > +	},
> > +	.probe = irqmux_probe,
> > +	.remove = irqmux_remove,
> > +};
> > +
> > +module_platform_driver(irqmux_driver);
> > +
> > +MODULE_DESCRIPTION("Renesas RZ/N1 GPIO IRQ Multiplexer Driver");
> > +MODULE_AUTHOR("Phil Edworthy <phil.edworthy@renesas.com>");
> > +MODULE_LICENSE("GPL v2");
> 
> Can you please confirm that my understanding of how the HW works is
> correct? If so, we can help you turning this around.
Looks like I've gone off in the wrong direction yet again.

Thanks for your comments,
Phil

Re: [PATCH v4 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Marc Zyngier]

On Wed, 20 Feb 2019 09:07:02 +0000,
Phil Edworthy <phil.edworthy@renesas.com> wrote:
> 
> Hi Marc
> 
> On 19 February 2019 20:29 Marc Zyngier wrote:

[...]

> > > +	for (i = 0; i < MAX_NR_INPUT_IRQS; i++)
> > > +		irq_create_mapping(priv->irq_domain, i);
> > 
> > This should never happen. Mappings should be created from discovering the
> > interrupt specifiers for devices in the DT, and not eagerly at probe time.
>
> The key issue here is that the mappings should not be dynamically
> allocated.  On the device that has this hardware, there is a Cortex
> M3 that is likely to use some of these GPIO interrupts.  Maybe it
> would be better to limit the number of GPIO irqs that Linux can
> configure dynamically.

But whatever the M3 is going to use is known statically for a given
instance of this platform, right? You can always tell from the device
tree which pins are available for Linux and which are not. Or can't you?

> Looks like I've gone off in the wrong direction yet again.

Nothing we can't help with. If you can explain all the constraints of
the platform, we can come up with a fairly simple driver. And surely
LinusW can chime in for the DT part, which seems to need some loving
too.

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.

RE: [PATCH v4 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hi Marc,

On 20 February 2019 10:05 Marc Zyngier wrote:
> On Wed, 20 Feb 2019 09:07:02 +0000, Phil Edworthy wrote:
> > On 19 February 2019 20:29 Marc Zyngier wrote:
> 
> [...]
> 
> > > > +	for (i = 0; i < MAX_NR_INPUT_IRQS; i++)
> > > > +		irq_create_mapping(priv->irq_domain, i);
> > >
> > > This should never happen. Mappings should be created from discovering
> the
> > > interrupt specifiers for devices in the DT, and not eagerly at probe time.
> >
> > The key issue here is that the mappings should not be dynamically
> > allocated.  On the device that has this hardware, there is a Cortex
> > M3 that is likely to use some of these GPIO interrupts.  Maybe it
> > would be better to limit the number of GPIO irqs that Linux can
> > configure dynamically.
> 
> But whatever the M3 is going to use is known statically for a given
> instance of this platform, right? You can always tell from the device
> tree which pins are available for Linux and which are not. Or can't you?
Yes, you can tell what pins are available to Linux. You can't tell how
many pins have been setup as GPIO irqs for use by the M3 though.
I suppose the DT could describe the M3 as well, though I can imagine
that could get complicated. I've not seen a DT that covers different
cores running different software, esp. when the M3 firmware doesn't
use DT.
I was thinking that the DT could have a prop to tell this driver what
GPIO irqs are already in use.
 
> > Looks like I've gone off in the wrong direction yet again.
> 
> Nothing we can't help with. If you can explain all the constraints of
> the platform, we can come up with a fairly simple driver. And surely
> LinusW can chime in for the DT part, which seems to need some loving
> too.
Many thanks, I need some help here!

Phil

RE: [PATCH v4 2/2] irqchip: Add support for Renesas RZ/N1 GPIO interrupt multiplexer

[Phil Edworthy]

Hi Marc,

On 20 February 2019 11:33 Phil Edworthy wrote:
> On 20 February 2019 10:05 Marc Zyngier wrote:
> > On Wed, 20 Feb 2019 09:07:02 +0000, Phil Edworthy wrote:
> > > On 19 February 2019 20:29 Marc Zyngier wrote:
> >
> > [...]
> >
> > > > > +	for (i = 0; i < MAX_NR_INPUT_IRQS; i++)
> > > > > +		irq_create_mapping(priv->irq_domain, i);
> > > >
> > > > This should never happen. Mappings should be created from
> > > > discovering
> > the
> > > > interrupt specifiers for devices in the DT, and not eagerly at probe time.
> > >
> > > The key issue here is that the mappings should not be dynamically
> > > allocated.  On the device that has this hardware, there is a Cortex
> > > M3 that is likely to use some of these GPIO interrupts.  Maybe it
> > > would be better to limit the number of GPIO irqs that Linux can
> > > configure dynamically.
> >
> > But whatever the M3 is going to use is known statically for a given
> > instance of this platform, right? You can always tell from the device
> > tree which pins are available for Linux and which are not. Or can't you?
> Yes, you can tell what pins are available to Linux. You can't tell how many pins
> have been setup as GPIO irqs for use by the M3 though.
> I suppose the DT could describe the M3 as well, though I can imagine that
> could get complicated. I've not seen a DT that covers different cores running
> different software, esp. when the M3 firmware doesn't use DT.
> I was thinking that the DT could have a prop to tell this driver what GPIO irqs
> are already in use.
I've just realised that it doesn't matter if we don't know how many interrupts
are used by the M3, as Linux will not claim more than the remaining interrupts.

Thanks
Phil

> > > Looks like I've gone off in the wrong direction yet again.
> >
> > Nothing we can't help with. If you can explain all the constraints of
> > the platform, we can come up with a fairly simple driver. And surely
> > LinusW can chime in for the DT part, which seems to need some loving
> > too.
> Many thanks, I need some help here!
> 
> Phil