Messages in this thread | | | From | Geert Uytterhoeven <> | Date | Fri, 29 Apr 2022 11:53:13 +0200 | Subject | Re: [PATCH 2/2] irqchip: Add RZ/G2L IA55 Interrupt Controller driver |
| |
Hi Prabhakar,
On Fri, Apr 29, 2022 at 11:43 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > On Thu, Apr 28, 2022 at 10:42 AM Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > On Fri, Apr 22, 2022 at 12:12 AM Lad Prabhakar > > <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > > Add a driver for the Renesas RZ/G2L Interrupt Controller. > > > > > > This supports external pins being used as interrupts. It supports > > > one line for NMI, 8 external pins and 32 GPIO pins (out of 123) > > > to be used as IRQ lines. > > > > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > --- /dev/null > > > +++ b/drivers/irqchip/irq-renesas-rzg2l.c > > > @@ -0,0 +1,447 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Renesas RZ/G2L IRQC Driver > > > + * > > > + * Copyright (C) 2022 Renesas Electronics Corporation. > > > + * > > > + * Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > + */ > > > + > > > +#include <linux/clk.h> > > > +#include <linux/err.h> > > > +#include <linux/io.h> > > > +#include <linux/irqchip.h> > > > +#include <linux/irqdomain.h> > > > +#include <linux/of_address.h> > > > +#include <linux/reset.h> > > > +#include <linux/spinlock.h> > > > + > > > +#define IRQC_IRQ_START 1 > > > +#define IRQC_IRQ_COUNT 8 > > > +#define IRQC_TINT_START 9 > > > > = IRQC_IRQ_START + IRQC_IRQ_COUNT > > > OK > > > > +#define IRQC_TINT_COUNT 32 > > > +#define IRQC_NUM_IRQ 41 > > > > = IRQC_TINT_START + IRQC_TINT_COUNT > > > OK. > > > Should these be in a DT binding header file? > > > > Combining all types into a single linear number space makes it hard > > to extend the range, when reusing for an SoC that supports more > > interrupt sources. > > > Or DT data maybe?
Let's leave it for now. As I missed that DT consumers will refer to external interrupt numbers only (is that actually enforced?), there won't be an issue.
The driver can be changed later to derive IRQC_IRQ_COUNT from the compatible value, when needed.
> > > + u32 reg; > > > + > > > + reg = readl_relaxed(priv->base + ISCR); > > > + if (reg & bit) > > > + writel_relaxed(GENMASK(IRQC_IRQ_COUNT - 1, 0) & ~bit, > > > > As writes to the unused upper bits are ignored, you can drop the > > masking with GENMASK(IRQC_IRQ_COUNT - 1, 0), and be prepared for more > > interrupt sources. > > > Agreed.
> > > + u32 bit = BIT(hw_irq - IRQC_TINT_START); > > > + u32 reg; > > > + > > > + reg = readl_relaxed(priv->base + TSCR); > > > + if (reg & bit) > > > + writel_relaxed(GENMASK(IRQC_TINT_COUNT - 1, 0) & ~bit, > > > > Drop the masking with all-ones? > > > You mean instead of a mask just use the reg instead?
No, I meant to drop the masking with GENMASK(IRQC_TINT_COUNT - 1, 0), cfr. for external interrupts.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
| |