Add thead,c900-plic support

PATCH Superseded [email protected] 3 participants 11 messages Jan 29, 2022 – Jan 30, 2022

Subsystems (4)

Device tree bindings RISC-V irqchip drivers SiFive drivers

lore

Share & tools

lore

[email protected]

author

2022-01-29 16:27 UTC #0 lore raw

v6 cover Add thead,c900-plic support

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.

Changes since V6:
- Modify sifive,plic-1.0.0.yaml comment
- Remove vendor-prefixes patch which has been merged
- Only put dt naming distinguish in sifive-plic.c

Changes since V5:
- Move back to mask/unmask
- Fixup the problem in eoi callback
- Remove allwinner,sun20i-d1 IRQCHIP_DECLARE
- Rewrite comment log
- Add DT list
- Fixup compatible string
- Remove allwinner-d1 compatible
- make dt_binding_check
- Add T-head vendor-prefixes

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

Changes since V3:
- Rename "c9xx" to "c900"
- Add thead,c900-plic in the description section
- 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.

Guo Ren (2):
dt-bindings: update riscv plic compatible string irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

 .../sifive,plic-1.0.0.yaml                    | 21 +++++++++++-----
 drivers/irqchip/irq-sifive-plic.c             | 25 +++++++++++++++++--
 2 files changed, 38 insertions(+), 8 deletions(-)

Signature · 1 line

2.25.1

[email protected]

author

2022-01-29 16:27 UTC #1 lore raw

PATCH v6 1/2 dt-bindings: update riscv plic compatible string

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 Anup Patel <[email protected]>

Cc Heiko Stuebner <[email protected]>

Cc Rob Herring <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Samuel Holland <[email protected]>

---
 .../sifive,plic-1.0.0.yaml                    | 21 +++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

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

index 28b6b17fe4b2..1fa5aa7e4c2e 100644
@@ -35,6 +35,10 @@ 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 thead,c900-plic is different from sifive,plic-1.0.0 in opensbi, the
+  T-HEAD PLIC implementation requires setting a delegation bit to allow access
+  from S-mode. So add thead,c900-plic to distinguish them.
+
 maintainers:
   - Sagar Kadam <[email protected]>
   - Paul Walmsley  <[email protected]>

@@ -42,12 +46,17 @@ maintainers:
 
 properties:
   compatible:
-    items:
-      - enum:
-          - sifive,fu540-c000-plic
-          - starfive,jh7100-plic
-          - canaan,k210-plic
-      - const: sifive,plic-1.0.0
+    oneOf:
+      - items:
+          - enum:
+              - sifive,fu540-c000-plic
+              - starfive,jh7100-plic
+              - canaan,k210-plic
+          - const: sifive,plic-1.0.0
+      - items:
+          - enum:
+              - allwinner,sun20i-d1-plic
+          - const: thead,c900-plic
 
   reg:
     maxItems: 1

Signature · 1 line

2.25.1

[email protected]

author

2022-01-29 16:28 UTC #2 lore raw

PATCH v6 2/2 irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

In reply to [email protected]

From: Guo Ren <[email protected]>

The thead,c900-plic has been used in opensbi to distinguish PLIC [1]. Although PLICs have the same behaviors in Linux, they are different hardware with some custom initializing in firmware(opensbi).

[1]: https://github.com/riscv-software-src/opensbi/commit/78c2b19218bd62653b9fb31623a42ced45f38ea6

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

Cc Anup Patel <[email protected]>

Cc Marc Zyngier <[email protected]>

Cc Palmer Dabbelt <[email protected]>

Cc Samuel Holland <[email protected]>

Cc Thomas Gleixner <[email protected]>

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

drivers/irqchip/irq-sifive-plic.c +23 -2

index 259065d271ef..245655928076 100644
@@ -172,7 +172,7 @@ static void plic_irq_eoi(struct irq_data *d)
 	}
 }
 
-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,

@@ -182,12 +182,24 @@ static struct irq_chip plic_chip = {
 #endif
 };
 
+static struct irq_chip thead_plic_chip = {
+	.name		= "T-Head PLIC",
+	.irq_mask	= plic_irq_mask,
+	.irq_unmask	= plic_irq_unmask,
+	.irq_eoi	= plic_irq_eoi,
+#ifdef CONFIG_SMP
+	.irq_set_affinity = plic_set_affinity,
+#endif
+};
+
+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);

@@ -396,5 +408,14 @@ 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);

Signature · 1 line

2.25.1

Marc Zyngier

2022-01-29 18:32 UTC #3 lore raw

Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

Quoted 36 lines · On Sat, 29 Jan 2022 16:27:26 +0000, [email protected] wrote:

From: Guo Ren <[email protected]>
The thead,c900-plic has been used in opensbi to distinguish PLIC [1]. Although PLICs have the same behaviors in Linux, they are different hardware with some custom initializing in firmware(opensbi).
[1]: https://github.com/riscv-software-src/opensbi/commit/78c2b19218bd62653b9fb31623a42ced45f38ea6
Signed-off-by Guo Ren <[email protected]>
Cc Anup Patel <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Palmer Dabbelt <[email protected]>
Cc Samuel Holland <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/irqchip/irq-sifive-plic.c +1 -1
index 259065d271ef..245655928076 100644
@@ -172,7 +172,7 @@ static void plic_irq_eoi(struct irq_data *d)
 	}
 }
 
-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,
@@ -182,12 +182,24 @@ static struct irq_chip plic_chip = {
 #endif
 };

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

Quoted 10 lines

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

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

For pure entertainment, let's compare the two structures:

static struct irq_chip plic_chip = {
	.name		= "SiFive PLIC",
	.irq_mask	= plic_irq_mask,
	.irq_unmask	= plic_irq_unmask,
	.irq_eoi	= plic_irq_eoi,

#ifdef CONFIG_SMP
.irq_set_affinity = plic_set_affinity,
#endif
};

Oh wait: a string. Must be really important. Not.

Quoted 18 lines

drivers/irqchip/irq-sifive-plic.c +5 -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);

@@ -396,5 +408,14 @@ static int __init plic_init(struct device_node *node,
 	return error;
 }
 
+static int __init thead_c900_plic_init(struct device_node *node,

drivers/irqchip/irq-sifive-plic.c static int __init plic_init(struct device_node *node, · line 421

Quoted 10 lines

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

+		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);

Sorry, but I can't see any point to this patch.

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Guo Ren

author

2022-01-30 02:08 UTC #4 lore raw

Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

Quoted 57 lines · Marc Zyngier

Quoted 36 lines · On Sat, 29 Jan 2022 16:27:26 +0000, [email protected] wrote:
From: Guo Ren <[email protected]>
The thead,c900-plic has been used in opensbi to distinguish PLIC [1]. Although PLICs have the same behaviors in Linux, they are different hardware with some custom initializing in firmware(opensbi).
[1]: https://github.com/riscv-software-src/opensbi/commit/78c2b19218bd62653b9fb31623a42ced45f38ea6
Signed-off-by Guo Ren <[email protected]>
Cc Anup Patel <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Palmer Dabbelt <[email protected]>
Cc Samuel Holland <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/irqchip/irq-sifive-plic.c +1 -1
index 259065d271ef..245655928076 100644
@@ -172,7 +172,7 @@ static void plic_irq_eoi(struct irq_data *d)
      }
 }
 
-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,
@@ -182,12 +182,24 @@ static struct irq_chip plic_chip = {
 #endif
 };
drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 193
Quoted 10 lines
drivers/irqchip/irq-sifive-plic.c +9 -0
 
+static struct irq_chip thead_plic_chip = {
+     .name           = "T-Head PLIC",
+     .irq_mask       = plic_irq_mask,
+     .irq_unmask     = plic_irq_unmask,
+     .irq_eoi        = plic_irq_eoi,
+#ifdef CONFIG_SMP
+     .irq_set_affinity = plic_set_affinity,
+#endif
+};
For pure entertainment, let's compare the two structures:
static struct irq_chip plic_chip = {
        .name           = "SiFive PLIC",
        .irq_mask       = plic_irq_mask,
        .irq_unmask     = plic_irq_unmask,
        .irq_eoi        = plic_irq_eoi,

Quoted 6 lines

#ifdef CONFIG_SMP
.irq_set_affinity = plic_set_affinity,
#endif
};
Oh wait: a string. Must be really important. Not.

No, pls see below comment.

Quoted 18 lines

drivers/irqchip/irq-sifive-plic.c +4 -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);

@@ -396,5 +408,14 @@ static int __init plic_init(struct device_node *node,
      return error;
 }

Quoted 13 lines

drivers/irqchip/irq-sifive-plic.c static int __init plic_init(struct device_node *node, · line 421

Quoted 11 lines

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

+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);

Sorry, but I can't see any point to this patch.

You didn't see the link I've put in the patch. In that opensbi patch:

                intc: interrupt-controller@10000000 {
                        #interrupt-cells = <1>;

- compatible = "riscv,plic0";
+ compatible = "allwinner,sun20i-d1-plic",
+ "thead,c900-plic";

+#define THEAD_PLIC_CTRL_REG 0x1ffffc
+
+static void thead_plic_plat_init(struct plic_data *pd) +{
+ writel_relaxed(BIT(0), (void *)pd->addr + THEAD_PLIC_CTRL_REG);
+}
+

 static const struct fdt_match irqchip_plic_match[] = {
        { .compatible = "riscv,plic0" },
        { .compatible = "sifive,plic-1.0.0" },
+       { .compatible = "thead,c900-plic",
+         .data = thead_plic_plat_init },
        { },

};

We've changed the compatible name for thead,c900-plic, and there is no riscv,plic0 / sifive,plic-1.0.0 in dts. Without the patch, the newest opensbi + newest Linux would be broken in the Allwinner D1 dev board.

Quoted 5 lines

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

Signature · 4 lines

Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Samuel Holland

2022-01-30 02:51 UTC #5 lore raw

Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

Quoted 120 lines · Guo Ren

Quoted 57 lines · Marc Zyngier

Quoted 36 lines · On Sat, 29 Jan 2022 16:27:26 +0000, [email protected] wrote:
From: Guo Ren <[email protected]>
The thead,c900-plic has been used in opensbi to distinguish PLIC [1]. Although PLICs have the same behaviors in Linux, they are different hardware with some custom initializing in firmware(opensbi).
[1]: https://github.com/riscv-software-src/opensbi/commit/78c2b19218bd62653b9fb31623a42ced45f38ea6
Signed-off-by Guo Ren <[email protected]>
Cc Anup Patel <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Palmer Dabbelt <[email protected]>
Cc Samuel Holland <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/irqchip/irq-sifive-plic.c +1 -1
index 259065d271ef..245655928076 100644
@@ -172,7 +172,7 @@ static void plic_irq_eoi(struct irq_data *d)
      }
 }
 
-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,
@@ -182,12 +182,24 @@ static struct irq_chip plic_chip = {
 #endif
 };
drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 193
Quoted 10 lines
drivers/irqchip/irq-sifive-plic.c +9 -0
 
+static struct irq_chip thead_plic_chip = {
+     .name           = "T-Head PLIC",
+     .irq_mask       = plic_irq_mask,
+     .irq_unmask     = plic_irq_unmask,
+     .irq_eoi        = plic_irq_eoi,
+#ifdef CONFIG_SMP
+     .irq_set_affinity = plic_set_affinity,
+#endif
+};
For pure entertainment, let's compare the two structures:
static struct irq_chip plic_chip = {
        .name           = "SiFive PLIC",
        .irq_mask       = plic_irq_mask,
        .irq_unmask     = plic_irq_unmask,
        .irq_eoi        = plic_irq_eoi,

Quoted 6 lines

#ifdef CONFIG_SMP
.irq_set_affinity = plic_set_affinity,
#endif
};
Oh wait: a string. Must be really important. Not.

No, pls see below comment.

Quoted 18 lines

drivers/irqchip/irq-sifive-plic.c +4 -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);

@@ -396,5 +408,14 @@ static int __init plic_init(struct device_node *node,
      return error;
 }

Quoted 13 lines

drivers/irqchip/irq-sifive-plic.c static int __init plic_init(struct device_node *node, · line 421

Quoted 11 lines

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

+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);

Sorry, but I can't see any point to this patch.

You didn't see the link I've put in the patch. In that opensbi patch:

                intc: interrupt-controller@10000000 {
                        #interrupt-cells = <1>;

(quoted diff) +11 -1

-                       compatible = "riscv,plic0";
+                       compatible = "allwinner,sun20i-d1-plic",
+                                    "thead,c900-plic";
 
+#define THEAD_PLIC_CTRL_REG 0x1ffffc
+
+static void thead_plic_plat_init(struct plic_data *pd)
+{
+       writel_relaxed(BIT(0), (void *)pd->addr + THEAD_PLIC_CTRL_REG);
+}
+
 static const struct fdt_match irqchip_plic_match[] = {
        { .compatible = "riscv,plic0" },
        { .compatible = "sifive,plic-1.0.0" },
+       { .compatible = "thead,c900-plic",
+         .data = thead_plic_plat_init },
        { },
 };

Quoted 3 lines

We've changed the compatible name for thead,c900-plic, and there is no riscv,plic0 / sifive,plic-1.0.0 in dts. Without the patch, the newest opensbi + newest Linux would be broken in the Allwinner D1 dev board.

Yes, some patch is still necessary, because the hardware is indeed incompatible with riscv,plic0. However, this driver does not care about the difference. So all you need to do is hook up the existing code to the new compatible:

+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_init);

Regards,
Samuel

Guo Ren

author

2022-01-30 04:39 UTC #6 lore raw

Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

Quoted 130 lines · Samuel Holland

Quoted 120 lines · Guo Ren
Quoted 57 lines · Marc Zyngier
Quoted 36 lines · On Sat, 29 Jan 2022 16:27:26 +0000, [email protected] wrote:
From: Guo Ren <[email protected]>
The thead,c900-plic has been used in opensbi to distinguish PLIC [1]. Although PLICs have the same behaviors in Linux, they are different hardware with some custom initializing in firmware(opensbi).
[1]: https://github.com/riscv-software-src/opensbi/commit/78c2b19218bd62653b9fb31623a42ced45f38ea6
Signed-off-by Guo Ren <[email protected]>
Cc Anup Patel <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Palmer Dabbelt <[email protected]>
Cc Samuel Holland <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/irqchip/irq-sifive-plic.c +1 -1
index 259065d271ef..245655928076 100644
@@ -172,7 +172,7 @@ static void plic_irq_eoi(struct irq_data *d)
      }
 }
 
-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,
@@ -182,12 +182,24 @@ static struct irq_chip plic_chip = {
 #endif
 };
drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 193
Quoted 10 lines
drivers/irqchip/irq-sifive-plic.c +9 -0
 
+static struct irq_chip thead_plic_chip = {
+     .name           = "T-Head PLIC",
+     .irq_mask       = plic_irq_mask,
+     .irq_unmask     = plic_irq_unmask,
+     .irq_eoi        = plic_irq_eoi,
+#ifdef CONFIG_SMP
+     .irq_set_affinity = plic_set_affinity,
+#endif
+};
For pure entertainment, let's compare the two structures:
static struct irq_chip plic_chip = {
        .name           = "SiFive PLIC",
        .irq_mask       = plic_irq_mask,
        .irq_unmask     = plic_irq_unmask,
        .irq_eoi        = plic_irq_eoi,
Quoted 6 lines
#ifdef CONFIG_SMP
.irq_set_affinity = plic_set_affinity,
#endif
};
Oh wait: a string. Must be really important. Not.
No, pls see below comment.
Quoted 18 lines
Quoted 18 lines
drivers/irqchip/irq-sifive-plic.c +4 -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);
@@ -396,5 +408,14 @@ static int __init plic_init(struct device_node *node,
      return error;
 }
 
Quoted 13 lines
drivers/irqchip/irq-sifive-plic.c static int __init plic_init(struct device_node *node, · line 421
Quoted 11 lines
drivers/irqchip/irq-sifive-plic.c +9 -0
+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);
Sorry, but I can't see any point to this patch.
You didn't see the link I've put in the patch. In that opensbi patch:
                intc: interrupt-controller@10000000 {
                        #interrupt-cells = <1>;
(quoted diff) +11 -1
-                       compatible = "riscv,plic0";
+                       compatible = "allwinner,sun20i-d1-plic",
+                                    "thead,c900-plic";
 
+#define THEAD_PLIC_CTRL_REG 0x1ffffc
+
+static void thead_plic_plat_init(struct plic_data *pd)
+{
+       writel_relaxed(BIT(0), (void *)pd->addr + THEAD_PLIC_CTRL_REG);
+}
+
 static const struct fdt_match irqchip_plic_match[] = {
        { .compatible = "riscv,plic0" },
        { .compatible = "sifive,plic-1.0.0" },
+       { .compatible = "thead,c900-plic",
+         .data = thead_plic_plat_init },
        { },
 };
Quoted 3 lines
We've changed the compatible name for thead,c900-plic, and there is no riscv,plic0 / sifive,plic-1.0.0 in dts. Without the patch, the newest opensbi + newest Linux would be broken in the Allwinner D1 dev board.
Yes, some patch is still necessary, because the hardware is indeed incompatible with riscv,plic0. However, this driver does not care about the difference. So all you need to do is hook up the existing code to the new compatible:

Quoted 1 line

+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_init);

I think we should give clear info in /proc/interrupts. I hope we could keep thead_plic_init.

Quoted 3 lines

Regards,
Samuel

Signature · 4 lines

Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Marc Zyngier

2022-01-30 10:28 UTC #7 lore raw

Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

Quoted 101 lines · Guo Ren

Quoted 99 lines · Samuel Holland
Quoted 97 lines · Guo Ren
Quoted 57 lines · Marc Zyngier
Quoted 36 lines · On Sat, 29 Jan 2022 16:27:26 +0000, [email protected] wrote:
From: Guo Ren <[email protected]>
The thead,c900-plic has been used in opensbi to distinguish PLIC [1]. Although PLICs have the same behaviors in Linux, they are different hardware with some custom initializing in firmware(opensbi).
[1]: https://github.com/riscv-software-src/opensbi/commit/78c2b19218bd62653b9fb31623a42ced45f38ea6
Signed-off-by Guo Ren <[email protected]>
Cc Anup Patel <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Palmer Dabbelt <[email protected]>
Cc Samuel Holland <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/irqchip/irq-sifive-plic.c +1 -1
index 259065d271ef..245655928076 100644
@@ -172,7 +172,7 @@ static void plic_irq_eoi(struct irq_data *d)
      }
 }
 
-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,
@@ -182,12 +182,24 @@ static struct irq_chip plic_chip = {
 #endif
 };
drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 193
Quoted 10 lines
drivers/irqchip/irq-sifive-plic.c +9 -0
 
+static struct irq_chip thead_plic_chip = {
+     .name           = "T-Head PLIC",
+     .irq_mask       = plic_irq_mask,
+     .irq_unmask     = plic_irq_unmask,
+     .irq_eoi        = plic_irq_eoi,
+#ifdef CONFIG_SMP
+     .irq_set_affinity = plic_set_affinity,
+#endif
+};
For pure entertainment, let's compare the two structures:
static struct irq_chip plic_chip = {
        .name           = "SiFive PLIC",
        .irq_mask       = plic_irq_mask,
        .irq_unmask     = plic_irq_unmask,
        .irq_eoi        = plic_irq_eoi,
Quoted 6 lines
#ifdef CONFIG_SMP
.irq_set_affinity = plic_set_affinity,
#endif
};
Oh wait: a string. Must be really important. Not.
No, pls see below comment.
Quoted 31 lines
Quoted 19 lines
drivers/irqchip/irq-sifive-plic.c +5 -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);
@@ -396,5 +408,14 @@ static int __init plic_init(struct device_node *node,
      return error;
 }
 
+static int __init thead_c900_plic_init(struct device_node *node,
drivers/irqchip/irq-sifive-plic.c static int __init plic_init(struct device_node *node, · line 421
Quoted 10 lines
drivers/irqchip/irq-sifive-plic.c +8 -0
+             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);
Sorry, but I can't see any point to this patch.

Quoted 1 line

Quoted 1 line
Quoted 1 line
You didn't see the link I've put in the patch. In that opensbi patch:

No. If you can't explain why you need this in the commit message, why should I reverse engineer that from some obscure piece of firmware?

Quoted 21 lines

                intc: interrupt-controller@10000000 {
                        #interrupt-cells = <1>;

(quoted diff) +11 -1

-                       compatible = "riscv,plic0";
+                       compatible = "allwinner,sun20i-d1-plic",
+                                    "thead,c900-plic";
 
+#define THEAD_PLIC_CTRL_REG 0x1ffffc
+
+static void thead_plic_plat_init(struct plic_data *pd)
+{
+       writel_relaxed(BIT(0), (void *)pd->addr + THEAD_PLIC_CTRL_REG);
+}
+
 static const struct fdt_match irqchip_plic_match[] = {
        { .compatible = "riscv,plic0" },
        { .compatible = "sifive,plic-1.0.0" },
+       { .compatible = "thead,c900-plic",
+         .data = thead_plic_plat_init },
        { },
 };

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
We've changed the compatible name for thead,c900-plic, and there is no riscv,plic0 / sifive,plic-1.0.0 in dts. Without the patch, the newest opensbi + newest Linux would be broken in the Allwinner D1 dev board.

So the firmware changes things in incompatible ways. Why does it matter to Linux? Why isn't the fix directly applied to the firmware instead? Why isn't the riscv,plic0 fallback appropriate?

Quoted 7 lines

Quoted 5 lines
Yes, some patch is still necessary, because the hardware is indeed incompatible with riscv,plic0. However, this driver does not care about the difference. So all you need to do is hook up the existing code to the new compatible:
+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_init);
I think we should give clear info in /proc/interrupts. I hope we could keep thead_plic_init.

Why? There is no material difference at the driver level, and /proc/interrupts won't be the target of a branding exercise (which this series seems to be all about).

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Guo Ren

author

2022-01-30 13:09 UTC #8 lore raw

Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

Quoted 104 lines · Marc Zyngier

Quoted 101 lines · Guo Ren
Quoted 99 lines · Samuel Holland
Quoted 97 lines · Guo Ren
Quoted 57 lines · Marc Zyngier
Quoted 36 lines · On Sat, 29 Jan 2022 16:27:26 +0000, [email protected] wrote:
From: Guo Ren <[email protected]>
The thead,c900-plic has been used in opensbi to distinguish PLIC [1]. Although PLICs have the same behaviors in Linux, they are different hardware with some custom initializing in firmware(opensbi).
[1]: https://github.com/riscv-software-src/opensbi/commit/78c2b19218bd62653b9fb31623a42ced45f38ea6
Signed-off-by Guo Ren <[email protected]>
Cc Anup Patel <[email protected]>
Cc Marc Zyngier <[email protected]>
Cc Palmer Dabbelt <[email protected]>
Cc Samuel Holland <[email protected]>
Cc Thomas Gleixner <[email protected]>
---
 drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)
drivers/irqchip/irq-sifive-plic.c +1 -1
index 259065d271ef..245655928076 100644
@@ -172,7 +172,7 @@ static void plic_irq_eoi(struct irq_data *d)
      }
 }
 
-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,
@@ -182,12 +182,24 @@ static struct irq_chip plic_chip = {
 #endif
 };
drivers/irqchip/irq-sifive-plic.c static struct irq_chip plic_chip = { · line 193
Quoted 10 lines
drivers/irqchip/irq-sifive-plic.c +9 -0
 
+static struct irq_chip thead_plic_chip = {
+     .name           = "T-Head PLIC",
+     .irq_mask       = plic_irq_mask,
+     .irq_unmask     = plic_irq_unmask,
+     .irq_eoi        = plic_irq_eoi,
+#ifdef CONFIG_SMP
+     .irq_set_affinity = plic_set_affinity,
+#endif
+};
For pure entertainment, let's compare the two structures:
static struct irq_chip plic_chip = {
        .name           = "SiFive PLIC",
        .irq_mask       = plic_irq_mask,
        .irq_unmask     = plic_irq_unmask,
        .irq_eoi        = plic_irq_eoi,
Quoted 6 lines
#ifdef CONFIG_SMP
.irq_set_affinity = plic_set_affinity,
#endif
};
Oh wait: a string. Must be really important. Not.
No, pls see below comment.
Quoted 31 lines
Quoted 19 lines
drivers/irqchip/irq-sifive-plic.c +5 -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);
@@ -396,5 +408,14 @@ static int __init plic_init(struct device_node *node,
      return error;
 }
 
+static int __init thead_c900_plic_init(struct device_node *node,
drivers/irqchip/irq-sifive-plic.c static int __init plic_init(struct device_node *node, · line 421
Quoted 10 lines
drivers/irqchip/irq-sifive-plic.c +8 -0
+             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);
Sorry, but I can't see any point to this patch.

Quoted 4 lines

Quoted 1 line
Quoted 1 line
Quoted 1 line
You didn't see the link I've put in the patch. In that opensbi patch:
No. If you can't explain why you need this in the commit message, why should I reverse engineer that from some obscure piece of firmware?

I've explaned that in the commit message:

The thead,c900-plic has been used in opensbi to distinguish PLIC [1]. Although PLICs have the same behaviors in Linux, they are different hardware with some custom initializing in firmware(opensbi).

[1]: https://github.com/riscv-software-src/opensbi/commit/78c2b19218bd62653b9fb31623a42ced45f38ea6

If you think the above doesn't satisfy you, please point it out. Then I can improve the commit-msg.

Quoted 22 lines

                intc: interrupt-controller@10000000 {
                        #interrupt-cells = <1>;

(quoted diff) +11 -1

-                       compatible = "riscv,plic0";
+                       compatible = "allwinner,sun20i-d1-plic",
+                                    "thead,c900-plic";
 
+#define THEAD_PLIC_CTRL_REG 0x1ffffc
+
+static void thead_plic_plat_init(struct plic_data *pd)
+{
+       writel_relaxed(BIT(0), (void *)pd->addr + THEAD_PLIC_CTRL_REG);
+}
+
 static const struct fdt_match irqchip_plic_match[] = {
        { .compatible = "riscv,plic0" },
        { .compatible = "sifive,plic-1.0.0" },
+       { .compatible = "thead,c900-plic",
+         .data = thead_plic_plat_init },
        { },
 };

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
We've changed the compatible name for thead,c900-plic, and there is no riscv,plic0 / sifive,plic-1.0.0 in dts. Without the patch, the newest opensbi + newest Linux would be broken in the Allwinner D1 dev board.
So the firmware changes things in incompatible ways. Why does it matter to Linux?

The opensbi (firmware) reuses dts with the Linux, so PLIC driver separates into two parts: firmware & linux. For firmware part, we need another name to distinguish.

Quoted 1 line

Why isn't the fix directly applied to the firmware instead?

The opensbi patch commit-msg has explained:
The T-HEAD PLIC implementation requires setting a delegation bit to allow access from S-mode. Now that the T-HEAD PLIC has its own compatible string, set this bit automatically from the PLIC driver, instead of reaching into the PLIC's MMIO space from another driver.

If we can't modify the Linux driver, then we must abandon that patch in opensbi.

Quoted 1 line

Why isn't the riscv,plic0 fallback appropriate?

We can't use riscv,plic0 as the tag of thead,plic, because it's reserved for past.

Quoted 5 lines

Quoted 5 lines
Quoted 5 lines
Yes, some patch is still necessary, because the hardware is indeed incompatible with riscv,plic0. However, this driver does not care about the difference. So all you need to do is hook up the existing code to the new compatible:

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_init);
I think we should give clear info in /proc/interrupts. I hope we could keep thead_plic_init.
Why? There is no material difference at the driver level, and /proc/interrupts won't be the target of a branding exercise (which this series seems to be all about).

It's not a branding exercise, the series just correct the broken code & incorrect /proc/interrupts display.

Quoted 5 lines

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

Signature · 4 lines

Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

Marc Zyngier

2022-01-30 14:59 UTC #9 lore raw

Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

Quoted 7 lines · Guo Ren

Quoted 5 lines · Marc Zyngier
Quoted 2 lines · Guo Ren
Quoted 2 lines
+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_init);

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
I think we should give clear info in /proc/interrupts. I hope we could keep thead_plic_init.
Why? There is no material difference at the driver level, and /proc/interrupts won't be the target of a branding exercise (which this series seems to be all about).
It's not a branding exercise, the series just correct the broken code & incorrect /proc/interrupts display.

There is *nothing* incorrect in /proc/interrupt. If anything, changing the string *is* an ABI change, for no good reason.

M.

Signature · 1 line

Without deviation from the norm, progress is not possible.

Guo Ren

author

2022-01-30 15:11 UTC #10 lore raw

Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi

Quoted 13 lines · Marc Zyngier

Quoted 10 lines · Guo Ren
Quoted 8 lines · Marc Zyngier
Quoted 4 lines · Guo Ren
Quoted 2 lines
+IRQCHIP_DECLARE(thead_c900_plic, "thead,c900-plic", plic_init);
I think we should give clear info in /proc/interrupts. I hope we could keep thead_plic_init.

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Why? There is no material difference at the driver level, and /proc/interrupts won't be the target of a branding exercise (which this series seems to be all about).
It's not a branding exercise, the series just correct the broken code & incorrect /proc/interrupts display.
There is *nothing* incorrect in /proc/interrupt. If anything, changing the string *is* an ABI change, for no good reason.

I've minimized the modification to fix the problem, please have a look:

https://lore.kernel.org/linux-riscv/[email protected]/T/#u

Quoted 5 lines

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

--
Best Regards
Guo Ren

ML: https://lore.kernel.org/linux-csky/