irqchip: riscv: Add thead,c900-plic support

From: Guo Ren <[email protected]>

1) The irq_mask/unmask() is used by handle_fasteoi_irq() is mostly for ONESHOT irqs and there is no limitation in the RISC-V PLIC driver due to use of irq_mask/unmask() callbacks. In fact, a lot of irqchip drivers using handle_fasteoi_irq() also implement irq_mask/unmask().

2) The C9xx PLIC does not comply with the interrupt claim/completion process defined by the RISC-V PLIC specification because C9xx PLIC will mask an IRQ when it is claimed by PLIC driver (i.e. readl(claim) and the IRQ will be unmasked upon completion by PLIC driver (i.e. writel(claim). This behaviour breaks the handling of IRQS_ONESHOT by the generic handle_fasteoi_irq() used in the PLIC driver.

3) This patch adds an errata fix for IRQS_ONESHOT handling on C9xx PLIC by using irq_enable/disable() callbacks instead of irq_mask/unmask().

Signed-off-by Guo Ren <[email protected]>

Cc Anup Patel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Atish Patra <[email protected]>

---

Changes since V4:
- Update comment by Anup

Changes since V3:
- Rename "c9xx" to "c900"
- Add sifive_plic_chip and thead_plic_chip for difference

Changes since V2:
- Add a separate compatible string "thead,c9xx-plic"
- set irq_mask/unmask of "plic_chip" to NULL and point irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
- Add a detailed comment block in plic_init() about the differences in Claim/Completion process of RISC-V PLIC and C9xx PLIC.

---
 drivers/irqchip/irq-sifive-plic.c | 34 +++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

drivers/irqchip/irq-sifive-plic.c +7 -1

index cf74cfa82045..960b29d02070 100644
@@ -166,7 +166,7 @@ static void plic_irq_eoi(struct irq_data *d)
 	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
 }
 
-static struct irq_chip plic_chip = {
+static struct irq_chip sifive_plic_chip = {
 	.name		= "SiFive PLIC",
 	.irq_mask	= plic_irq_mask,
 	.irq_unmask	= plic_irq_unmask,

@@ -176,12 +176,32 @@ static struct irq_chip plic_chip = {
 #endif
 };
 
+/*
+ * The C9xx PLIC does not comply with the interrupt claim/completion
+ * process defined by the RISC-V PLIC specification because C9xx PLIC
+ * will mask an IRQ when it is claimed by PLIC driver (i.e. readl(claim)
+ * and the IRQ will be unmasked upon completion by PLIC driver (i.e.
+ * writel(claim). This behaviour breaks the handling of IRQS_ONESHOT by

drivers/irqchip/irq-sifive-plic.c +15 -1

+};
+
+static struct irq_chip *def_plic_chip = &sifive_plic_chip;
+
 static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
 			      irq_hw_number_t hwirq)
 {
 	struct plic_priv *priv = d->host_data;
 
-	irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
+	irq_domain_set_info(d, irq, hwirq, def_plic_chip, d->host_data,
 			    handle_fasteoi_irq, NULL, NULL);
 	irq_set_noprobe(irq);
 	irq_set_affinity(irq, &priv->lmask);

@@ -390,5 +410,15 @@ static int __init plic_init(struct device_node *node,
 	return error;
 }
 
+static int __init thead_c900_plic_init(struct device_node *node,
+		struct device_node *parent)
+{
+	def_plic_chip = &thead_plic_chip;
+
+	return plic_init(node, parent);
+}
+
 IRQCHIP_DECLARE(sifive_plic, "sifive,plic-1.0.0", plic_init);
 IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */
+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", thead_c900_plic_init);
+IRQCHIP_DECLARE(allwinner_sun20i_d1_plic, "allwinner,sun20i-d1-plic", thead_c900_plic_init);

Quoted 68 lines · On 10/15/21 10:21 PM, [email protected] wrote:

From: Guo Ren <[email protected]>

1) The irq_mask/unmask() is used by handle_fasteoi_irq() is mostly for ONESHOT irqs and there is no limitation in the RISC-V PLIC driver due to use of irq_mask/unmask() callbacks. In fact, a lot of irqchip drivers using handle_fasteoi_irq() also implement irq_mask/unmask().

2) The C9xx PLIC does not comply with the interrupt claim/completion process defined by the RISC-V PLIC specification because C9xx PLIC will mask an IRQ when it is claimed by PLIC driver (i.e. readl(claim) and the IRQ will be unmasked upon completion by PLIC driver (i.e. writel(claim). This behaviour breaks the handling of IRQS_ONESHOT by the generic handle_fasteoi_irq() used in the PLIC driver.

3) This patch adds an errata fix for IRQS_ONESHOT handling on C9xx PLIC by using irq_enable/disable() callbacks instead of irq_mask/unmask().

Signed-off-by Guo Ren <[email protected]>

Cc Anup Patel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Atish Patra <[email protected]>

---

Changes since V4:
- Update comment by Anup

Changes since V3:
- Rename "c9xx" to "c900"
- Add sifive_plic_chip and thead_plic_chip for difference

Changes since V2:
- Add a separate compatible string "thead,c9xx-plic"
- set irq_mask/unmask of "plic_chip" to NULL and point irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
- Add a detailed comment block in plic_init() about the differences in Claim/Completion process of RISC-V PLIC and C9xx PLIC.

---
 drivers/irqchip/irq-sifive-plic.c | 34 +++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

drivers/irqchip/irq-sifive-plic.c +7 -1

index cf74cfa82045..960b29d02070 100644
@@ -166,7 +166,7 @@ static void plic_irq_eoi(struct irq_data *d)
      writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
 }
 
-static struct irq_chip plic_chip = {
+static struct irq_chip sifive_plic_chip = {
      .name           = "SiFive PLIC",
      .irq_mask       = plic_irq_mask,
      .irq_unmask     = plic_irq_unmask,

@@ -176,12 +176,32 @@ static struct irq_chip plic_chip = {
 #endif
 };
 
+/*
+ * The C9xx PLIC does not comply with the interrupt claim/completion
+ * process defined by the RISC-V PLIC specification because C9xx PLIC
+ * will mask an IRQ when it is claimed by PLIC driver (i.e. readl(claim)
+ * and the IRQ will be unmasked upon completion by PLIC driver (i.e.
+ * writel(claim). This behaviour breaks the handling of IRQS_ONESHOT by

drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 194

Quoted 10 lines

drivers/irqchip/irq-sifive-plic.c +10 -0

+ * the generic handle_fasteoi_irq() used in the PLIC driver.
+ */
+static struct irq_chip thead_plic_chip = {
+     .name           = "T-Head PLIC",
+     .irq_disable    = plic_irq_mask,
+     .irq_enable     = plic_irq_unmask,
+     .irq_eoi        = plic_irq_eoi,
+#ifdef CONFIG_SMP
+     .irq_set_affinity = plic_set_affinity,
+#endif

I tested this, and it doesn't work. Without IRQCHIP_EOI_THREADED, .irq_eoi is called at the end of the hard IRQ handler. This unmasks the IRQ before the irqthread has a chance to run, so it causes an interrupt storm for any threaded level IRQ (I saw this happen for sun8i_thermal).

With IRQCHIP_EOI_THREADED, .irq_eoi is delayed until after the irqthread runs. This is good. Except that the call to unmask_threaded_irq() is inside a check for IRQD_IRQ_MASKED. And IRQD_IRQ_MASKED will never be set because .irq_mask is NULL. So the end result is that the IRQ is never EOI'd and is masked permanently.

If you set .flags = IRQCHIP_EOI_THREADED, and additionally set .irq_mask and .irq_unmask to a dummy function that does nothing, the IRQ core will properly set/unset IRQD_IRQ_MASKED, and the IRQs will flow as expected. But adding dummy functions seems not so ideal, so I am not sure if this is the best solution.

Regards,
Samuel

Quoted 31 lines

drivers/irqchip/irq-sifive-plic.c +15 -1

+};
+
+static struct irq_chip *def_plic_chip = &sifive_plic_chip;
+
 static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
                            irq_hw_number_t hwirq)
 {
      struct plic_priv *priv = d->host_data;
 
-     irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
+     irq_domain_set_info(d, irq, hwirq, def_plic_chip, d->host_data,
                          handle_fasteoi_irq, NULL, NULL);
      irq_set_noprobe(irq);
      irq_set_affinity(irq, &priv->lmask);

@@ -390,5 +410,15 @@ static int __init plic_init(struct device_node *node,
      return error;
 }
 
+static int __init thead_c900_plic_init(struct device_node *node,
+             struct device_node *parent)
+{
+     def_plic_chip = &thead_plic_chip;
+
+     return plic_init(node, parent);
+}
+
 IRQCHIP_DECLARE(sifive_plic, "sifive,plic-1.0.0", plic_init);
 IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */
+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", thead_c900_plic_init);
+IRQCHIP_DECLARE(allwinner_sun20i_d1_plic, "allwinner,sun20i-d1-plic", thead_c900_plic_init);

Quoted 74 lines · On 10/15/21 10:21 PM, [email protected] wrote:

From: Guo Ren <[email protected]>

1) The irq_mask/unmask() is used by handle_fasteoi_irq() is mostly for ONESHOT irqs and there is no limitation in the RISC-V PLIC driver due to use of irq_mask/unmask() callbacks. In fact, a lot of irqchip drivers using handle_fasteoi_irq() also implement irq_mask/unmask().

2) The C9xx PLIC does not comply with the interrupt claim/completion process defined by the RISC-V PLIC specification because C9xx PLIC will mask an IRQ when it is claimed by PLIC driver (i.e. readl(claim) and the IRQ will be unmasked upon completion by PLIC driver (i.e. writel(claim). This behaviour breaks the handling of IRQS_ONESHOT by the generic handle_fasteoi_irq() used in the PLIC driver.

3) This patch adds an errata fix for IRQS_ONESHOT handling on C9xx PLIC by using irq_enable/disable() callbacks instead of irq_mask/unmask().

Signed-off-by Guo Ren <[email protected]>

Cc Anup Patel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Atish Patra <[email protected]>

---

Changes since V4:
- Update comment by Anup

Changes since V3:
- Rename "c9xx" to "c900"
- Add sifive_plic_chip and thead_plic_chip for difference

Changes since V2:
- Add a separate compatible string "thead,c9xx-plic"
- set irq_mask/unmask of "plic_chip" to NULL and point irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
- Add a detailed comment block in plic_init() about the differences in Claim/Completion process of RISC-V PLIC and C9xx PLIC.

---
 drivers/irqchip/irq-sifive-plic.c | 34 +++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

drivers/irqchip/irq-sifive-plic.c +13 -1

index cf74cfa82045..960b29d02070 100644
@@ -166,7 +166,7 @@ static void plic_irq_eoi(struct irq_data *d)
      writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
 }
 
-static struct irq_chip plic_chip = {
+static struct irq_chip sifive_plic_chip = {
      .name           = "SiFive PLIC",
      .irq_mask       = plic_irq_mask,
      .irq_unmask     = plic_irq_unmask,

@@ -176,12 +176,32 @@ static struct irq_chip plic_chip = {
 #endif
 };
 
+/*
+ * The C9xx PLIC does not comply with the interrupt claim/completion
+ * process defined by the RISC-V PLIC specification because C9xx PLIC
+ * will mask an IRQ when it is claimed by PLIC driver (i.e. readl(claim)
+ * and the IRQ will be unmasked upon completion by PLIC driver (i.e.
+ * writel(claim). This behaviour breaks the handling of IRQS_ONESHOT by
+ * the generic handle_fasteoi_irq() used in the PLIC driver.
+ */
+static struct irq_chip thead_plic_chip = {
+     .name           = "T-Head PLIC",
+     .irq_disable    = plic_irq_mask,
+     .irq_enable     = plic_irq_unmask,

drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 194

Quoted 4 lines

drivers/irqchip/irq-sifive-plic.c +4 -0

+     .irq_eoi        = plic_irq_eoi,
+#ifdef CONFIG_SMP
+     .irq_set_affinity = plic_set_affinity,
+#endif

I tested this, and it doesn't work. Without IRQCHIP_EOI_THREADED, .irq_eoi is called at the end of the hard IRQ handler. This unmasks the IRQ before the irqthread has a chance to run, so it causes an interrupt storm for any threaded level IRQ (I saw this happen for sun8i_thermal).

With IRQCHIP_EOI_THREADED, .irq_eoi is delayed until after the irqthread runs. This is good. Except that the call to unmask_threaded_irq() is inside a check for IRQD_IRQ_MASKED. And IRQD_IRQ_MASKED will never be set because .irq_mask is NULL. So the end result is that the IRQ is never EOI'd and is masked permanently.

If you set .flags = IRQCHIP_EOI_THREADED, and additionally set .irq_mask and .irq_unmask to a dummy function that does nothing, the IRQ core will properly set/unset IRQD_IRQ_MASKED, and the IRQs will flow as expected. But adding dummy functions seems not so ideal, so I am not sure if this is the best solution.

Regards,
Samuel

Quoted 31 lines

drivers/irqchip/irq-sifive-plic.c +15 -1

+};
+
+static struct irq_chip *def_plic_chip = &sifive_plic_chip;
+
 static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
                            irq_hw_number_t hwirq)
 {
      struct plic_priv *priv = d->host_data;
 
-     irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
+     irq_domain_set_info(d, irq, hwirq, def_plic_chip, d->host_data,
                          handle_fasteoi_irq, NULL, NULL);
      irq_set_noprobe(irq);
      irq_set_affinity(irq, &priv->lmask);

@@ -390,5 +410,15 @@ static int __init plic_init(struct device_node *node,
      return error;
 }
 
+static int __init thead_c900_plic_init(struct device_node *node,
+             struct device_node *parent)
+{
+     def_plic_chip = &thead_plic_chip;
+
+     return plic_init(node, parent);
+}
+
 IRQCHIP_DECLARE(sifive_plic, "sifive,plic-1.0.0", plic_init);
 IRQCHIP_DECLARE(riscv_plic0, "riscv,plic0", plic_init); /* for legacy systems */
+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", thead_c900_plic_init);
+IRQCHIP_DECLARE(allwinner_sun20i_d1_plic, "allwinner,sun20i-d1-plic", thead_c900_plic_init);

Quoted 3 lines · On 10/15/21 10:21 PM, [email protected] wrote:

From: Guo Ren <[email protected]>

1) The irq_mask/unmask() is used by handle_fasteoi_irq() is mostly

Quoted 3 lines

for ONESHOT irqs and there is no limitation in the RISC-V PLIC driver due to use of irq_mask/unmask() callbacks. In fact, a lot of irqchip drivers using handle_fasteoi_irq() also implement irq_mask/unmask().

Quoted 8 lines

2) The C9xx PLIC does not comply with the interrupt claim/completion process defined by the RISC-V PLIC specification because C9xx PLIC will mask an IRQ when it is claimed by PLIC driver (i.e. readl(claim) and the IRQ will be unmasked upon completion by PLIC driver (i.e. writel(claim). This behaviour breaks the handling of IRQS_ONESHOT by the generic handle_fasteoi_irq() used in the PLIC driver.

3) This patch adds an errata fix for IRQS_ONESHOT handling on

Quoted 2 lines

C9xx PLIC by using irq_enable/disable() callbacks instead of irq_mask/unmask().

Quoted 56 lines

Signed-off-by Guo Ren <[email protected]>

Cc Anup Patel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Atish Patra <[email protected]>

---

Changes since V4:
- Update comment by Anup

Changes since V3:
- Rename "c9xx" to "c900"
- Add sifive_plic_chip and thead_plic_chip for difference

Changes since V2:
- Add a separate compatible string "thead,c9xx-plic"
- set irq_mask/unmask of "plic_chip" to NULL and point irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
- Add a detailed comment block in plic_init() about the differences in Claim/Completion process of RISC-V PLIC and C9xx PLIC.

---
 drivers/irqchip/irq-sifive-plic.c | 34 
+++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

drivers/irqchip/irq-sifive-plic.c +7 -1

index cf74cfa82045..960b29d02070 100644
@@ -166,7 +166,7 @@ static void plic_irq_eoi(struct irq_data *d)
 	writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
 }
 
-static struct irq_chip plic_chip = {
+static struct irq_chip sifive_plic_chip = {
 	.name		= "SiFive PLIC",
 	.irq_mask	= plic_irq_mask,
 	.irq_unmask	= plic_irq_unmask,

@@ -176,12 +176,32 @@ static struct irq_chip plic_chip = {
 #endif
 };
 
+/*
+ * The C9xx PLIC does not comply with the interrupt claim/completion
+ * process defined by the RISC-V PLIC specification because C9xx PLIC
+ * will mask an IRQ when it is claimed by PLIC driver (i.e. 
 readl(claim)
+ * and the IRQ will be unmasked upon completion by PLIC driver (i.e.
+ * writel(claim). This behaviour breaks the handling of IRQS_ONESHOT 

by
+ * the generic handle_fasteoi_irq() used in the PLIC driver.
+ */

drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 194

Quoted 8 lines

drivers/irqchip/irq-sifive-plic.c +8 -0

+static struct irq_chip thead_plic_chip = {
+	.name		= "T-Head PLIC",
+	.irq_disable	= plic_irq_mask,
+	.irq_enable	= plic_irq_unmask,
+	.irq_eoi	= plic_irq_eoi,
+#ifdef CONFIG_SMP
+	.irq_set_affinity = plic_set_affinity,
+#endif

I tested this, and it doesn't work. Without IRQCHIP_EOI_THREADED, .irq_eoi is called at the end of the hard IRQ handler. This unmasks the IRQ before the irqthread has a chance to run, so it causes an interrupt storm for any threaded level IRQ (I saw this happen for sun8i_thermal).

With IRQCHIP_EOI_THREADED, .irq_eoi is delayed until after the irqthread runs. This is good. Except that the call to unmask_threaded_irq() is inside a check for IRQD_IRQ_MASKED. And IRQD_IRQ_MASKED will never be set because .irq_mask is NULL. So the end result is that the IRQ is never EOI'd and is masked permanently.

If you set .flags = IRQCHIP_EOI_THREADED, and additionally set .irq_mask and .irq_unmask to a dummy function that does nothing, the IRQ core will properly set/unset IRQD_IRQ_MASKED, and the IRQs will flow as expected. But adding dummy functions seems not so ideal, so I am not sure if this is the best solution.

Quoted 4 lines · Samuel Holland

Quoted 3 lines · On 10/15/21 10:21 PM, [email protected] wrote:

From: Guo Ren <[email protected]>

1) The irq_mask/unmask() is used by handle_fasteoi_irq() is mostly

Drop this useless numbering.

Quoted 4 lines

Quoted 4 lines

for ONESHOT irqs and there is no limitation in the RISC-V PLIC driver due to use of irq_mask/unmask() callbacks. In fact, a lot of irqchip drivers using handle_fasteoi_irq() also implement irq_mask/unmask().

This paragraph doesn't provide any useful information in the context of this patch. That's at best cover-letter material.

Quoted 9 lines

Quoted 9 lines

2) The C9xx PLIC does not comply with the interrupt claim/completion process defined by the RISC-V PLIC specification because C9xx PLIC will mask an IRQ when it is claimed by PLIC driver (i.e. readl(claim) and the IRQ will be unmasked upon completion by PLIC driver (i.e. writel(claim). This behaviour breaks the handling of IRQS_ONESHOT by the generic handle_fasteoi_irq() used in the PLIC driver.

3) This patch adds an errata fix for IRQS_ONESHOT handling on

s/fix/workaround/

Quoted 3 lines

Quoted 3 lines

C9xx PLIC by using irq_enable/disable() callbacks instead of irq_mask/unmask().

From Documentation/process/submitting-patches.rst:

<quote>
Describe your changes in imperative mood, e.g. "make xyzzy do frotz" instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy to do frotz", as if you are giving orders to the codebase to change its behaviour.
</quote>

Quoted 78 lines

Quoted 58 lines

Signed-off-by Guo Ren <[email protected]>

Cc Anup Patel <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Atish Patra <[email protected]>

---

Changes since V4:
- Update comment by Anup

Changes since V3:
- Rename "c9xx" to "c900"
- Add sifive_plic_chip and thead_plic_chip for difference

Changes since V2:
- Add a separate compatible string "thead,c9xx-plic"
- set irq_mask/unmask of "plic_chip" to NULL and point irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
- Add a detailed comment block in plic_init() about the differences in Claim/Completion process of RISC-V PLIC and C9xx PLIC.

---
 drivers/irqchip/irq-sifive-plic.c | 34
+++++++++++++++++++++++++++++--
 1 file changed, 32 insertions(+), 2 deletions(-)

drivers/irqchip/irq-sifive-plic.c +7 -1

index cf74cfa82045..960b29d02070 100644
@@ -166,7 +166,7 @@ static void plic_irq_eoi(struct irq_data *d)
     writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
 }
 
-static struct irq_chip plic_chip = {
+static struct irq_chip sifive_plic_chip = {
     .name           = "SiFive PLIC",
     .irq_mask       = plic_irq_mask,
     .irq_unmask     = plic_irq_unmask,

@@ -176,12 +176,32 @@ static struct irq_chip plic_chip = {
 #endif
 };
 
+/*
+ * The C9xx PLIC does not comply with the interrupt claim/completion
+ * process defined by the RISC-V PLIC specification because C9xx PLIC
+ * will mask an IRQ when it is claimed by PLIC driver (i.e.
 readl(claim)
+ * and the IRQ will be unmasked upon completion by PLIC driver (i.e.
+ * writel(claim). This behaviour breaks the handling of IRQS_ONESHOT

by
+ * the generic handle_fasteoi_irq() used in the PLIC driver.
+ */

drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 194

Quoted 8 lines

drivers/irqchip/irq-sifive-plic.c +8 -0

+static struct irq_chip thead_plic_chip = {
+    .name           = "T-Head PLIC",
+    .irq_disable    = plic_irq_mask,
+    .irq_enable     = plic_irq_unmask,
+    .irq_eoi        = plic_irq_eoi,
+#ifdef CONFIG_SMP
+    .irq_set_affinity = plic_set_affinity,
+#endif

I tested this, and it doesn't work. Without IRQCHIP_EOI_THREADED, .irq_eoi is called at the end of the hard IRQ handler. This unmasks the IRQ before the irqthread has a chance to run, so it causes an interrupt storm for any threaded level IRQ (I saw this happen for sun8i_thermal).

With IRQCHIP_EOI_THREADED, .irq_eoi is delayed until after the irqthread
runs. This is good. Except that the call to unmask_threaded_irq() is inside a check for IRQD_IRQ_MASKED. And IRQD_IRQ_MASKED will never be set because .irq_mask is NULL. So the end result is that the IRQ is never EOI'd and is masked permanently.

Quoted 7 lines

If you set .flags = IRQCHIP_EOI_THREADED, and additionally set .irq_mask
and .irq_unmask to a dummy function that does nothing, the IRQ core will
properly set/unset IRQD_IRQ_MASKED, and the IRQs will flow as expected. But adding dummy functions seems not so ideal, so I am not sure if this is the best solution.

This series is totally broken indeed, because it assumes that enable/disable are a substitute to mask/unmask. Nothing could be further from the truth. mask/unmask must be implemented, and enable/disable supplement them if the HW requires something different at startup time.

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

There is an example of this in the Apple AIC driver.

         M.

--
Jazz is not dead. It just smells funny...

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Quoted 6 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

Quoted 4 lines

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 6 lines

Quoted 3 lines

There is an example of this in the Apple AIC driver.

Thx for the tip, I think your suggestion is:
+++ b/drivers/irqchip/irq-sifive-plic.c

Review static void plic_irq_eoi(struct irq_data *d) · line 169

Quoted 8 lines

(quoted diff) +4 -1

@@ -163,7 +163,12 @@ static void plic_irq_eoi(struct irq_data *d)
 {
        struct plic_handler *handler = this_cpu_ptr(&plic_handlers);
 
-       writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+       if (irqd_irq_masked(d)) {
+               plic_irq_unmask(d);
+               writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
+               plic_irq_mask(d);

This looks pretty dodgy. You are relying on interrupts being globally masked on the CPU, I guess. It probably works today, but man, what a terrible HW implementation.

You'll definitely have to move this into a c900-specific callback.

        M.

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

Quoted 15 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Quoted 2 lines

Really? This means that you cannot mask an interrupt while it is being handled? How great...

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

        M.

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

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Here is the description of Interrupt Completion in PLIC spec [1]:

The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the

                                      ^^ ^^^^^^^^^ ^^^^^^^

completion is silently ignored.

[1] https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

Did we misunderstand the PLIC spec?

Quoted 41 lines · Guo Ren

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Here is the description of Interrupt Completion in PLIC spec [1]:

The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the

                                      ^^ ^^^^^^^^^ ^^^^^^^

completion is silently ignored.

Quoted 4 lines

[1] https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

Did we misunderstand the PLIC spec?

That clause sounds to me like it is due to the SiFive implementation, which the RISC-V PLIC specification is based on. Since the PLIC spec is still a draft I would expect it to change before release.

Quoted 43 lines · Darius Rad

Quoted 41 lines · Guo Ren

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Here is the description of Interrupt Completion in PLIC spec [1]:

The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the

                                      ^^ ^^^^^^^^^ ^^^^^^^

completion is silently ignored.

Quoted 8 lines

Quoted 4 lines

[1] https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

Did we misunderstand the PLIC spec?

That clause sounds to me like it is due to the SiFive implementation, which the RISC-V PLIC specification is based on. Since the PLIC spec is still a draft I would expect it to change before release.

The SiFive PLIC has been adopted by various RISC-V platforms (including SiFive themselves). Almost all existing RISC-V boards have PLIC as the interrupt controller.

Considering the wide usage of PLIC across existing platforms, the RISC-V International has adopted it as an official RISC-V non-ISA spec. ...

... Of course,
the RISC-V PLIC spec needs to follow the process for RISC-V non-ISA spec but changing the RISC-V PLIC spec now would mean all existing RISC-V platforms will become non-compliant.

The RISC-V AIA spec is intended to replace the RISC-V PLIC spec as the new interrupt controller spec for future RISC-V platforms.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 53 lines · Anup Patel

Quoted 51 lines · Darius Rad

Quoted 38 lines · Guo Ren

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Here is the description of Interrupt Completion in PLIC spec [1]:

The PLIC signals it has completed executing an interrupt handler by writing the interrupt ID it received from the claim to the claim/complete register. The PLIC does not check whether the completion ID is the same as the last claim ID for that target. If the completion ID does not match an interrupt source that is currently enabled for the target, the

Quoted 7 lines

                                      ^^ ^^^^^^^^^ ^^^^^^^

completion is silently ignored.

[1] https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

Did we misunderstand the PLIC spec?

That clause sounds to me like it is due to the SiFive implementation, which the RISC-V PLIC specification is based on. Since the PLIC spec is still a draft I would expect it to change before release.

Quoted 6 lines

The SiFive PLIC has been adopted by various RISC-V platforms (including SiFive themselves). Almost all existing RISC-V boards have PLIC as the interrupt controller.

Considering the wide usage of PLIC across existing platforms, the RISC-V International has adopted it as an official RISC-V non-ISA spec. ...

You mean is in the process of adopting it, right?

Quoted 6 lines

... Of course,
the RISC-V PLIC spec needs to follow the process for RISC-V non-ISA spec but changing the RISC-V PLIC spec now would mean all existing RISC-V platforms will become non-compliant.

I would expect the review process to produce a proper specification, rather than a verbatim copy of the SiFive datasheet, and clarify some ambgiuous and implementation specific language. Clarifying the specification does not necessarily make all existing implementations non-compliant, as this has been done numerous times with other RISC-V specifications.

Quoted 11 lines

The RISC-V AIA spec is intended to replace the RISC-V PLIC spec as the new interrupt controller spec for future RISC-V platforms.

Regards,
Anup

_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

        M.

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

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Yes, we are dealing with a broken/non-compliant PLIC implementation.

The RISC-V PLIC spec defines a very different behaviour for the interrupt claim (i.e. readl(claim)) and interrupt completion (i.e. writel(claim)). The T-HEAD PLIC implementation does things different from what the RISC-V PLIC spec says because it will mask an interrupt upon interrupt claim whereas PLIC spec says it should only clear the interrupt pending bit (not mask the interrupt).

Quoting interrupt claim process (chapter 9) from PLIC spec:
"The PLIC can perform an interrupt claim by reading the claim/complete register, which returns the ID of the highest priority pending interrupt or zero if there is no pending interrupt. A successful claim will also atomically clear the corresponding pending bit on the interrupt source."

Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

Quoted 48 lines · Anup Patel

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Yes, we are dealing with a broken/non-compliant PLIC implementation.

The RISC-V PLIC spec defines a very different behaviour for the interrupt claim (i.e. readl(claim)) and interrupt completion (i.e. writel(claim)). The T-HEAD PLIC implementation does things different from what the RISC-V PLIC spec says because it will mask an interrupt upon interrupt claim whereas PLIC spec says it should only clear the interrupt pending bit (not mask the interrupt).

Quoting interrupt claim process (chapter 9) from PLIC spec:
"The PLIC can perform an interrupt claim by reading the claim/complete register, which returns the ID of the highest priority pending interrupt or zero if there is no pending interrupt. A successful claim will also atomically clear the corresponding pending bit on the interrupt source."

Quoted 1 line

Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

That's not the point I'm making. According to Guo, the PLIC (any implementation of it) will ignore a write to claim on a masked interrupt.

If that's indeed correct, then a sequence such as:

(1) irq = read(claim)

(2) mask from the interrupt handler with the right flags so that it isn't done lazily
(3) write(irq, claim)

will result in an interrupt blocked in ack state (and probably no more interrupt for this CPU at this priority). That would be an interesting bug in the current code, but also a pretty bad architectural choice.

        M.

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

Quoted 51 lines · Marc Zyngier

Quoted 48 lines · Anup Patel

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Yes, we are dealing with a broken/non-compliant PLIC implementation.

The RISC-V PLIC spec defines a very different behaviour for the interrupt claim (i.e. readl(claim)) and interrupt completion (i.e. writel(claim)). The T-HEAD PLIC implementation does things different from what the RISC-V PLIC spec says because it will mask an interrupt upon interrupt claim whereas PLIC spec says it should only clear the interrupt pending bit (not mask the interrupt).

Quoting interrupt claim process (chapter 9) from PLIC spec:
"The PLIC can perform an interrupt claim by reading the claim/complete register, which returns the ID of the highest priority pending interrupt or zero if there is no pending interrupt. A successful claim will also atomically clear the corresponding pending bit on the interrupt source."

Quoted 5 lines

Quoted 1 line

Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

That's not the point I'm making. According to Guo, the PLIC (any implementation of it) will ignore a write to claim on a masked interrupt.

Yes, write to claim on a masked interrupt is certainly ignored but read to claim does not automatically mask the interrupt.

Quoted 4 lines

If that's indeed correct, then a sequence such as:

(1) irq = read(claim)

This will return highest priority pending interrupt and clear the pending bit as-per RISC-V PLIC spec.

Quoted 4 lines

(2) mask from the interrupt handler with the right flags so that it isn't done lazily
(3) write(irq, claim)

Quoted 3 lines

will result in an interrupt blocked in ack state (and probably no more interrupt for this CPU at this priority). That would be an interesting bug in the current code, but also a pretty bad architectural choice.

The interrupt claim/completion is for each interrupt and not at CPU level so if an interrupt is masked then only that interrupt is blocked for all CPUs but other interrupts can still be raised.

Quoted 78 lines · Anup Patel

Quoted 51 lines · Marc Zyngier

Quoted 48 lines · Anup Patel

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Yes, we are dealing with a broken/non-compliant PLIC implementation.

The RISC-V PLIC spec defines a very different behaviour for the interrupt claim (i.e. readl(claim)) and interrupt completion (i.e. writel(claim)). The T-HEAD PLIC implementation does things different from what the RISC-V PLIC spec says because it will mask an interrupt upon interrupt claim whereas PLIC spec says it should only clear the interrupt pending bit (not mask the interrupt).

Quoting interrupt claim process (chapter 9) from PLIC spec:
"The PLIC can perform an interrupt claim by reading the claim/complete register, which returns the ID of the highest priority pending interrupt or zero if there is no pending interrupt. A successful claim will also atomically clear the corresponding pending bit on the interrupt source."

Quoted 5 lines

Quoted 1 line

Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

That's not the point I'm making. According to Guo, the PLIC (any implementation of it) will ignore a write to claim on a masked interrupt.

Yes, write to claim on a masked interrupt is certainly ignored but read to claim does not automatically mask the interrupt.

Quoted 4 lines

If that's indeed correct, then a sequence such as:

(1) irq = read(claim)

This will return highest priority pending interrupt and clear the pending bit as-per RISC-V PLIC spec.

Quoted 7 lines

(2) mask from the interrupt handler with the right flags so that it isn't done lazily
(3) write(irq, claim)

will result in an interrupt blocked in ack state (and probably no more interrupt for this CPU at this priority). That would be an interesting bug in the current code, but also a pretty bad architectural choice.

Quoted 3 lines

The interrupt claim/completion is for each interrupt and not at CPU level so if an interrupt is masked then only that interrupt is blocked for all CPUs but other interrupts can still be raised.

Do you mean that another interrupt of the same priority will be able to be taken on *this* CPU, despite the completion being silently ignored?

        M.

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

Quoted 51 lines · Marc Zyngier

Quoted 48 lines · Anup Patel

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Yes, we are dealing with a broken/non-compliant PLIC implementation.

The RISC-V PLIC spec defines a very different behaviour for the interrupt claim (i.e. readl(claim)) and interrupt completion (i.e. writel(claim)). The T-HEAD PLIC implementation does things different from what the RISC-V PLIC spec says because it will mask an interrupt upon interrupt claim whereas PLIC spec says it should only clear the interrupt pending bit (not mask the interrupt).

Quoting interrupt claim process (chapter 9) from PLIC spec:
"The PLIC can perform an interrupt claim by reading the claim/complete register, which returns the ID of the highest priority pending interrupt or zero if there is no pending interrupt. A successful claim will also atomically clear the corresponding pending bit on the interrupt source."

Quoted 5 lines

Quoted 1 line

Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

That's not the point I'm making. According to Guo, the PLIC (any implementation of it) will ignore a write to claim on a masked interrupt.

Yes, write to claim on a masked interrupt is certainly ignored but read to claim does not automatically mask the interrupt.

Quoted 4 lines

If that's indeed correct, then a sequence such as:

(1) irq = read(claim)

This will return highest priority pending interrupt and clear the pending bit as-per RISC-V PLIC spec.

Quoted 4 lines

(2) mask from the interrupt handler with the right flags so that it isn't done lazily
(3) write(irq, claim)

Quoted 3 lines

will result in an interrupt blocked in ack state (and probably no more interrupt for this CPU at this priority). That would be an interesting bug in the current code, but also a pretty bad architectural choice.

The interrupt claim/completion is for each interrupt and not at CPU level so if an interrupt is masked then only that interrupt is blocked for all CPUs but other interrupts can still be raised.

Regards,
Anup

Quoted 5 lines

        M.

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

Quoted 42 lines · Anup Patel

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Yes, we are dealing with a broken/non-compliant PLIC implementation.

The RISC-V PLIC spec defines a very different behaviour for the interrupt claim (i.e. readl(claim)) and interrupt completion (i.e. writel(claim)). The T-HEAD PLIC implementation does things different from what the RISC-V PLIC spec says because it will mask an interrupt upon interrupt claim whereas PLIC spec says it should only clear the interrupt pending bit (not mask the interrupt).

Quoted 7 lines

Quoting interrupt claim process (chapter 9) from PLIC spec:
"The PLIC can perform an interrupt claim by reading the claim/complete register, which returns the ID of the highest priority pending interrupt or zero if there is no pending interrupt. A successful claim will also atomically clear the corresponding pending bit on the interrupt source."

Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

That's not the point I'm making. According to Guo, the PLIC (any implementation of it) will ignore a write to claim on a masked interrupt.

If that's indeed correct, then a sequence such as:

(1) irq = read(claim)
(2) mask from the interrupt handler with the right flags so that it isn't done lazily
(3) write(irq, claim)

will result in an interrupt blocked in ack state (and probably no more interrupt for this CPU at this priority). That would be an interesting bug in the current code, but also a pretty bad architectural choice.

        M.

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

Quoted 57 lines · Marc Zyngier

Quoted 42 lines · Anup Patel

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Yes, we are dealing with a broken/non-compliant PLIC implementation.

The RISC-V PLIC spec defines a very different behaviour for the interrupt claim (i.e. readl(claim)) and interrupt completion (i.e. writel(claim)). The T-HEAD PLIC implementation does things different from what the RISC-V PLIC spec says because it will mask an interrupt upon interrupt claim whereas PLIC spec says it should only clear the interrupt pending bit (not mask the interrupt).

Quoted 7 lines

Quoting interrupt claim process (chapter 9) from PLIC spec:
"The PLIC can perform an interrupt claim by reading the claim/complete register, which returns the ID of the highest priority pending interrupt or zero if there is no pending interrupt. A successful claim will also atomically clear the corresponding pending bit on the interrupt source."

Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

That's not the point I'm making. According to Guo, the PLIC (any implementation of it) will ignore a write to claim on a masked interrupt.

Quoted 6 lines

If that's indeed correct, then a sequence such as:

(1) irq = read(claim)
(2) mask from the interrupt handler with the right flags so that it isn't done lazily
(3) write(irq, claim)

How about letting the IRQ chip change?

kernel/irq/chip.c +3 -3

index a98bcfc4be7b..ed6ace1058ac 100644
@@ -444,10 +444,10 @@ void unmask_threaded_irq(struct irq_desc *desc)
 {
        struct irq_chip *chip = desc->irq_data.chip;
 
+       unmask_irq(desc);
+
        if (chip->flags & IRQCHIP_EOI_THREADED)
                chip->irq_eoi(&desc->irq_data);
-
-       unmask_irq(desc);
 }
 
 /*

@@ -673,8 +673,8 @@ static void cond_unmask_eoi_irq(struct irq_desc
 *desc, struct irq_chip *chip)
         */
        if (!irqd_irq_disabled(&desc->irq_data) &&
            irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
-               chip->irq_eoi(&desc->irq_data);
                unmask_irq(desc);
+               chip->irq_eoi(&desc->irq_data);
        } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
                chip->irq_eoi(&desc->irq_data);

        }

Quoted 93 lines · Guo Ren

Quoted 57 lines · Marc Zyngier

Quoted 42 lines · Anup Patel

Quoted 22 lines · Marc Zyngier

Quoted 19 lines · Guo Ren

Quoted 17 lines · Marc Zyngier

Quoted 10 lines · Guo Ren

Quoted 6 lines

If you have an 'automask' behavior and yet the HW doesn't record this in a separate bit, then you need to track this by yourself in the irq_eoi() callback instead. I guess that you would skip the write to the CLAIM register in this case, though I have no idea whether this breaks
the HW interrupt state or not.

The problem is when enable bit is 0 for that irq_number, "writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM)" wouldn't affect the hw state machine. Then this irq would enter in ack state and no continues irqs could come in.

Really? This means that you cannot mask an interrupt while it is being handled? How great...

Quoted 7 lines

Quoted 3 lines

If the completion ID does not match an interrupt source that is currently enabled for the target, the completion is silently ignored. So, C9xx completion depends on enable-bit.

Is that what the PLIC spec says? Or what your implementation does? I can understand that one implementation would be broken, but if the PLIC architecture itself is broken, that's far more concerning.

Yes, we are dealing with a broken/non-compliant PLIC implementation.

The RISC-V PLIC spec defines a very different behaviour for the interrupt claim (i.e. readl(claim)) and interrupt completion (i.e. writel(claim)). The T-HEAD PLIC implementation does things different from what the RISC-V PLIC spec says because it will mask an interrupt upon interrupt claim whereas PLIC spec says it should only clear the interrupt pending bit (not mask the interrupt).

Quoted 7 lines

Quoting interrupt claim process (chapter 9) from PLIC spec:
"The PLIC can perform an interrupt claim by reading the claim/complete register, which returns the ID of the highest priority pending interrupt or zero if there is no pending interrupt. A successful claim will also atomically clear the corresponding pending bit on the interrupt source."

Refer, https://github.com/riscv/riscv-plic-spec/blob/master/riscv-plic.adoc

That's not the point I'm making. According to Guo, the PLIC (any implementation of it) will ignore a write to claim on a masked interrupt.

Quoted 6 lines

If that's indeed correct, then a sequence such as:

(1) irq = read(claim)
(2) mask from the interrupt handler with the right flags so that it isn't done lazily
(3) write(irq, claim)

How about letting the IRQ chip change?

kernel/irq/chip.c +3 -3

index a98bcfc4be7b..ed6ace1058ac 100644
@@ -444,10 +444,10 @@ void unmask_threaded_irq(struct irq_desc *desc)
 {
        struct irq_chip *chip = desc->irq_data.chip;
 
+       unmask_irq(desc);
+
        if (chip->flags & IRQCHIP_EOI_THREADED)
                chip->irq_eoi(&desc->irq_data);
-
-       unmask_irq(desc);
 }
 
 /*

@@ -673,8 +673,8 @@ static void cond_unmask_eoi_irq(struct irq_desc
 *desc, struct irq_chip *chip)
         */
        if (!irqd_irq_disabled(&desc->irq_data) &&
            irqd_irq_masked(&desc->irq_data) && !desc->threads_oneshot) {
-               chip->irq_eoi(&desc->irq_data);
                unmask_irq(desc);
+               chip->irq_eoi(&desc->irq_data);

Quoted 3 lines

kernel/irq/chip.c +0 -0

        } else if (!(chip->flags & IRQCHIP_EOI_THREADED)) {
                chip->irq_eoi(&desc->irq_data);

        }

No, I don't think that's acceptable, and I strongly suspect that other irqchips have the opposite requirement. You'll have to keep the workaround in the PLIC code and track the EOI vs unmask to do the right thing in both callbacks.

        M.

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

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml description: · line 41

Quoted 4 lines · On Okt 16 2021, [email protected] wrote:

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +4 -0

+  The C9xx PLIC does not comply with the interrupt claim/completion process defined
+  by the RISC-V PLIC specification because C9xx PLIC will mask an IRQ when it is
+  claimed by PLIC driver (i.e. readl(claim) and the IRQ will be unmasked upon
+  completion by PLIC driver (i.e. writel(claim). This behaviour breaks the handling

Missing close paren in both parenthetical remarks above.

Andreas.

--
Andreas Schwab, [email protected]
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1 "And now for something completely different."

From: Guo Ren <[email protected]>

Add the compatible string "thead,c900-plic" to the riscv plic bindings to support allwinner d1 SOC which contains c906 core.

Signed-off-by Guo Ren <[email protected]>

Cc Rob Herring <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Anup Patel <[email protected]>

Cc Atish Patra <[email protected]>

---

Changes since V4:
- Update description in errata style
- Update enum suggested by Anup, Heiko, Samuel

Changes since V3:
- Rename "c9xx" to "c900"
- Add thead,c900-plic in the description section

---
 .../interrupt-controller/sifive,plic-1.0.0.yaml       | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +6 -0

index 08d5a57ce00f..272f29540135 100644
@@ -35,6 +35,12 @@ description:
   contains a specific memory layout, which is documented in chapter 8 of the
   SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
 
+  The C9xx PLIC does not comply with the interrupt claim/completion process defined
+  by the RISC-V PLIC specification because C9xx PLIC will mask an IRQ when it is
+  claimed by PLIC driver (i.e. readl(claim) and the IRQ will be unmasked upon
+  completion by PLIC driver (i.e. writel(claim). This behaviour breaks the handling
+  of IRQS_ONESHOT by the generic handle_fasteoi_irq() used in the PLIC driver.
+
 maintainers:
   - Sagar Kadam <[email protected]>
   - Paul Walmsley  <[email protected]>

Hi Guo,

Am Samstag, 16. Oktober 2021, 05:21:59 CEST schrieb [email protected]:

Quoted 4 lines

From: Guo Ren <[email protected]>

Add the compatible string "thead,c900-plic" to the riscv plic bindings to support allwinner d1 SOC which contains c906 core.

The compatible strings sound good now, but some things below

Quoted 38 lines

Signed-off-by Guo Ren <[email protected]>

Cc Rob Herring <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Anup Patel <[email protected]>

Cc Atish Patra <[email protected]>

---

Changes since V4:
- Update description in errata style
- Update enum suggested by Anup, Heiko, Samuel

Changes since V3:
- Rename "c9xx" to "c900"
- Add thead,c900-plic in the description section

---
 .../interrupt-controller/sifive,plic-1.0.0.yaml       | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +6 -0

index 08d5a57ce00f..272f29540135 100644
@@ -35,6 +35,12 @@ description:
   contains a specific memory layout, which is documented in chapter 8 of the
   SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
 
+  The C9xx PLIC does not comply with the interrupt claim/completion process defined
+  by the RISC-V PLIC specification because C9xx PLIC will mask an IRQ when it is
+  claimed by PLIC driver (i.e. readl(claim) and the IRQ will be unmasked upon
+  completion by PLIC driver (i.e. writel(claim). This behaviour breaks the handling
+  of IRQS_ONESHOT by the generic handle_fasteoi_irq() used in the PLIC driver.
+
 maintainers:
   - Sagar Kadam <[email protected]>
   - Paul Walmsley  <[email protected]>

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml properties: · line 55

Quoted 5 lines

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +1 -1

@@ -46,7 +52,10 @@ properties:
       - enum:
           - sifive,fu540-c000-plic
           - canaan,k210-plic
-      - const: sifive,plic-1.0.0
+      - enmu:

^ spelling enum

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml properties: · line 58

Quoted 3 lines

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +3 -0

+          - sifive,plic-1.0.0
+          - thead,c900-plic
+          - allwinner,sun20i-d1-plic

but in general I'd think that you want something like

compatible:
oneOf:
- items:
- enum:
- sifive,fu540-c000-plic
- canaan,k210-plic
- const: sifive,plic-1.0.0
- items:
- enum:
- allwinner,sun20i-d1-plic
- const: thead,c900-plic

Having only one item list would allow as valid combinations like "sifive,fu540-c000-plic", "thead,c900-plic" when checking the schema.

With the oneOf and separate lists we can make sure that such "illegal" combinations get flagged by the dtbs_check

[the enum with the single allwinner entry already leaves
room for later addition to the c900-plic variant]

Heiko

Quoted 75 lines

Hi Guo,

Am Samstag, 16. Oktober 2021, 05:21:59 CEST schrieb [email protected]:

Quoted 4 lines

From: Guo Ren <[email protected]>

Add the compatible string "thead,c900-plic" to the riscv plic bindings to support allwinner d1 SOC which contains c906 core.

The compatible strings sound good now, but some things below

Quoted 38 lines

Signed-off-by Guo Ren <[email protected]>

Cc Rob Herring <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Anup Patel <[email protected]>

Cc Atish Patra <[email protected]>

---

Changes since V4:
- Update description in errata style
- Update enum suggested by Anup, Heiko, Samuel

Changes since V3:
- Rename "c9xx" to "c900"
- Add thead,c900-plic in the description section

---
 .../interrupt-controller/sifive,plic-1.0.0.yaml       | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +6 -0

index 08d5a57ce00f..272f29540135 100644
@@ -35,6 +35,12 @@ description:
   contains a specific memory layout, which is documented in chapter 8 of the
   SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
 
+  The C9xx PLIC does not comply with the interrupt claim/completion process defined
+  by the RISC-V PLIC specification because C9xx PLIC will mask an IRQ when it is
+  claimed by PLIC driver (i.e. readl(claim) and the IRQ will be unmasked upon
+  completion by PLIC driver (i.e. writel(claim). This behaviour breaks the handling
+  of IRQS_ONESHOT by the generic handle_fasteoi_irq() used in the PLIC driver.
+
 maintainers:
   - Sagar Kadam <[email protected]>
   - Paul Walmsley  <[email protected]>

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml properties: · line 55

Quoted 5 lines

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +1 -1

@@ -46,7 +52,10 @@ properties:
       - enum:
           - sifive,fu540-c000-plic
           - canaan,k210-plic
-      - const: sifive,plic-1.0.0
+      - enmu:

^ spelling enum

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml properties: · line 58

Quoted 3 lines

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +3 -0

+          - sifive,plic-1.0.0
+          - thead,c900-plic
+          - allwinner,sun20i-d1-plic

but in general I'd think that you want something like

compatible:
oneOf:
- items:
- enum:
- sifive,fu540-c000-plic
- canaan,k210-plic
- const: sifive,plic-1.0.0
- items:
- enum:
- allwinner,sun20i-d1-plic
- const: thead,c900-plic

Quoted 8 lines

Having only one item list would allow as valid combinations like "sifive,fu540-c000-plic", "thead,c900-plic" when checking the schema.

With the oneOf and separate lists we can make sure that such "illegal" combinations get flagged by the dtbs_check

[the enum with the single allwinner entry already leaves
room for later addition to the c900-plic variant]

Thx, I'll fix it in the next version.

another question: Is the allwinner_sun20i_d1_plic needed to IRQCHIP_DECLARE?

(quoted diff) +2 -0

+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", thead_c900_plic_init);
+IRQCHIP_DECLARE(allwinner_sun20i_d1_plic, "allwinner,sun20i-d1-plic",

thead_c900_plic_init);

Am Samstag, 16. Oktober 2021, 14:56:51 CEST schrieb Guo Ren:

Quoted 90 lines · Heiko Stuebner

Quoted 75 lines

Hi Guo,

Am Samstag, 16. Oktober 2021, 05:21:59 CEST schrieb [email protected]:

Quoted 4 lines

From: Guo Ren <[email protected]>

Add the compatible string "thead,c900-plic" to the riscv plic bindings to support allwinner d1 SOC which contains c906 core.

The compatible strings sound good now, but some things below

Quoted 38 lines

Signed-off-by Guo Ren <[email protected]>

Cc Rob Herring <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Anup Patel <[email protected]>

Cc Atish Patra <[email protected]>

---

Changes since V4:
- Update description in errata style
- Update enum suggested by Anup, Heiko, Samuel

Changes since V3:
- Rename "c9xx" to "c900"
- Add thead,c900-plic in the description section

---
 .../interrupt-controller/sifive,plic-1.0.0.yaml       | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +6 -0

index 08d5a57ce00f..272f29540135 100644
@@ -35,6 +35,12 @@ description:
   contains a specific memory layout, which is documented in chapter 8 of the
   SiFive U5 Coreplex Series Manual <https://static.dev.sifive.com/U54-MC-RVCoreIP.pdf>.
 
+  The C9xx PLIC does not comply with the interrupt claim/completion process defined
+  by the RISC-V PLIC specification because C9xx PLIC will mask an IRQ when it is
+  claimed by PLIC driver (i.e. readl(claim) and the IRQ will be unmasked upon
+  completion by PLIC driver (i.e. writel(claim). This behaviour breaks the handling
+  of IRQS_ONESHOT by the generic handle_fasteoi_irq() used in the PLIC driver.
+
 maintainers:
   - Sagar Kadam <[email protected]>
   - Paul Walmsley  <[email protected]>

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml properties: · line 55

Quoted 5 lines

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +1 -1

@@ -46,7 +52,10 @@ properties:
       - enum:
           - sifive,fu540-c000-plic
           - canaan,k210-plic
-      - const: sifive,plic-1.0.0
+      - enmu:

^ spelling enum

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml properties: · line 58

Quoted 3 lines

Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml +3 -0

+          - sifive,plic-1.0.0
+          - thead,c900-plic
+          - allwinner,sun20i-d1-plic

but in general I'd think that you want something like

compatible:
oneOf:
- items:
- enum:
- sifive,fu540-c000-plic
- canaan,k210-plic
- const: sifive,plic-1.0.0
- items:
- enum:
- allwinner,sun20i-d1-plic
- const: thead,c900-plic

Quoted 8 lines

Having only one item list would allow as valid combinations like "sifive,fu540-c000-plic", "thead,c900-plic" when checking the schema.

With the oneOf and separate lists we can make sure that such "illegal" combinations get flagged by the dtbs_check

[the enum with the single allwinner entry already leaves
room for later addition to the c900-plic variant]

Thx, I'll fix it in the next version.

another question: Is the allwinner_sun20i_d1_plic needed to IRQCHIP_DECLARE?

(quoted diff) +2 -0

+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", thead_c900_plic_init);
+IRQCHIP_DECLARE(allwinner_sun20i_d1_plic, "allwinner,sun20i-d1-plic",

Quoted 1 line

thead_c900_plic_init);

Doing

        IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", thead_c900_plic_init);

should be enough for now.

Compatible-parsing happens from left to right, from most-specific to most-generic. So having the allwinner-d1 compatible in there is sort of a safeguard.

If at some _later point in time_ , some specific new quirk of the D1 implementation comes to light, we can _then_ just add a

        IRQCHIP_DECLARE(allwinner_d1_plic, "allwinner,sun20i-d1-plic", allwinner_d1_plic_init);

Devicetrees should be stable and newer kernels should work with old devicetrees, so having the soc-specific compatible in there just makes it future proof :-)

Heiko

From: Guo Ren <[email protected]>

Add the compatible string "thead,c900-plic" to the riscv plic bindings to support allwinner d1 SOC which contains c906 core.

Signed-off-by Guo Ren <[email protected]>

Cc Rob Herring <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Anup Patel <[email protected]>

Cc Atish Patra <[email protected]>

Quoted 11 lines · On Fri, Oct 15, 2021 at 10:22 PM <[email protected]> wrote:

From: Guo Ren <[email protected]>

Add the compatible string "thead,c900-plic" to the riscv plic bindings to support allwinner d1 SOC which contains c906 core.

Signed-off-by Guo Ren <[email protected]>

Cc Rob Herring <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Anup Patel <[email protected]>

Cc Atish Patra <[email protected]>

Please send to the DT list so that checks run and it's in my review queue (IOW, use get_maintainers.pl). And run 'make dt_binding_check' so reviewers don't have to find your typos and other errors for you.

Rob