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
