Re: [PATCH 08/14] irqchip: Add driver for WPCM450 interrupt controller

[Marc Zyngier <maz@kernel.org>]

On Sat, 20 Mar 2021 18:16:04 +0000,
Jonathan Neuschäfer <j.neuschaefer@gmx.net> wrote:
> 
> The WPCM450 AIC ("Advanced Interrupt Controller") is the interrupt
> controller found in the Nuvoton WPCM450 SoC and other Winbond/Nuvoton
> SoCs.
> 
> The list of registers if based on the AMI vendor kernel and the
> Nuvoton W90N745 datasheet.
> 
> Although the hardware supports other interrupt modes, the driver only
> supports high-level interrupts at the moment, because other modes could
> not be tested so far.
> 
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> ---
>  arch/arm/mach-npcm/Kconfig        |   1 +
>  drivers/irqchip/Kconfig           |   6 ++
>  drivers/irqchip/Makefile          |   1 +
>  drivers/irqchip/irq-wpcm450-aic.c | 162 ++++++++++++++++++++++++++++++
>  4 files changed, 170 insertions(+)
>  create mode 100644 drivers/irqchip/irq-wpcm450-aic.c
> 
> diff --git a/arch/arm/mach-npcm/Kconfig b/arch/arm/mach-npcm/Kconfig
> index 658c8efb4ca14..a71cf1d189ae5 100644
> --- a/arch/arm/mach-npcm/Kconfig
> +++ b/arch/arm/mach-npcm/Kconfig
> @@ -10,6 +10,7 @@ config ARCH_WPCM450
>  	bool "Support for WPCM450 BMC (Hermon)"
>  	depends on ARCH_MULTI_V5
>  	select CPU_ARM926T
> +	select WPCM450_AIC
>  	select NPCM7XX_TIMER
>  	help
>  	  General support for WPCM450 BMC (Hermon).
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index e74fa206240a1..baf4efec31c67 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -586,4 +586,10 @@ config MST_IRQ
>  	help
>  	  Support MStar Interrupt Controller.
> 
> +config WPCM450_AIC
> +	bool "Nuvoton WPCM450 Advanced Interrupt Controller"
> +	depends on ARCH_WPCM450 || COMPILE_TEST
> +	help
> +	  Support for the interrupt controller in the Nuvoton WPCM450 BMC SoC.
> +
>  endmenu
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index c59b95a0532c9..bef57937e7296 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -113,3 +113,4 @@ obj-$(CONFIG_LOONGSON_PCH_MSI)		+= irq-loongson-pch-msi.o
>  obj-$(CONFIG_MST_IRQ)			+= irq-mst-intc.o
>  obj-$(CONFIG_SL28CPLD_INTC)		+= irq-sl28cpld.o
>  obj-$(CONFIG_MACH_REALTEK_RTL)		+= irq-realtek-rtl.o
> +obj-$(CONFIG_WPCM450_AIC)		+= irq-wpcm450-aic.o
> diff --git a/drivers/irqchip/irq-wpcm450-aic.c b/drivers/irqchip/irq-wpcm450-aic.c
> new file mode 100644
> index 0000000000000..0d6dd8b1fc824
> --- /dev/null
> +++ b/drivers/irqchip/irq-wpcm450-aic.c
> @@ -0,0 +1,162 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +// Copyright 2021 Jonathan Neuschäfer
> +
> +#include <linux/console.h>

That's unexpected. Why do you need this?

> +#include <linux/irqchip.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#include <asm/exception.h>
> +
> +#define AIC_SCR(x)	((x)*4)	/* Source control registers */
> +#define AIC_GEN		0x84
> +#define AIC_GRSR	0x88
> +#define AIC_IRSR	0x100	/* Interrupt raw status register */
> +#define AIC_IASR	0x104	/* Interrupt active status register */
> +#define AIC_ISR		0x108	/* Interrupt status register */
> +#define AIC_IPER	0x10c	/* Interrupt priority encoding register */
> +#define AIC_ISNR	0x110	/* Interrupt source number register */
> +#define AIC_IMR		0x114	/* Interrupt mask register */
> +#define AIC_OISR	0x118	/* Output interrupt status register */
> +#define AIC_MECR	0x120	/* Mask enable command register */
> +#define AIC_MDCR	0x124	/* Mask disable command register */
> +#define AIC_SSCR	0x128	/* Source set command register */
> +#define AIC_SCCR	0x12c	/* Source clear command register */
> +#define AIC_EOSCR	0x130	/* End of service command register */
> +
> +#define AIC_SCR_SRCTYPE_LOW_LEVEL	(0 << 6)
> +#define AIC_SCR_SRCTYPE_HIGH_LEVEL	(1 << 6)
> +#define AIC_SCR_SRCTYPE_NEG_EDGE	(2 << 6)
> +#define AIC_SCR_SRCTYPE_POS_EDGE	(3 << 6)
> +#define AIC_SCR_PRIORITY(x)		(x)

A mask would be welcomed for this field.

> +
> +#define IRQS		32

Please use something a bit less generic.

> +
> +struct wpcm450_aic {
> +	void __iomem *regs;
> +	struct irq_domain *domain;
> +};
> +
> +static struct wpcm450_aic *aic;
> +
> +static void wpcm450_aic_init_hw(void)
> +{
> +	int i;
> +
> +	/* Disable (mask) all interrupts */
> +	writel(0xffffffff, aic->regs + AIC_MDCR);

Consider using relaxed accessors throughout this driver.

> +
> +	/*
> +	 * Make sure the interrupt controller is ready to serve new interrupts.
> +	 * Reading from IPER indicates that the nIRQ signal may be deasserted,
> +	 * and writing to EOSCR indicates that interrupt handling has finished.
> +	 */
> +	readl(aic->regs + AIC_IPER);
> +	writel(0, aic->regs + AIC_EOSCR);
> +
> +	/* Initialize trigger mode and priority of each interrupt source */
> +	for (i = 0; i < IRQS; i++)
> +		writel(AIC_SCR_SRCTYPE_HIGH_LEVEL | AIC_SCR_PRIORITY(7),
> +		       aic->regs + AIC_SCR(i));
> +}
> +
> +static void __exception_irq_entry wpcm450_aic_handle_irq(struct pt_regs *regs)
> +{
> +	int hwirq;
> +
> +	/* Determine the interrupt source */
> +	/* Read IPER to signal that nIRQ can be de-asserted */
> +	hwirq = readl(aic->regs + AIC_IPER) / 4;
> +
> +	handle_domain_irq(aic->domain, hwirq, regs);
> +}
> +
> +static void wpcm450_aic_ack(struct irq_data *d)
> +{
> +	/* Signal end-of-service */
> +	writel(0, aic->regs + AIC_EOSCR);

Is that an Ack or an EOI? My gut feeling is that the above read is the
Ack, and that this write should actually be an EOI callback.

> +}
> +
> +static void wpcm450_aic_mask(struct irq_data *d)
> +{
> +	unsigned int mask = 1U << d->hwirq;

Consider using BIT().

> +
> +	/* Disable (mask) the interrupt */
> +	writel(mask, aic->regs + AIC_MDCR);
> +}
> +
> +static void wpcm450_aic_unmask(struct irq_data *d)
> +{
> +	unsigned int mask = 1U << d->hwirq;
> +
> +	/* Enable (unmask) the interrupt */
> +	writel(mask, aic->regs + AIC_MECR);
> +}
> +
> +static int wpcm450_aic_set_type(struct irq_data *d, unsigned int flow_type)
> +{
> +	/*
> +	 * The hardware supports high/low level, as well as rising/falling edge
> +	 * modes, and the DT binding accommodates for that, but as long as
> +	 * other modes than high level mode are not used and can't be tested,
> +	 * they are rejected in this driver.
> +	 */
> +	if ((flow_type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_HIGH) {
> +		pr_err("IRQ mode %#x is not supported\n", flow_type);

The core kernel shouts loudly enough, no need for extra messages.

> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct irq_chip wpcm450_aic_chip = {
> +	.name = "wpcm450-aic",
> +	.irq_ack = wpcm450_aic_ack,
> +	.irq_mask = wpcm450_aic_mask,
> +	.irq_unmask = wpcm450_aic_unmask,
> +	.irq_set_type = wpcm450_aic_set_type,
> +};
> +
> +static int wpcm450_aic_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hwirq)
> +{
> +	if (hwirq > IRQS)
> +		return -EPERM;
> +
> +	irq_set_chip_and_handler(irq, &wpcm450_aic_chip, handle_level_irq);
> +	irq_set_chip_data(irq, aic);
> +	irq_set_probe(irq);
> +
> +	return 0;
> +}
> +
> +static const struct irq_domain_ops wpcm450_aic_ops = {
> +	.map = wpcm450_aic_map,
> +	.xlate = irq_domain_xlate_twocell,
> +};
> +
> +static int __init wpcm450_aic_of_init(struct device_node *node,
> +				      struct device_node *parent)
> +{
> +	if (parent)
> +		return -EINVAL;
> +
> +	aic = kzalloc(sizeof(*aic), GFP_KERNEL);
> +	if (!aic)
> +		return -ENOMEM;
> +
> +	aic->regs = of_iomap(node, 0);
> +	if (!aic->regs) {
> +		pr_err("Failed to map WPCM450 AIC registers\n");
> +		return -ENOMEM;
> +	}
> +
> +	wpcm450_aic_init_hw();
> +
> +	set_handle_irq(wpcm450_aic_handle_irq);
> +
> +	aic->domain = irq_domain_add_linear(node, IRQS, &wpcm450_aic_ops, aic);
> +
> +	return 0;
> +}
> +
> +IRQCHIP_DECLARE(wpcm450_aic, "nuvoton,wpcm450-aic", wpcm450_aic_of_init);

Otherwise, looks good.

	M.

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