Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 25 Jun 2022 17:05:59 +0100
Message-ID: <87zgi0eniw.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
Cc:     Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Sagar Kadam <sagar.kadam@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        Geert Uytterhoeven <geert+renesas@glider.be>,
        Linux-Renesas <linux-renesas-soc@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Biju Das <biju.das.jz@bp.renesas.com>
Subject: Re: [PATCH 2/2] irqchip/sifive-plic: Add support for Renesas RZ/Five SoC
In-Reply-To: <CA+V-a8tFn7aS-1jD6o9TX9kNDQ=4S3xWCXwkww3Es2+Red6vmA@mail.gmail.com>
References: <20220624180311.3007-1-prabhakar.mahadev-lad.rj@bp.renesas.com>
        <20220624180311.3007-3-prabhakar.mahadev-lad.rj@bp.renesas.com>
        <8735ftf73p.wl-maz@kernel.org>
        <CA+V-a8ukQsotuWLm6_qR79qO9n_Ffo2e79AitC_=53ocsjZtzA@mail.gmail.com>
        <87h7492c58.wl-maz@kernel.org>
        <CA+V-a8tFn7aS-1jD6o9TX9kNDQ=4S3xWCXwkww3Es2+Red6vmA@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1
 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Sat, 25 Jun 2022 14:03:33 +0100,
"Lad, Prabhakar" <prabhakar.csengg@gmail.com> wrote:
> 
> [1  <text/plain; UTF-8 (7bit)>]
> Hi Marc,
> 
> On Sat, Jun 25, 2022 at 12:52 PM Marc Zyngier <maz@kernel.org> 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.