Messages in this thread Patch in this message | | | Date | Wed, 23 Nov 2022 14:07:04 +0000 | From | Marc Zyngier <> | Subject | Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant |
| |
On Wed, 23 Nov 2022 13:35:58 +0000, Icenowy Zheng <uwu@icenowy.me> wrote: > > 在 2022-11-23星期三的 13:31 +0000,Marc Zyngier写道: > > On Wed, 23 Nov 2022 13:16:01 +0000, > > Icenowy Zheng <uwu@icenowy.me> wrote: > > > > > > 在 2022-11-23星期三的 13:13 +0000,Marc Zyngier写道: > > > > On Wed, 23 Nov 2022 12:38:56 +0000, > > > > Icenowy Zheng <uwu@icenowy.me> wrote: > > > > > > > > > > 在 2022-11-22星期二的 17:28 +0000,Marc Zyngier写道: > > > > > > On Mon, 21 Nov 2022 04:20:26 +0000, > > > > > > Icenowy Zheng <uwu@icenowy.me> wrote: > > > > > > > > > > > > > > As the special handling of edge-triggered interrupts are > > > > > > > defined in > > > > > > > the > > > > > > > PLIC spec, we can assume it's not a quirk, but a feature of > > > > > > > the > > > > > > > PLIC > > > > > > > spec; thus making it a quirk and use quirk-based codepath > > > > > > > is > > > > > > > not so > > > > > > > necessary. > > > > > > > > > > > > It *is* necessary. > > > > > > > > > > > > > > > > > > > > Move to a #interrupt-cells-based practice which will allow > > > > > > > both > > > > > > > device > > > > > > > trees without interrupt flags and with interrupt flags work > > > > > > > for > > > > > > > all > > > > > > > compatible strings. > > > > > > > > > > > > No. You're tying together two unrelated concepts: > > > > > > > > > > > > - Edges get dropped in some implementations (and only some). > > > > > > You > > > > > > can > > > > > > argue that the architecture allows it, but I see it is an > > > > > > implementation bug. > > > > > > > > > > As the specification allows it, it's not an implementation bug > > > > > -- > > > > > and > > > > > for those which do not show this problem, it's possible that > > > > > it's > > > > > just > > > > > all using the same trigger type (e.g. Rocket). > > > > > > > > What are you against? The fact that this is flagged as a quirk? > > > > Honestly, I don't care about that. If we can fold all > > > > implementations > > > > into the same scheme, that's fine by me. > > > > > > Then what should I do? > > > > Make all edge-triggered interrupts use the edge flow. > > > > > > > > > > > > > > > > > > > > > > > > > > - The need for expressing additional information in the > > > > > > interrupt > > > > > > specifier is not necessarily related to the above. Other > > > > > > interrupt > > > > > > controllers use extra cells to encode the interrupt > > > > > > affinity, > > > > > > for > > > > > > example. > > > > > > > > > > I think in these situations, if the interrupt controller does > > > > > not > > > > > contain any special handling for edge interrupts, we can just > > > > > describe > > > > > them as level ones in SW. > > > > > > > > No, that's utterly wrong. We don't describe an edge as level. > > > > Ever. > > > > > > > > > > > > > > > > > > > > > I want these two things to be kept separate. Otherwise, once > > > > > > we > > > > > > get > > > > > > some fancy ACPI support for RISCV (no, please...), we'll have > > > > > > to > > > > > > redo > > > > > > the whole thing... > > > > > > > > > > > > > In addition, this addresses a stable version DT binding > > > > > > > violation - > > > > > > > - > > > > > > > Linux v5.19 comes with "thead,c900-plic" with #interrupt- > > > > > > > cells > > > > > > > defined to > > > > > > > be 1 instead of 2, this commit will allow DTs that complies > > > > > > > to > > > > > > > Linux > > > > > > > v5.19 binding work (although no such DT is devliered to the > > > > > > > public > > > > > > > now). > > > > > > > > > > > > *That* is what should get fixed. > > > > > > > > > > Supporting all stable versions' DT binding is our promise, I > > > > > think. > > > > > > > > Absolutely. And I'm asking you to fix it. And only that. > > > > > > Then what should I do? Mask this as another quirk that is only > > > applicable to c900-plic? > > > > No. Make interrupts with a single cell use the level flow. > > This sounds exactly like what we do in this patch now.
No. Really not. If anything, you add more pointless crap.
> Or, should we keep the quirk, and require both a flag cell containing > IRQ_TYPE_EDGE_RISING and an interrupt controller that matches the quirk > to use the special codepath for edge interrupts?
This is becoming tedious.
M.
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c index 2f4784860df5..6774ae19ad0b 100644 --- a/drivers/irqchip/irq-sifive-plic.c +++ b/drivers/irqchip/irq-sifive-plic.c @@ -60,13 +60,10 @@ #define PLIC_DISABLE_THRESHOLD 0x7 #define PLIC_ENABLE_THRESHOLD 0 -#define PLIC_QUIRK_EDGE_INTERRUPT 0 - struct plic_priv { struct cpumask lmask; struct irq_domain *irqdomain; void __iomem *regs; - unsigned long plic_quirks; }; struct plic_handler { @@ -208,9 +205,6 @@ static int plic_irq_set_type(struct irq_data *d, unsigned int type) { struct plic_priv *priv = irq_data_get_irq_chip_data(d); - if (!test_bit(PLIC_QUIRK_EDGE_INTERRUPT, &priv->plic_quirks)) - return IRQ_SET_MASK_OK_NOCOPY; - switch (type) { case IRQ_TYPE_EDGE_RISING: irq_set_chip_handler_name_locked(d, &plic_edge_chip, @@ -244,9 +238,7 @@ static int plic_irq_domain_translate(struct irq_domain *d, unsigned long *hwirq, unsigned int *type) { - struct plic_priv *priv = d->host_data; - - if (test_bit(PLIC_QUIRK_EDGE_INTERRUPT, &priv->plic_quirks)) + if (irq_fwspec->param_count >= 2) return irq_domain_translate_twocell(d, fwspec, hwirq, type); return irq_domain_translate_onecell(d, fwspec, hwirq, type); @@ -335,9 +327,8 @@ static int plic_starting_cpu(unsigned int cpu) return 0; } -static int __init __plic_init(struct device_node *node, - struct device_node *parent, - unsigned long plic_quirks) +static int __init plic_init(struct device_node *node, + struct device_node *parent) { int error = 0, nr_contexts, nr_handlers = 0, i; u32 nr_irqs; @@ -348,8 +339,6 @@ static int __init __plic_init(struct device_node *node, if (!priv) return -ENOMEM; - priv->plic_quirks = plic_quirks; - priv->regs = of_iomap(node, 0); if (WARN_ON(!priv->regs)) { error = -EIO; @@ -471,20 +460,7 @@ static int __init __plic_init(struct device_node *node, return error; } -static int __init plic_init(struct device_node *node, - struct device_node *parent) -{ - return __plic_init(node, parent, 0); -} - IRQCHIP_DECLARE(sifive_plic, "sifive,plic-1.0.0", plic_init); IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */ - -static int __init plic_edge_init(struct device_node *node, - struct device_node *parent) -{ - return __plic_init(node, parent, BIT(PLIC_QUIRK_EDGE_INTERRUPT)); -} - -IRQCHIP_DECLARE(andestech_nceplic100, "andestech,nceplic100", plic_edge_init); -IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_edge_init); +IRQCHIP_DECLARE(andestech_nceplic100, "andestech,nceplic100", plic_init); +IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_init); -- Without deviation from the norm, progress is not possible.
| |