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
