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

[Marc Zyngier]

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.

> 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.

	M.

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