On Sat, 20 Mar 2021 18:16:04 +0000,
Jonathan Neuschäfer <j.neuschaefer@xxxxxxx> 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@xxxxxxx>
> ---
> 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.
