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

[Lad, Prabhakar]

Hi Marc,

On Sat, Jun 25, 2022 at 5:05 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> On Sat, 25 Jun 2022 14:03:33 +0100,
> "Lad, Prabhakar" <prabhakar.csengg@xxxxxxxxx> wrote:
> >
> > [1  <text/plain; UTF-8 (7bit)>]
> > Hi Marc,
> >
> > On Sat, Jun 25, 2022 at 12:52 PM Marc Zyngier <maz@xxxxxxxxxx> wrote:
> > >
>
> [...]
>
> > > You are just reinventing the wheel we are already have, except that
> > > yours is a bit square ;-). What really should happen is that the
> > > set_type method should set the correct flow depending on the trigger
> > > of the interrupt, and *never* have to check the configuration on the
> > > handling path.
> > >
> > A Bit lost here..
> >
> > We have the below chained handler:
> >
> > static void plic_handle_irq(struct irq_desc *desc)
> > {
> >     struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> >     struct irq_chip *chip = irq_desc_get_chip(desc);
> >     void __iomem *claim = handler->hart_base + CONTEXT_CLAIM;
> >     irq_hw_number_t hwirq;
> >
> >     WARN_ON_ONCE(!handler->present);
> >
> >     chained_irq_enter(chip, desc);
> >
> >     while ((hwirq = readl(claim))) {
> >         int err = generic_handle_domain_irq(handler->priv->irqdomain,
> >                             hwirq);
> >         if (unlikely(err))
> >             pr_warn_ratelimited("can't find mapping for hwirq %lu\n",
> >                     hwirq);
> >     }
> >
> >     chained_irq_exit(chip, desc);
> > }
> >
> > static void plic_irq_eoi(struct irq_data *d)
> > {
> >     struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> >
> >     if (irqd_irq_masked(d)) {
> >         plic_irq_unmask(d);
> >         writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> >         plic_irq_mask(d);
> >     } else {
> >         writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> >     }
> > }
> >
> > Where it's claiming -> handling interrupt -> interrupt completion in
> > eoi which is according to architecture.
> >
> >
> > Now with fasteoi_ack flow If I introduce the below ack callback to
> > issue interrupt completion.
> >
> > static void plic_irq_ack(struct irq_data *d)
> > {
> >     struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
> >
> >     if (irqd_irq_masked(d)) {
> >         plic_irq_unmask(d);
> >         writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> >         plic_irq_mask(d);
> >     } else {
> >         writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> >     }
> > }
> >
> > Here we are issuing an interrupt completion first, and later in the
> > handler  plic_handle_irq() we are claiming the interrupt by reading
> > the claim register. With this we are not following [0].
>
> Whatever [0] says doesn't really matter, since the HW is totally
> busted.
>
OK

> > Do you think this flow is OK (interrupt completion -> Interrupt claim
> > -> handle IRQ)?
>
> You keep missing my point. Edge and Level *must* have different flows
> and this also implies different callbacks. You can't just handle both
> at once. You should have something like this (untested):
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index bb87e4c3b88e..5e072be32d9f 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -176,16 +176,52 @@ static void plic_irq_eoi(struct irq_data *d)
>         }
>  }
>
> +static int broken_set_type(struct irq_data *d, unsigned int type);
> +
>  static struct irq_chip plic_chip = {
>         .name           = "SiFive PLIC",
>         .irq_mask       = plic_irq_mask,
>         .irq_unmask     = plic_irq_unmask,
>         .irq_eoi        = plic_irq_eoi,
> +       .irq_set_type   = broken_set_type,
> +#ifdef CONFIG_SMP
> +       .irq_set_affinity = plic_set_affinity,
> +#endif
> +};
> +
> +static void broken_eoi(struct irq_data *data) {}
> +
> +static struct irq_chip plic_chip_edge = {
> +       .name           = "Edgy PLIC",
> +       .irq_mask       = plic_irq_mask,
> +       .irq_unmask     = plic_irq_unmask,
> +       .irq_ack        = plic_irq_eoi,
> +       .irq_eoi        = broken_eoi,
> +       .irq_set_type   = broken_set_type,
>  #ifdef CONFIG_SMP
>         .irq_set_affinity = plic_set_affinity,
>  #endif
>  };
>
> +static int broken_set_type(struct irq_data *d, unsigned int type)
> +{
> +       if (!plic_is_totaly_broken())
> +               return 0;
> +
> +       if (type == IRQ_TYPE_EDGE_RISING)
> +               irq_set_chip_handler_name_locked(d, plic_chip_edge,
> +                                                handle_fasteoi_ack_irq,
> +                                                "Edge");
> +       else if (type == IRQ_TYPE_LEVEL_HIGH)
> +               irq_set_chip_handler_name_locked(d, plic_chip,
> +                                                handle_fasteoi_irq,
> +                                                "Level");
> +       else
> +               return -EINVAL;
> +
> +       return 0;
> +}
> +
>  static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
>                               irq_hw_number_t hwirq)
>  {
>
> which applies the correct flow and chip depending on the trigger
> information. This also implies that for chained PLICs, the secondary
> PLIC output is handled as a level into the primary PLIC.
>
Agreed, the above chunk does work. I'll re-spin a v2 with the above included.

Cheers,
Prabhakar