Message-ID: <87h7czam5d.wl-maz@kernel.org>
Date:   Fri, 29 Oct 2021 16:25:18 +0100
From:   Marc Zyngier <maz@...nel.org>
To:     Qin Jian <qinjian@...lus1.com>
Cc:     robh+dt@...nel.org, mturquette@...libre.com, sboyd@...nel.org,
        tglx@...utronix.de, p.zabel@...gutronix.de, broonie@...nel.org,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
        wells.lu@...plus.com
Subject: Re: [PATCH v2 8/8] irqchip: Add support for Sunplus SP7021 interrupt controller

On Fri, 29 Oct 2021 09:44:34 +0100,
Qin Jian <qinjian@...lus1.com> wrote:
> 
> Add interrupt driver for Sunplus SP7021 SoC.
> 
> This is the interrupt controller in P chip which collects all interrupt
> sources in P-chip and routes them to C-chip. C-chip adopts ARM CA7
> interrupt controller (compitable = "arm,cortex-a7-gic"). It is a parent

This information isn't relevant.

> interrupt controller of P-chip interrupt controller.
> 
> Signed-off-by: Qin Jian <qinjian@...lus1.com>
> ---
>  MAINTAINERS                       |   1 +
>  drivers/irqchip/Kconfig           |   9 +
>  drivers/irqchip/Makefile          |   1 +
>  drivers/irqchip/irq-sp7021-intc.c | 324 ++++++++++++++++++++++++++++++
>  4 files changed, 335 insertions(+)
>  create mode 100644 drivers/irqchip/irq-sp7021-intc.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index be0334d6a..bfa891d86 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2665,6 +2665,7 @@ F:	Documentation/devicetree/bindings/clock/sunplus,sp7021-clkc.yaml
>  F:	Documentation/devicetree/bindings/interrupt-controller/sunplus,sp7021-intc.yaml
>  F:	Documentation/devicetree/bindings/reset/sunplus,reset.yaml
>  F:	drivers/clk/clk-sp7021.c
> +F:	drivers/irqchip/irq-sp7021-intc.c
>  F:	drivers/reset/reset-sunplus.c
>  F:	include/dt-bindings/clock/sp-sp7021.h
>  F:	include/dt-bindings/interrupt-controller/sp7021-intc.h
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index aca7b595c..8a58dfb88 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -602,4 +602,13 @@ config APPLE_AIC
>  	  Support for the Apple Interrupt Controller found on Apple Silicon SoCs,
>  	  such as the M1.
>  
> +config SUNPLUS_SP7021_INTC
> +	bool "Sunplus SP7021 interrupt controller"
> +	default SOC_SP7021
> +	help
> +	  Support for the Sunplus SP7021 Interrupt Controller IP core.
> +	  This is used as a primary controller with SP7021 ChipP and can also
> +	  be used as a secondary chained controller on SP7021 ChipC.
> +	  This is selected automatically by platform config.

If it is selected, drop the default.

> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index f88cbf36a..75411f654 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -116,3 +116,4 @@ obj-$(CONFIG_MACH_REALTEK_RTL)		+= irq-realtek-rtl.o
>  obj-$(CONFIG_WPCM450_AIC)		+= irq-wpcm450-aic.o
>  obj-$(CONFIG_IRQ_IDT3243X)		+= irq-idt3243x.o
>  obj-$(CONFIG_APPLE_AIC)			+= irq-apple-aic.o
> +obj-$(CONFIG_SUNPLUS_SP7021_INTC)	+= irq-sp7021-intc.o
> diff --git a/drivers/irqchip/irq-sp7021-intc.c b/drivers/irqchip/irq-sp7021-intc.c
> new file mode 100644
> index 000000000..3431ec746
> --- /dev/null
> +++ b/drivers/irqchip/irq-sp7021-intc.c
> @@ -0,0 +1,324 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Copyright (C) Sunplus Technology Co., Ltd.
> + *       All rights reserved.
> + */
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/io.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#include <asm/exception.h>
> +#include <dt-bindings/interrupt-controller/sp7021-intc.h>
> +
> +#define SP_INTC_HWIRQ_MIN     0
> +#define SP_INTC_HWIRQ_MAX     223
> +
> +/* Interrupt G0/G1 offset */
> +#define INTR_REG_SIZE		(7 * 4)

Why isn't that derived from the number of interrupts?

> +
> +#define G0_INTR_TYPE		(0)
> +#define G0_INTR_POLARITY	(G0_INTR_TYPE + INTR_REG_SIZE)
> +#define G0_INTR_PRIORITY	(G0_INTR_POLARITY + INTR_REG_SIZE)
> +#define G0_INTR_MASK		(G0_INTR_PRIORITY + INTR_REG_SIZE)
> +
> +#define G1_INTR_CLR			(0)
> +#define G1_MASKED_EXT1		(G1_INTR_CLR + INTR_REG_SIZE)
> +#define G1_MASKED_EXT0		(G1_MASKED_EXT1 + INTR_REG_SIZE)
> +#define G1_INTR_GROUP		(31 * 4)
> +#define G1_INTR_MASK		(0x7F)
> +#define G1_EXT1_SHIFT		(0)
> +#define G1_EXT0_SHIFT		(8)
> +
> +static struct sp_intctl {
> +	void __iomem *g0;
> +	void __iomem *g1;

What is G0? what is G1?

> +	struct irq_domain *domain;
> +	struct device_node *node;
> +	raw_spinlock_t lock;
> +	int virq[2];
> +} sp_intc;
> +
> +/* GPIO INT EDGE BUG WORKAROUND */
> +#define GPIO_INT0_HWIRQ			120
> +#define GPIO_INT7_HWIRQ			127
> +#define GPIO_INT_EDGE_ACTIVE	BIT(31)
> +#define IS_GPIO_INT(n)	(((n) >= GPIO_INT0_HWIRQ) && ((n) <= GPIO_INT7_HWIRQ))
> +/* array to hold which interrupt needs to workaround the bug
> + * INT_TYPE_NONE: no need
> + * INT_TYPE_EDGE_FALLING/INT_TYPE_EDGE_RISING: need to workaround
> + * GPIO_INT_EDGE_ACTIVING: workaround is on going

Please describe the nature of the workaround. s/ACTIVING/ACTIVE/.

> + */
> +static u32 edge_trigger[SP_INTC_HWIRQ_MAX];

4 states per interrupt, and yet you use a u32 for each of them...
Also, why isn't this part of your sp_intc data structure? You also
have enough space for 200+ interrupts (ignoring the obvious off-by-one
bug), and yet you are only concerned with 8 of them. Do you spot a
trend here?

> +
> +static struct irq_chip sp_intc_chip;
> +
> +static void sp_intc_assign_bit(u32 hwirq, void __iomem *base, u32 value)

If value describes a single bit, why is it a u32?

> +{
> +	u32 offset, mask;
> +	unsigned long flags;
> +	void __iomem *reg;
> +
> +	offset = (hwirq / 32) * 4;
> +	reg = base + offset;
> +
> +	raw_spin_lock_irqsave(&sp_intc.lock, flags);
> +	mask = readl_relaxed(reg);
> +	if (value)
> +		mask |= BIT(hwirq % 32);
> +	else
> +		mask &= ~BIT(hwirq % 32);
> +	writel_relaxed(mask, reg);
> +	raw_spin_unlock_irqrestore(&sp_intc.lock, flags);
> +}
> +
> +static void sp_intc_ack_irq(struct irq_data *d)
> +{
> +	u32 hwirq = d->hwirq;
> +
> +	if (edge_trigger[hwirq] != IRQ_TYPE_NONE) {

Why don't you just check the irq number instead?

> +		sp_intc_assign_bit(hwirq, sp_intc.g0 + G0_INTR_POLARITY,
> +			(edge_trigger[hwirq] == IRQ_TYPE_EDGE_RISING));
> +		edge_trigger[hwirq] |= GPIO_INT_EDGE_ACTIVE;

The whole thing is terrible. For each of the 8 interrupts, you only
need to know whether:

- it is rising or falling
- it is active or not

That's a grand total of 16 bits instead of almost a 1kB worth of
nothing. Use atomic bitops, and this is done.

> +	}
> +
> +	sp_intc_assign_bit(hwirq, sp_intc.g1 + G1_INTR_CLR, 1);
> +}
> +
> +static void sp_intc_mask_irq(struct irq_data *d)
> +{
> +	sp_intc_assign_bit(d->hwirq, sp_intc.g0 + G0_INTR_MASK, 0);
> +}
> +
> +static void sp_intc_unmask_irq(struct irq_data *d)
> +{
> +	sp_intc_assign_bit(d->hwirq, sp_intc.g0 + G0_INTR_MASK, 1);
> +}
> +
> +static void sp_intc_set_priority(u32 hwirq, u32 priority)
> +{
> +	sp_intc_assign_bit(hwirq, sp_intc.g0 + G0_INTR_PRIORITY, priority);
> +}
> +
> +static int sp_intc_set_type(struct irq_data *d, unsigned int type)
> +{
> +	u32 intr_type;		/* 0: level			1: edge */
> +	u32 intr_polarity;	/* 0: high/rising	1: low/falling */

So how about giving the variables sensible names and types:

	bool is_level, is_high;

> +	u32 hwirq = d->hwirq;
> +
> +	/* update the chip/handler */
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		irq_set_chip_handler_name_locked(d, &sp_intc_chip,
> +						   handle_level_irq, NULL);
> +	else
> +		irq_set_chip_handler_name_locked(d, &sp_intc_chip,
> +						   handle_edge_irq, NULL);
> +
> +	edge_trigger[hwirq] = IRQ_TYPE_NONE;
> +
> +	if (type & IRQ_TYPE_LEVEL_MASK)
> +		intr_type = 0;
> +	else if (IS_GPIO_INT(hwirq)) {
> +		intr_type = 0;
> +		edge_trigger[hwirq] = type;
> +	} else
> +		intr_type = 1;

Coding style.

> +
> +	intr_polarity = (type == IRQ_TYPE_LEVEL_LOW || type == IRQ_TYPE_EDGE_FALLING);
> +
> +	sp_intc_assign_bit(hwirq, sp_intc.g0 + G0_INTR_TYPE, intr_type);
> +	sp_intc_assign_bit(hwirq, sp_intc.g0 + G0_INTR_POLARITY, intr_polarity);
> +
> +	return IRQ_SET_MASK_OK;
> +}
> +
> +static int sp_intc_get_ext_irq(int ext_num)
> +{
> +	u32 hwirq, mask;
> +	u32 i;
> +
> +	i = readl_relaxed(sp_intc.g1 + G1_INTR_GROUP);
> +	if (ext_num)
> +		mask = (i >> G1_EXT1_SHIFT) & G1_INTR_MASK;
> +	else
> +		mask = (i >> G1_EXT0_SHIFT) & G1_INTR_MASK;
> +	if (mask) {
> +		i = fls(mask) - 1;
> +		if (ext_num)
> +			mask = readl_relaxed(sp_intc.g1 + G1_MASKED_EXT1 + i * 4);
> +		else
> +			mask = readl_relaxed(sp_intc.g1 + G1_MASKED_EXT0 + i * 4);
> +		if (mask) {
> +			hwirq = (i << 5) + fls(mask) - 1;
> +			return hwirq;
> +		}
> +	}

What a terrible control flow. Also, variable names are cheap, and you
don't need to reuse them when they mean something different.

	if (ext_num) {
		shift = G1_EXT1_SHIFT;
		offset = G1_MASKED_EXT1;
	} else {
		[[ same thing with G0 ]
	}

	status = readl_relaxed();

	pending_summary = (status >> shift) & G1_INTR_MASK;
	if (!pending_summary)
		return -1;

	index = fls(pending_summary) - 1;
	pending = readl_relaxed(g1 + offset + index * 4);
	if (!pending)
		return -1;

	return (index << 5) | (fls(pending) - 1);

Look: no nesting, obvious names, anyone can understand it.

> +	return -1; /* No interrupt */
> +}
> +
> +static void sp_intc_handle_ext_cascaded(struct irq_desc *desc)
> +{
> +	struct irq_chip *host_chip = irq_desc_get_chip(desc);
> +	int ext_num = (int)irq_desc_get_handler_data(desc);
> +	int hwirq;
> +
> +	chained_irq_enter(host_chip, desc);
> +
> +	while ((hwirq = sp_intc_get_ext_irq(ext_num)) >= 0) {
> +		if (edge_trigger[hwirq] & GPIO_INT_EDGE_ACTIVE) {
> +			edge_trigger[hwirq] &= ~GPIO_INT_EDGE_ACTIVE;
> +			sp_intc_assign_bit(hwirq, sp_intc.g0 + G0_INTR_POLARITY,
> +				(edge_trigger[hwirq] != IRQ_TYPE_EDGE_RISING));
> +		} else
> +			generic_handle_domain_irq(sp_intc.domain, hwirq);

Coding style.

> +	}
> +
> +	chained_irq_exit(host_chip, desc);
> +}
> +
> +static void __exception_irq_entry sp_intc_handle_irq(struct pt_regs *regs)
> +{
> +	int hwirq;
> +
> +	while ((hwirq = sp_intc_get_ext_irq(0)) >= 0)
> +		generic_handle_domain_irq(sp_intc.domain, hwirq);
> +}
> +
> +static void __init sp_intc_chip_init(void __iomem *base0, void __iomem *base1)
> +{
> +	int i;
> +
> +	sp_intc.g0 = base0;
> +	sp_intc.g1 = base1;
> +
> +	for (i = 0; i < 7; i++) {
> +		/* all mask */
> +		writel_relaxed(0, sp_intc.g0 + G0_INTR_MASK + i * 4);
> +		/* all edge */
> +		writel_relaxed(~0, sp_intc.g0 + G0_INTR_TYPE + i * 4);
> +		/* all high-active */
> +		writel_relaxed(0, sp_intc.g0 + G0_INTR_POLARITY + i * 4);
> +		/* all irq */
> +		writel_relaxed(~0, sp_intc.g0 + G0_INTR_PRIORITY + i * 4);
> +		/* all clear */
> +		writel_relaxed(~0, sp_intc.g1 + G1_INTR_CLR + i * 4);
> +	}
> +}
> +
> +int sp_intc_xlate_of(struct irq_domain *d, struct device_node *node,
> +			  const u32 *intspec, unsigned int intsize,
> +			  irq_hw_number_t *out_hwirq, unsigned int *out_type)
> +{
> +	int ret;
> +
> +	ret = irq_domain_xlate_twocell(d, node,
> +		intspec, intsize, out_hwirq, out_type);
> +	if (!ret) {
> +		/* intspec[1]: IRQ_TYPE | SP_INTC_EXT_INT
> +		 * SP_INTC_EXT_INT: 0-1,
> +		 *   to indicate route to which parent irq: EXT_INT0/EXT_INT1
> +		 */

Why is that in the DT? If that's a SW decision, it doesn't belong there.

> +		u32 ext_int = (intspec[1] & SP_INTC_EXT_INT_MASK) >> SP_INTC_EXT_INT_SHFIT;
> +
> +		/* priority = 0, route to EXT_INT1
> +		 *    otherwise, route to EXT_INT0
> +		 */
> +		sp_intc_set_priority(*out_hwirq, 1 - ext_int);

I already said in the initial review that changing the HW didn't
belong in the translate callback, but should be done at map() time.

> +	}
> +
> +	return ret;
> +}
> +
> +static struct irq_chip sp_intc_chip = {
> +	.name = "sp_intc",
> +	.irq_ack = sp_intc_ack_irq,
> +	.irq_mask = sp_intc_mask_irq,
> +	.irq_unmask = sp_intc_unmask_irq,
> +	.irq_set_type = sp_intc_set_type,
> +};
> +
> +static int sp_intc_irq_domain_map(struct irq_domain *domain,
> +	unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	irq_set_chip_and_handler(irq, &sp_intc_chip, handle_level_irq);
> +	irq_set_chip_data(irq, &sp_intc_chip);
> +	irq_set_noprobe(irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops sp_intc_dm_ops = {
> +	.xlate = sp_intc_xlate_of,
> +	.map = sp_intc_irq_domain_map,
> +};
> +
> +#ifdef CONFIG_OF

Why the #ifdef? This thing doesn't work without OF anyway.

> +static int sp_intc_irq_map(struct device_node *node, int i)
> +{
> +	sp_intc.virq[i] = irq_of_parse_and_map(node, i);
> +	if (!sp_intc.virq[i]) {
> +		pr_err("%s: missed EXT_INT%d in DT\n", __func__, i);

-ENOENT is enough, no need to shout on the console.

> +		return -ENOENT;
> +	}
> +
> +	pr_info("%s: EXT_INT%d = %d\n", __func__, i, sp_intc.virq[i]);

Nobody cares about this. Get rid of it.

> +	irq_set_chained_handler_and_data(sp_intc.virq[i],
> +		sp_intc_handle_ext_cascaded, (void *)i);
> +
> +	return 0;
> +}
> +
> +int __init sp_intc_init_dt(
> +	struct device_node *node, struct device_node *parent)

Single line.

> +{
> +	void __iomem *base0, *base1;
> +
> +	base0 = of_iomap(node, 0);
> +	if (!base0) {
> +		pr_err("unable to map sp-intc base 0\n");
> +		return -EINVAL;
> +	}
> +
> +	base1 = of_iomap(node, 1);
> +	if (!base1) {
> +		pr_err("unable to map sp-intc base 1\n");
> +		return -EINVAL;
> +	}
> +
> +	sp_intc.node = node;
> +
> +	sp_intc_chip_init(base0, base1);
> +
> +	sp_intc.domain = irq_domain_add_linear(node,
> +			SP_INTC_HWIRQ_MAX - SP_INTC_HWIRQ_MIN,
> +			&sp_intc_dm_ops, &sp_intc);
> +	if (!sp_intc.domain) {
> +		pr_err("%s: unable to create linear domain\n", __func__);

Drop the error message.

> +		return -EINVAL;
> +	}
> +
> +	raw_spin_lock_init(&sp_intc.lock);
> +
> +	if (parent) {
> +		/* secondary chained controller */
> +		if (sp_intc_irq_map(node, 0)) // EXT_INT0
> +			return -ENOENT;

Just return whatever the helper returned. You don't need to
reinterpret it.

> +
> +		if (sp_intc_irq_map(node, 1)) // EXT_INT1
> +			return -ENOENT;
> +	} else {
> +		/* primary controller */
> +		set_handle_irq(sp_intc_handle_irq);
> +	}

So what happens if you have *two* of these blocks? One as the root
controller, and another implementing the chained stuff?

> +
> +	return 0;
> +}
> +IRQCHIP_DECLARE(sp_intc, "sunplus,sp7021-intc", sp_intc_init_dt);
> +#endif
> +
> +MODULE_AUTHOR("Qin Jian <qinjian@...lus1.com>");
> +MODULE_DESCRIPTION("Sunplus SP7021 Interrupt Controller Driver");
> +MODULE_LICENSE("GPL v2");

You are using IRQCHIP_DECLARE(), so this isn't a module. Drop this.

Thankfully, it is too late for 5.16, so you have a full cycle to give
this code another major cleanup.

	M.

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

Powered by blists - more mailing lists