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

[Jonathan Neuschäfer]

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 <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>
> > ---
[...]
> > +// 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.

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

> > +
> > +#define IRQS		32
> 
> Please use something a bit less generic.

Ok, I'll rename it to AIC_NUM_IRQS.

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

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

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

Will do.

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

Attachment: signature.asc
Description: PGP signature