From: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>
To: Marc Zyngier <maz@kernel.org>
Cc: "Jonathan Neuschäfer" <j.neuschaefer@gmx.net>,
openbmc@lists.ozlabs.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
"Avi Fishman" <avifishman70@gmail.com>,
"Tomer Maimon" <tmaimon77@gmail.com>,
"Tali Perry" <tali.perry1@gmail.com>,
"Patrick Venture" <venture@google.com>,
"Nancy Yuen" <yuenn@google.com>,
"Benjamin Fair" <benjaminfair@google.com>,
"Russell King" <linux@armlinux.org.uk>,
"Thomas Gleixner" <tglx@linutronix.de>
Subject: Re: [PATCH 08/14] irqchip: Add driver for WPCM450 interrupt controller
Date: Fri, 26 Mar 2021 19:52:07 +0100 [thread overview]
Message-ID: <YF4tV+L71Lso94kT@latitude> (raw)
In-Reply-To: <87sg4kiia4.wl-maz@kernel.org>
[-- Attachment #1: Type: text/plain, Size: 3546 bytes --]
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@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>
> > ---
[...]
> > +// 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2021-03-26 18:53 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-20 18:15 [PATCH 00/14] Initial support for Nuvoton WPCM450 BMC SoC Jonathan Neuschäfer
2021-03-20 18:15 ` [PATCH 01/14] dt-bindings: vendor-prefixes: Add Supermicro Jonathan Neuschäfer
2021-03-27 16:36 ` Rob Herring
2021-03-20 18:15 ` [PATCH 02/14] dt-bindings: arm: npcm: Add nuvoton,wpcm450 compatible string Jonathan Neuschäfer
2021-03-27 16:36 ` [PATCH 02/14] dt-bindings: arm: npcm: Add nuvoton, wpcm450 " Rob Herring
2021-03-20 18:15 ` [PATCH 03/14] dt-bindings: interrupt-controller: Add nuvoton,wpcm450-aic Jonathan Neuschäfer
2021-03-27 16:37 ` [PATCH 03/14] dt-bindings: interrupt-controller: Add nuvoton, wpcm450-aic Rob Herring
2021-03-20 18:16 ` [PATCH 04/14] dt-bindings: serial: 8250: Add nuvoton,wpcm450-uart Jonathan Neuschäfer
2021-03-27 16:37 ` Rob Herring
2021-03-20 18:16 ` [PATCH 05/14] dt-bindings: timer: nuvoton,npcm7xx: Add wpcm450-timer Jonathan Neuschäfer
2021-03-27 16:39 ` Rob Herring
2021-04-04 20:28 ` Daniel Lezcano
2021-04-09 10:27 ` [tip: timers/core] " tip-bot2 for Jonathan Neuschäfer
2021-03-20 18:16 ` [PATCH 06/14] dt-bindings: watchdog: npcm: Add nuvoton,wpcm450-wdt Jonathan Neuschäfer
2021-03-27 16:39 ` Rob Herring
2021-03-20 18:16 ` [PATCH 07/14] ARM: npcm: Introduce Nuvoton WPCM450 SoC Jonathan Neuschäfer
2021-03-20 18:16 ` [PATCH 08/14] irqchip: Add driver for WPCM450 interrupt controller Jonathan Neuschäfer
2021-03-24 17:16 ` Marc Zyngier
2021-03-26 18:52 ` Jonathan Neuschäfer [this message]
2021-04-05 17:25 ` Jonathan Neuschäfer
2021-03-20 18:16 ` [PATCH 09/14] serial: 8250_of: Add nuvoton,wpcm450-uart Jonathan Neuschäfer
2021-03-20 18:16 ` [PATCH 10/14] clocksource/drivers/npcm: Add support for WPCM450 Jonathan Neuschäfer
2021-03-22 10:46 ` Daniel Lezcano
2021-04-09 10:27 ` [tip: timers/core] " tip-bot2 for Jonathan Neuschäfer
2021-03-20 18:16 ` [PATCH 11/14] watchdog: npcm: " Jonathan Neuschäfer
2021-03-20 20:24 ` Guenter Roeck
2021-03-20 20:43 ` Jonathan Neuschäfer
2021-03-20 18:16 ` [PATCH 12/14] ARM: dts: Add devicetree for Nuvoton WPCM450 BMC chip Jonathan Neuschäfer
2021-03-20 18:16 ` [PATCH 13/14] ARM: dts: Add devicetree for Supermicro X9SCi-LN4F based on WPCM450 Jonathan Neuschäfer
2021-03-20 18:44 ` Jonathan Neuschäfer
2021-03-20 18:16 ` [PATCH 14/14] MAINTAINERS: Nuvoton NPCM: Add wpcm patterns Jonathan Neuschäfer
2021-03-20 18:47 ` [PATCH 00/14] Initial support for Nuvoton WPCM450 BMC SoC Jonathan Neuschäfer
[not found] ` <CAP6Zq1jdO_kw-B-SX0VNiVqQ1rz1vbt+DJ1quvm286+cbKec1Q@mail.gmail.com>
2021-03-21 12:31 ` Jonathan Neuschäfer
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YF4tV+L71Lso94kT@latitude \
--to=j.neuschaefer@gmx.net \
--cc=avifishman70@gmail.com \
--cc=benjaminfair@google.com \
--cc=devicetree@vger.kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=maz@kernel.org \
--cc=openbmc@lists.ozlabs.org \
--cc=tali.perry1@gmail.com \
--cc=tglx@linutronix.de \
--cc=tmaimon77@gmail.com \
--cc=venture@google.com \
--cc=yuenn@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).