On Wed, Mar 24, 2021 at 05:16:35PM +0000, Marc Zyngier wrote:

On Sat, 20 Mar 2021 18:16:04 +0000,
Jonathan Neuschäfer [off-list ref] wrote:

quoted

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>
---

[...]

quoted

+// SPDX-License-Identifier: GPL-2.0-only
+// Copyright 2021 Jonathan Neuschäfer
+
+#include <linux/console.h>

That's unexpected. Why do you need this?

I forgot about linux/printk.h.

quoted

+#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.

Ok, I'll add

+#define AIC_SCR_PRIORITY_MASK           0x7

Should I apply it in AIC_SCR_PRIORITY(x), too?

quoted

+
+#define IRQS		32

Please use something a bit less generic.

Ok, I'll rename it to AIC_NUM_IRQS.

quoted

+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.

I'll read up on how to use them correctly.

quoted

+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.

I agree that EOSCR (End of Service Command Register) matches the
description of EOI.

The reading IPER serves a dual purpose, as indicated above. I could
move the IPER read to a separate irq_ack function and use ISNR
(Interrupt source number register) in the IRQ handler instead. This
should work (haven't tested it yet), but I'm not sure it's strictly
better.

quoted

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

Consider using BIT().

Will do.

quoted

+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.

Ok.

Otherwise, looks good.

	M.

Thanks for your review!
Jonathan Neuschäfer