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

[Lad, Prabhakar]

Hi Marc,

Thank you for the review.

On Sun, Jun 26, 2022 at 9:56 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Sun, 26 Jun 2022 01:43:26 +0100,
> Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> > ---
> > 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