[PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[Icenowy Zheng]

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.

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.

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

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 drivers/irqchip/irq-sifive-plic.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 2f4784860df5..219e4f1b62f0 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -67,6 +67,7 @@ struct plic_priv {
 	struct irq_domain *irqdomain;
 	void __iomem *regs;
 	unsigned long plic_quirks;
+	u32 interrupt_cells;
 };
 
 struct plic_handler {
@@ -208,7 +209,7 @@ 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))
+	if (priv->interrupt_cells < 2)
 		return IRQ_SET_MASK_OK_NOCOPY;
 
 	switch (type) {
@@ -246,7 +247,7 @@ static int plic_irq_domain_translate(struct irq_domain *d,
 {
 	struct plic_priv *priv = d->host_data;
 
-	if (test_bit(PLIC_QUIRK_EDGE_INTERRUPT, &priv->plic_quirks))
+	if (priv->interrupt_cells >= 2)
 		return irq_domain_translate_twocell(d, fwspec, hwirq, type);
 
 	return irq_domain_translate_onecell(d, fwspec, hwirq, type);
@@ -357,6 +358,10 @@ static int __init __plic_init(struct device_node *node,
 	}
 
 	error = -EINVAL;
+	of_property_read_u32(node, "#interrupt-cells", &priv->interrupt_cells);
+	if (WARN_ON(!priv->interrupt_cells))
+		goto out_iounmap;
+
 	of_property_read_u32(node, "riscv,ndev", &nr_irqs);
 	if (WARN_ON(!nr_irqs))
 		goto out_iounmap;
@@ -479,12 +484,5 @@ static int __init plic_init(struct device_node *node,
 
 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);
-- 
2.37.1

[PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[Icenowy Zheng]

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.

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.

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

Signed-off-by: Icenowy Zheng <uwu@icenowy.me>
---
 drivers/irqchip/irq-sifive-plic.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 2f4784860df5..219e4f1b62f0 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -67,6 +67,7 @@ struct plic_priv {
 	struct irq_domain *irqdomain;
 	void __iomem *regs;
 	unsigned long plic_quirks;
+	u32 interrupt_cells;
 };
 
 struct plic_handler {
@@ -208,7 +209,7 @@ 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))
+	if (priv->interrupt_cells < 2)
 		return IRQ_SET_MASK_OK_NOCOPY;
 
 	switch (type) {
@@ -246,7 +247,7 @@ static int plic_irq_domain_translate(struct irq_domain *d,
 {
 	struct plic_priv *priv = d->host_data;
 
-	if (test_bit(PLIC_QUIRK_EDGE_INTERRUPT, &priv->plic_quirks))
+	if (priv->interrupt_cells >= 2)
 		return irq_domain_translate_twocell(d, fwspec, hwirq, type);
 
 	return irq_domain_translate_onecell(d, fwspec, hwirq, type);
@@ -357,6 +358,10 @@ static int __init __plic_init(struct device_node *node,
 	}
 
 	error = -EINVAL;
+	of_property_read_u32(node, "#interrupt-cells", &priv->interrupt_cells);
+	if (WARN_ON(!priv->interrupt_cells))
+		goto out_iounmap;
+
 	of_property_read_u32(node, "riscv,ndev", &nr_irqs);
 	if (WARN_ON(!nr_irqs))
 		goto out_iounmap;
@@ -479,12 +484,5 @@ static int __init plic_init(struct device_node *node,
 
 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);
-- 
2.37.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

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

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

Thanks,

	M.

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

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

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

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

Thanks,

	M.

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[Icenowy Zheng]

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

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

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

> 
> Thanks,
> 
>         M.
> 

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[Icenowy Zheng]

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

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

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

> 
> Thanks,
> 
>         M.
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

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

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

Thanks,

	M.

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

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

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

Thanks,

	M.

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

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[Icenowy Zheng]

在 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?

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

Sounds more crazy...

> 
> Thanks,
> 
>         M.
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[Icenowy Zheng]

在 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?

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

Sounds more crazy...

> 
> Thanks,
> 
>         M.
> 

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

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

> Sounds more crazy...

There is obviously no accounting for taste.

      M.

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

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

> Sounds more crazy...

There is obviously no accounting for taste.

      M.

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

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[Icenowy Zheng]

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

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?

> 
> > Sounds more crazy...
> 
> There is obviously no accounting for taste.
> 
>       M.
> 


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[Icenowy Zheng]

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

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?

> 
> > Sounds more crazy...
> 
> There is obviously no accounting for taste.
> 
>       M.
> 

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[Marc Zyngier]

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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH] irqchip/sifive-plic: drop quirk for two-cell variant

[Marc Zyngier]

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.