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.
