[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+V-a8tcxj_N0sBHhgAZAN8WSJ12JnDzAvUUnCXto3wHLqNVwg@mail.gmail.com>
Date: Sun, 26 Jun 2022 10:38:18 +0100
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Marc Zyngier <maz@...nel.org>
Cc: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>,
Thomas Gleixner <tglx@...utronix.de>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Sagar Kadam <sagar.kadam@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Paul Walmsley <paul.walmsley@...ive.com>,
linux-riscv <linux-riscv@...ts.infradead.org>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Biju Das <biju.das.jz@...renesas.com>
Subject: Re: [PATCH v2 2/2] irqchip/sifive-plic: Add support for Renesas
RZ/Five SoC
Hi Marc,
Thank you for the review.
On Sun, Jun 26, 2022 at 9:56 AM Marc Zyngier <maz@...nel.org> wrote:
>
> On Sun, 26 Jun 2022 01:43:26 +0100,
> Lad Prabhakar <prabhakar.mahadev-lad.rj@...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@...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(-)
>
> [...]
>
> >
> > +static int plic_irq_set_type(struct irq_data *d, unsigned int type)
> > +{
> > + struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> > +
> > + if (handler->priv->of_data != RENESAS_R9A07G043_PLIC)
> > + return 0;
> > +
> > + switch (type) {
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + irq_set_chip_handler_name_locked(d, &renesas_rzfive_edge_plic_chip,
> > + handle_fasteoi_ack_irq,
> > + "Edge");
> > + break;
> > +
> > + case IRQ_TYPE_EDGE_RISING:
> > + irq_set_chip_handler_name_locked(d, &plic_chip,
> > + handle_fasteoi_irq,
> > + "Level");
> > + break;
>
> Really? Have you even tested this?
>
Ouch my bad, while rebasing I did swap this up!
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > irq_hw_number_t hwirq)
> > {
> > @@ -198,6 +248,19 @@ static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > return 0;
> > }
> >
> > +static int plic_irq_domain_translate(struct irq_domain *d,
> > + struct irq_fwspec *fwspec,
> > + unsigned long *hwirq,
> > + unsigned int *type)
> > +{
> > + struct plic_priv *priv = d->host_data;
> > +
> > + if (priv->of_data == RENESAS_R9A07G043_PLIC)
> > + return irq_domain_translate_twocell(d, fwspec, hwirq, type);
> > +
> > + return irq_domain_translate_onecell(d, fwspec, hwirq, type);
> > +}
> > +
> > static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > unsigned int nr_irqs, void *arg)
> > {
> > @@ -206,7 +269,7 @@ static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > unsigned int type;
> > struct irq_fwspec *fwspec = arg;
> >
> > - ret = irq_domain_translate_onecell(domain, fwspec, &hwirq, &type);
> > + ret = plic_irq_domain_translate(domain, fwspec, &hwirq, &type);
> > if (ret)
> > return ret;
> >
> > @@ -220,7 +283,7 @@ static int plic_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> > }
> >
> > static const struct irq_domain_ops plic_irqdomain_ops = {
> > - .translate = irq_domain_translate_onecell,
> > + .translate = plic_irq_domain_translate,
> > .alloc = plic_irq_domain_alloc,
> > .free = irq_domain_free_irqs_top,
> > };
> > @@ -293,6 +356,11 @@ static int __init plic_init(struct device_node *node,
> > if (!priv)
> > return -ENOMEM;
> >
> > + 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.
Cheers,
Prabhakar
Powered by blists - more mailing lists