Re: [RFC PATCH 2/4] irqchip: Add RZ/G2L IA55 Interrupt Controller driver

[Marc Zyngier <maz@xxxxxxxxxx>]

On Tue, 03 Aug 2021 18:51:07 +0100,
Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> 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 (GPIOINT0-122)
> to be used as IRQ lines.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> ---
>  drivers/irqchip/Kconfig             |   8 +
>  drivers/irqchip/Makefile            |   1 +
>  drivers/irqchip/irq-renesas-rzg2l.c | 557 ++++++++++++++++++++++++++++
>  drivers/soc/renesas/Kconfig         |   1 +
>  4 files changed, 567 insertions(+)
>  create mode 100644 drivers/irqchip/irq-renesas-rzg2l.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 62543a4eccc0..790c19ad6e39 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -236,6 +236,14 @@ 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_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..abfaaa8a09af
> --- /dev/null
> +++ b/drivers/irqchip/irq-renesas-rzg2l.c
> @@ -0,0 +1,557 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Renesas RZ/G2L IRQC Driver
> + *
> + * Copyright (C) 2020 Renesas Electronics Corporation.
> + */
> +
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/ioport.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/clk.h>
> +#include <linux/irqdomain.h>
> +#include <linux/err.h>
> +#include <linux/of_irq.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +
> +#define RZG2L_IRQC_IRQ_MAX		41
> +#define RZG2L_TINT_MAX_INTERRUPTS	32
> +#define RZG2L_TINT_IRQ_START_INDEX	9
> +
> +#define TSCR		0x20
> +#define TITSR0		0x24
> +#define TITSR1		0x28
> +#define TSSR(n)		(0x30 + ((n) * 4))
> +#define IRQ_MASK	0x3
> +
> +#define RISING_EDGE	0
> +#define FALLING_EDGE	1
> +#define HIGH_LEVEL	2
> +#define LOW_LEVEL	3
> +
> +#define RZG2L_TINT_EDGE_BOTH	BIT(4)
> +
> +#define RZG2L_MAX_PINS_PER_PORT		8
> +#define RZG2L_PIN_ID_TO_PORT(id)	((id) / RZG2L_MAX_PINS_PER_PORT)
> +#define RZG2L_PIN_ID_TO_PIN(id)		((id) % RZG2L_MAX_PINS_PER_PORT)
> +
> +struct irqc_irq {
> +	int hw_irq;
> +	int requested_irq;
> +	struct irqc_priv *p;
> +	struct rzg2l_pin_info *pin;
> +};
> +
> +struct rzg2l_pin_info {
> +	u32 port;
> +	u8 bit;
> +	u32 id;

Given the way you initialise things below, are these types the right
ones?

> +	u32 trigger_type;
> +	unsigned int type;
> +	u8 pin_state;
> +};
> +
> +struct irqc_priv {
> +	void __iomem *iomem;
> +	struct irqc_irq irq[RZG2L_IRQC_IRQ_MAX];
> +	DECLARE_BITMAP(tint_slot, RZG2L_TINT_MAX_INTERRUPTS);
> +	unsigned int number_of_irqs;
> +	struct device *dev;
> +	struct irq_chip_generic *gc;
> +	struct irq_chip chip;
> +	struct irq_domain *irq_domain;
> +	struct irq_domain *parent_domain;
> +	unsigned int tint_irq_start;
> +	atomic_t wakeup_path;
> +	struct clk *clk;
> +	struct clk *pclk;
> +};
> +
> +static const struct rzg2l_pin_info gpio_pin_info[] = {
> +	{ 0, 0, 0 }, { 0, 1, 1 },
> +	{ 1, 0, 2 }, { 1, 1, 3 },
> +	{ 2, 0, 4 }, { 2, 1, 5 },
> +	{ 3, 0, 6 }, { 3, 1, 7 },
> +	{ 4, 0, 8 }, { 4, 1, 9 },
> +	{ 5, 0, 10 }, { 5, 1, 11 }, { 5, 2, 12 },
> +	{ 6, 0, 13 }, { 6, 1, 14 },
> +	{ 7, 0, 15 }, { 7, 1, 16 }, { 7, 2, 17 },
> +	{ 8, 0, 18 }, { 8, 1, 19 }, { 8, 2, 20 },
> +	{ 9, 0, 21 }, { 9, 1, 22 },
> +	{ 10, 0, 23 }, { 10, 1, 24 },
> +	{ 11, 0, 25 }, { 11, 1, 26 },
> +	{ 12, 0, 27 }, { 12, 1, 28 },
> +	{ 13, 0, 29 }, { 13, 1, 30 }, { 13, 2, 31 },
> +	{ 14, 0, 32 }, { 14, 1, 33 },
> +	{ 15, 0, 34 }, { 15, 1, 35 },
> +	{ 16, 0, 36 }, { 16, 1, 37 },
> +	{ 17, 0, 38 }, { 17, 1, 39 }, { 17, 2, 40 },
> +	{ 18, 0, 41 }, { 18, 1, 42 },
> +	{ 19, 0, 43 }, { 19, 1, 44 },
> +	{ 20, 0, 45 }, { 20, 1, 46 }, { 20, 2, 47 },
> +	{ 21, 0, 48 }, { 21, 1, 49 },
> +	{ 22, 0, 50 }, { 22, 1, 51 },
> +	{ 23, 0, 52 }, { 23, 1, 53 },
> +	{ 24, 0, 54 }, { 24, 1, 55 },
> +	{ 25, 0, 56 }, { 25, 1, 57 },
> +	{ 26, 0, 58 }, { 26, 1, 59 },
> +	{ 27, 0, 60 }, { 27, 1, 61 },
> +	{ 28, 0, 62 }, { 28, 1, 63 },
> +	{ 29, 0, 64 }, { 29, 1, 65 },
> +	{ 30, 0, 66 }, { 30, 1, 67 },
> +	{ 31, 0, 68 }, { 31, 1, 69 },
> +	{ 32, 0, 70 }, { 32, 1, 71 },
> +	{ 33, 0, 72 }, { 33, 1, 73 },
> +	{ 34, 0, 74 }, { 34, 1, 75 },
> +	{ 35, 0, 76 }, { 35, 1, 77 },
> +	{ 36, 0, 78 }, { 36, 1, 79 },
> +	{ 37, 0, 80 }, { 37, 1, 81 }, { 37, 2, 82 },
> +	{ 38, 0, 83 }, { 38, 1, 84 },
> +	{ 39, 0, 85 }, { 39, 1, 86 }, { 39, 2, 87 },
> +	{ 40, 0, 88 }, { 40, 1, 89 }, { 40, 2, 90 },
> +	{ 41, 0, 91 }, { 41, 1, 92 },
> +	{ 42, 0, 93 }, { 42, 1, 94 }, { 42, 2, 95 }, { 42, 3, 96 }, { 42, 4, 97 },
> +	{ 43, 0, 98 }, { 43, 1, 99 }, { 43, 2, 100 }, { 43, 3, 101 },
> +	{ 44, 0, 102 }, { 44, 1, 103 }, { 44, 2, 104 }, { 44, 3, 105 },
> +	{ 45, 0, 106 }, { 45, 1, 107 }, { 45, 2, 108 }, { 45, 3, 109 },
> +	{ 46, 0, 110 }, { 46, 1, 111 }, { 46, 2, 112 }, { 46, 3, 113 },
> +	{ 47, 0, 114 }, { 47, 1, 115 }, { 47, 2, 116 }, { 47, 3, 117 },
> +	{ 48, 0, 118 }, { 48, 1, 119 }, { 48, 2, 120 }, { 48, 3, 121 }, { 48, 3, 122 },
> +};

Is there any reason why this information is not kept in DT instead?
What does it describe exactly?

> +
> +static int rzg2l_gpio_irq_validate_id(u32 port, u32 bit)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(gpio_pin_info); i++) {
> +		if (gpio_pin_info[i].port == port && gpio_pin_info[i].bit == bit)
> +			return gpio_pin_info[i].id;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static struct irq_domain *rzg2l_get_pinctrl_domain(struct device *dev)
> +{
> +	struct device_node *pinctrl_np;
> +	struct irq_domain *pinctrl_domain;
> +
> +	pinctrl_np = of_find_compatible_node(NULL, NULL, "renesas,r9a07g044-pinctrl");
> +	if (!pinctrl_np)
> +		return NULL;
> +
> +	pinctrl_domain = irq_find_host(pinctrl_np);
> +	of_node_put(pinctrl_np);
> +
> +	return pinctrl_domain;

What if you have multiple instances of this pinctrl? The information
should directly come from DT in the form of a phandle.

> +}
> +
> +static struct irqc_priv *irq_data_to_priv(struct irq_data *data)
> +{
> +	return data->domain->host_data;
> +}
> +
> +static void rzg2l_tint_set_edge(struct irqc_priv *priv,
> +				u32 bit, unsigned int irq_type)
> +{
> +	u32 port_bit = bit;
> +	u32 reg;
> +
> +	if (port_bit <= 15) {
> +		reg = readl(priv->iomem + TITSR0);
> +		reg &= ~(IRQ_MASK << (port_bit * 2));
> +		reg |= irq_type << (port_bit * 2);
> +		writel(reg, priv->iomem + TITSR0);
> +	} else {
> +		reg = readl(priv->iomem + TITSR1);
> +		port_bit = port_bit / 2;
> +		reg &= ~(IRQ_MASK << (port_bit * 2));
> +		reg |= irq_type << (port_bit * 2);
> +		writel(reg, priv->iomem + TITSR1);
> +	}

This will result in corruption if two interrupts change their type at
the same time. Also, please use relaxed accessors.

> +}
> +
> +static int rzg2l_tint_set_type(struct irq_data *d, unsigned int type)
> +{
> +	struct rzg2l_pin_info *pin_info = d->chip_data;
> +	struct irqc_priv *priv = irq_data_to_priv(d);
> +	u8 irq_type;
> +
> +	pin_info->trigger_type = type & GENMASK(15, 0);
> +	pin_info->pin_state = ((type & ~GENMASK(15, 0)) >> 15);

This shift is obviously wrong. Also, why are you tracking random
information independently of the core?

> +	pin_info->type = pin_info->trigger_type;

More pointless tracking...

> +
> +	if (pin_info->trigger_type & BIT(0))
> +		irq_type = RISING_EDGE;
> +	else if (pin_info->trigger_type & BIT(1))
> +		irq_type = FALLING_EDGE;
> +	else if (pin_info->trigger_type & BIT(2))
> +		irq_type = HIGH_LEVEL;
> +	else if (pin_info->trigger_type & BIT(3))
> +		irq_type = LOW_LEVEL;
> +	else if (pin_info->trigger_type & BIT(4))
> +		irq_type = pin_info->pin_state ? FALLING_EDGE : RISING_EDGE;
> +	else
> +		return -EINVAL;

... considering that this can fail. And please use the macros that
already exist rather than these BIT(x).

> +
> +	pin_info->trigger_type = irq_type;
> +	rzg2l_tint_set_edge(priv, pin_info->bit, irq_type);
> +
> +	return 0;
> +}
> +
> +static irqreturn_t rzg2l_irqc_tint_irq_handler(int irq, void *dev_id)
> +{
> +	struct irq_domain *pctl_irq_domain;
> +	struct rzg2l_pin_info *pin_info;
> +	struct irqc_irq *i = dev_id;
> +	struct irqc_priv *priv = i->p;
> +	struct irq_data *irq_data;
> +	unsigned int offset;
> +	u32 reg;
> +
> +	offset = irq - priv->irq[RZG2L_TINT_IRQ_START_INDEX].requested_irq;
> +	pin_info = priv->irq[offset + RZG2L_TINT_IRQ_START_INDEX].pin;
> +	if (!pin_info)
> +		return IRQ_NONE;
> +
> +	irq_data = irq_domain_get_irq_data(priv->parent_domain, i->requested_irq);
> +	if (!irq_data)
> +		return IRQ_NONE;
> +
> +	pctl_irq_domain = rzg2l_get_pinctrl_domain(priv->dev);

Are you really comfortable with parsing the device tree on each
interrupt you get? This is madness. It also makes no sense to resolve
an interrupt in a foreign irqdomain. Why don't you allocate yours?

> +	if (!pctl_irq_domain)
> +		return IRQ_NONE;
> +
> +	reg = readl(priv->iomem + TSCR);
> +	reg &= ~BIT(offset);
> +	writel(reg, priv->iomem + TSCR);
> +
> +	generic_handle_irq(irq_find_mapping(pctl_irq_domain,
> +					    ((pin_info->port * 8) + pin_info->bit)));

generic_handle_domain_irq() is your new friend.

> +
> +	if (pin_info->type & BIT(4)) {
> +		pin_info->trigger_type = (pin_info->trigger_type == FALLING_EDGE) ?
> +					RISING_EDGE : FALLING_EDGE;
> +		rzg2l_tint_set_edge(priv, pin_info->bit, pin_info->trigger_type);
> +	}

This has hardly any chance of working reliably. Yes, plenty of drivers
do that. They are all doomed to fail.

> +
> +	return IRQ_HANDLED;

No way. This isn't a standard interrupt handler. This is a mux, with a
number of inputs for each of the *49* outputs. Please implement this
as a chained interrupt controller.

> +}
> +
> +static void rzg2l_tint_irq_disable(struct irq_data *d)
> +{
> +	struct rzg2l_pin_info *pin_info = d->chip_data;
> +	struct irqc_priv *priv = irq_data_to_priv(d);
> +	u32 reg;
> +
> +	reg = readl(priv->iomem + TSSR(pin_info->id / 4));
> +	reg &= ~(GENMASK(7, 0) << (pin_info->id % 4));
> +	writel(reg, priv->iomem + TSSR(pin_info->id / 4));

More buggy RMW, magic numbers all over the shop. Does this act as a
disable (pending state is lost) or as a mask (pending state is
preserved)?

> +}
> +
> +static void rzg2l_tint_irq_enable(struct irq_data *d)
> +{
> +	struct rzg2l_pin_info *pin_info = d->chip_data;
> +	struct irqc_priv *priv = irq_data_to_priv(d);
> +	u32 gpioint;
> +	u32 reg;
> +
> +	gpioint = rzg2l_gpio_irq_validate_id(pin_info->port, pin_info->bit);
> +
> +	reg = readl(priv->iomem + TSSR(pin_info->id / 4));
> +	reg |= (BIT(7) | gpioint) << (8 * (pin_info->id % 4));
> +	writel(reg, priv->iomem + TSSR(pin_info->id / 4));

On top of the buggy RMW, what happens when
rzg2l_gpio_irq_validate_id() returns -EINVAL?

> +}
> +
> +static int rzg2l_irqc_domain_translate(struct irq_domain *domain,
> +				       struct irq_fwspec *fwspec,
> +				       unsigned long *hwirq,
> +				       unsigned int *type)
> +{
> +	if (WARN_ON(fwspec->param_count < 2))
> +		return -EINVAL;
> +
> +	*hwirq = fwspec->param[0];
> +	*type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
> +
> +	return 0;
> +}

This is irq_domain_translate_twocell().

> +
> +static int rzg2l_irqc_domain_alloc(struct irq_domain *domain,
> +				   unsigned int virq, unsigned int nr_irqs,
> +				   void *arg)
> +{
> +	struct irqc_priv *priv = domain->host_data;
> +	struct irq_fwspec parent_fwspec;
> +	struct rzg2l_pin_info *pin_info;
> +	struct irq_data *irq_data;
> +	irq_hw_number_t hwirq;
> +	unsigned int type;
> +	int gpioint;
> +	u32 port;
> +	int irq;
> +	int ret;
> +	u8 bit;
> +
> +	ret = rzg2l_irqc_domain_translate(domain, arg, &hwirq, &type);
> +	if (ret)
> +		return ret;
> +
> +	port = RZG2L_PIN_ID_TO_PORT(hwirq);
> +	bit = RZG2L_PIN_ID_TO_PIN(hwirq);
> +
> +	switch (type) {
> +	case IRQ_TYPE_EDGE_RISING:
> +	case IRQ_TYPE_LEVEL_HIGH:
> +		break;
> +	case IRQ_TYPE_EDGE_FALLING:
> +		type = IRQ_TYPE_EDGE_RISING;
> +		break;
> +	case IRQ_TYPE_LEVEL_LOW:
> +		type = IRQ_TYPE_LEVEL_HIGH;

Really? How is that handled?

I stopped reading at this stage. This needs to be redesigned from the
ground up as an interrupt controller instead of an interrupt handler,
have proper locking, data structures that are not redundant with that
of the core code, and be written in a way that is self documenting.

Thanks,

	M.

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