Re: [PATCH v2 2/2] irqchip/sifive-plic: Add support for Renesas RZ/Five SoC

[Marc Zyngier <maz@kernel.org>]

On Sun, 26 Jun 2022 10:38:18 +0100,
"Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> 
> Hi Marc,
> 
> Thank you for the review.
> 
> On Sun, Jun 26, 2022 at 9:56 AM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Sun, 26 Jun 2022 01:43:26 +0100,
> > Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote:
> > >
> > > The Renesas RZ/Five SoC has a RISC-V AX45MP AndesCore with NCEPLIC100. The
> > > NCEPLIC100 supports both edge-triggered and level-triggered interrupts. In
> > > case of edge-triggered interrupts NCEPLIC100 ignores the next interrupt
> > > edge until the previous completion message has been received and
> > > NCEPLIC100 doesn't support pending interrupt counter, hence losing the
> > > interrupts if not acknowledged in time.
> > >
> > > So the workaround for edge-triggered interrupts to be handled correctly
> > > and without losing is that it needs to be acknowledged first and then
> > > handler must be run so that we don't miss on the next edge-triggered
> > > interrupt.
> > >
> > > This patch adds a new compatible string for Renesas RZ/Five SoC and adds
> > > support to change interrupt flow based on the interrupt type. It also
> > > implements irq_ack and irq_set_type callbacks.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > ---
> > > v1->v2:
> > > * Implemented IRQ flow as suggested by Marc
> > >
> > > RFC-->v1:
> > > * Fixed review comments pointed by Geert
> > > * Dropped handle_fasteoi_ack_irq support as for the PLIC we need to
> > > claim the interrupt by reading the register and then acknowledge it.
> > > * Add a new chained handler for RZ/Five SoC.
> > > ---
> > >  drivers/irqchip/Kconfig           |  1 +
> > >  drivers/irqchip/irq-sifive-plic.c | 73 ++++++++++++++++++++++++++++++-
> > >  2 files changed, 72 insertions(+), 2 deletions(-)
> >

[...]

> > > +     if (of_device_is_compatible(node, "renesas,r9a07g043-plic")) {
> > > +             priv->of_data = RENESAS_R9A07G043_PLIC;
> > > +             plic_chip.name = "Renesas RZ/Five PLIC";
> >
> > NAK. The irq_chip structure isn't the place for platform marketing.
> > This is way too long anyway (and same for the edge version), and you
> > even sent me a patch to make that structure const...
> >
> My bad will drop this.

And why you're at it, please turn this rather random 'of_data' into
something like:

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index bb87e4c3b88e..cd1683b77caf 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -64,6 +64,10 @@ struct plic_priv {
 	struct cpumask lmask;
 	struct irq_domain *irqdomain;
 	void __iomem *regs;
+	enum {
+		VANILLA_PLIC,
+		RENESAS_R9A07G043_PLIC,
+	} flavour;
 };
 
 struct plic_handler {

to give some structure to the whole thing, because I'm pretty sure
we'll see more braindead implementations as time goes by.

It almost feels like I've written this whole patch. Oh wait...

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv