* [PATCH V6 0/2] Add thead,c900-plic support
@ 2022-01-29 16:27 ` guoren
0 siblings, 0 replies; 22+ messages in thread
From: guoren @ 2022-01-29 16:27 UTC (permalink / raw)
To: guoren, anup, maz, tglx, palmer, samuel
Cc: linux-kernel, linux-riscv, devicetree, Guo Ren
From: Guo Ren <guoren@linux.alibaba.com>
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(-)
--
2.25.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH V6 0/2] Add thead,c900-plic support
@ 2022-01-29 16:27 ` guoren
0 siblings, 0 replies; 22+ messages in thread
From: guoren @ 2022-01-29 16:27 UTC (permalink / raw)
To: guoren, anup, maz, tglx, palmer, samuel
Cc: linux-kernel, linux-riscv, devicetree, Guo Ren
From: Guo Ren <guoren@linux.alibaba.com>
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(-)
--
2.25.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH V6 1/2] dt-bindings: update riscv plic compatible string
2022-01-29 16:27 ` guoren
@ 2022-01-29 16:27 ` guoren
-1 siblings, 0 replies; 22+ messages in thread
From: guoren @ 2022-01-29 16:27 UTC (permalink / raw)
To: guoren, anup, maz, tglx, palmer, samuel
Cc: linux-kernel, linux-riscv, devicetree, Guo Ren, Heiko Stuebner,
Rob Herring, Rob Herring, Palmer Dabbelt
From: Guo Ren <guoren@linux.alibaba.com>
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 <guoren@linux.alibaba.com>
Cc: Anup Patel <anup@brainfault.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Rob Herring <robh@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Palmer Dabbelt <palmerdabbelt@google.com>
Cc: Samuel Holland <samuel@sholland.org>
---
.../sifive,plic-1.0.0.yaml | 21 +++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
index 28b6b17fe4b2..1fa5aa7e4c2e 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
@@ -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 <sagar.kadam@sifive.com>
- Paul Walmsley <paul.walmsley@sifive.com>
@@ -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
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH V6 1/2] dt-bindings: update riscv plic compatible string
@ 2022-01-29 16:27 ` guoren
0 siblings, 0 replies; 22+ messages in thread
From: guoren @ 2022-01-29 16:27 UTC (permalink / raw)
To: guoren, anup, maz, tglx, palmer, samuel
Cc: linux-kernel, linux-riscv, devicetree, Guo Ren, Heiko Stuebner,
Rob Herring, Rob Herring, Palmer Dabbelt
From: Guo Ren <guoren@linux.alibaba.com>
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 <guoren@linux.alibaba.com>
Cc: Anup Patel <anup@brainfault.org>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: Rob Herring <robh@kernel.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Palmer Dabbelt <palmerdabbelt@google.com>
Cc: Samuel Holland <samuel@sholland.org>
---
.../sifive,plic-1.0.0.yaml | 21 +++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
index 28b6b17fe4b2..1fa5aa7e4c2e 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
+++ b/Documentation/devicetree/bindings/interrupt-controller/sifive,plic-1.0.0.yaml
@@ -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 <sagar.kadam@sifive.com>
- Paul Walmsley <paul.walmsley@sifive.com>
@@ -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
--
2.25.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
2022-01-29 16:27 ` guoren
@ 2022-01-29 16:27 ` guoren
-1 siblings, 0 replies; 22+ messages in thread
From: guoren @ 2022-01-29 16:27 UTC (permalink / raw)
To: guoren, anup, maz, tglx, palmer, samuel
Cc: linux-kernel, linux-riscv, devicetree, Guo Ren
From: Guo Ren <guoren@linux.alibaba.com>
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 <guoren@linux.alibaba.com>
Cc: Anup Patel <anup@brainfault.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Samuel Holland <samuel@sholland.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 259065d271ef..245655928076 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -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);
--
2.25.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic dt parse in opensbi
@ 2022-01-29 16:27 ` guoren
0 siblings, 0 replies; 22+ messages in thread
From: guoren @ 2022-01-29 16:27 UTC (permalink / raw)
To: guoren, anup, maz, tglx, palmer, samuel
Cc: linux-kernel, linux-riscv, devicetree, Guo Ren
From: Guo Ren <guoren@linux.alibaba.com>
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 <guoren@linux.alibaba.com>
Cc: Anup Patel <anup@brainfault.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Samuel Holland <samuel@sholland.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
index 259065d271ef..245655928076 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -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);
--
2.25.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
2022-01-29 16:27 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic " guoren
@ 2022-01-29 18:32 ` Marc Zyngier
-1 siblings, 0 replies; 22+ messages in thread
From: Marc Zyngier @ 2022-01-29 18:32 UTC (permalink / raw)
To: guoren
Cc: anup, tglx, palmer, samuel, linux-kernel, linux-riscv,
devicetree, Guo Ren
On Sat, 29 Jan 2022 16:27:26 +0000,
guoren@kernel.org wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> 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 <guoren@linux.alibaba.com>
> Cc: Anup Patel <anup@brainfault.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Samuel Holland <samuel@sholland.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
> drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 259065d271ef..245655928076 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -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
> +};
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.
> +
> +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);
Sorry, but I can't see any point to this patch.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic dt parse in opensbi
@ 2022-01-29 18:32 ` Marc Zyngier
0 siblings, 0 replies; 22+ messages in thread
From: Marc Zyngier @ 2022-01-29 18:32 UTC (permalink / raw)
To: guoren
Cc: anup, tglx, palmer, samuel, linux-kernel, linux-riscv,
devicetree, Guo Ren
On Sat, 29 Jan 2022 16:27:26 +0000,
guoren@kernel.org wrote:
>
> From: Guo Ren <guoren@linux.alibaba.com>
>
> 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 <guoren@linux.alibaba.com>
> Cc: Anup Patel <anup@brainfault.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Samuel Holland <samuel@sholland.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> ---
> drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> 1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> index 259065d271ef..245655928076 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -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
> +};
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.
> +
> +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);
Sorry, but I can't see any point to this patch.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
2022-01-29 18:32 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic " Marc Zyngier
@ 2022-01-30 2:08 ` Guo Ren
-1 siblings, 0 replies; 22+ messages in thread
From: Guo Ren @ 2022-01-30 2:08 UTC (permalink / raw)
To: Marc Zyngier
Cc: Anup Patel, Thomas Gleixner, Palmer Dabbelt, Samuel Holland,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sat, 29 Jan 2022 16:27:26 +0000,
> guoren@kernel.org wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > 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 <guoren@linux.alibaba.com>
> > Cc: Anup Patel <anup@brainfault.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Cc: Samuel Holland <samuel@sholland.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> > 1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 259065d271ef..245655928076 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -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
> > +};
>
> 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.
No, pls see below comment.
>
> > +
> > +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);
>
> 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.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
@ 2022-01-30 2:08 ` Guo Ren
0 siblings, 0 replies; 22+ messages in thread
From: Guo Ren @ 2022-01-30 2:08 UTC (permalink / raw)
To: Marc Zyngier
Cc: Anup Patel, Thomas Gleixner, Palmer Dabbelt, Samuel Holland,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sat, 29 Jan 2022 16:27:26 +0000,
> guoren@kernel.org wrote:
> >
> > From: Guo Ren <guoren@linux.alibaba.com>
> >
> > 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 <guoren@linux.alibaba.com>
> > Cc: Anup Patel <anup@brainfault.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > Cc: Samuel Holland <samuel@sholland.org>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> > 1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > index 259065d271ef..245655928076 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -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
> > +};
>
> 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.
No, pls see below comment.
>
> > +
> > +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);
>
> 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.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
2022-01-30 2:08 ` Guo Ren
@ 2022-01-30 2:50 ` Samuel Holland
-1 siblings, 0 replies; 22+ messages in thread
From: Samuel Holland @ 2022-01-30 2:50 UTC (permalink / raw)
To: Guo Ren
Cc: Marc Zyngier, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On 1/29/22 8:08 PM, Guo Ren wrote:
> On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Sat, 29 Jan 2022 16:27:26 +0000,
>> guoren@kernel.org wrote:
>>>
>>> From: Guo Ren <guoren@linux.alibaba.com>
>>>
>>> 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 <guoren@linux.alibaba.com>
>>> Cc: Anup Patel <anup@brainfault.org>
>>> Cc: Marc Zyngier <maz@kernel.org>
>>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>>> Cc: Samuel Holland <samuel@sholland.org>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> ---
>>> drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
>>> 1 file changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
>>> index 259065d271ef..245655928076 100644
>>> --- a/drivers/irqchip/irq-sifive-plic.c
>>> +++ b/drivers/irqchip/irq-sifive-plic.c
>>> @@ -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
>>> +};
>>
>> 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.
> No, pls see below comment.
>
>>
>>> +
>>> +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);
>>
>> 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.
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
@ 2022-01-30 2:50 ` Samuel Holland
0 siblings, 0 replies; 22+ messages in thread
From: Samuel Holland @ 2022-01-30 2:50 UTC (permalink / raw)
To: Guo Ren
Cc: Marc Zyngier, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On 1/29/22 8:08 PM, Guo Ren wrote:
> On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
>>
>> On Sat, 29 Jan 2022 16:27:26 +0000,
>> guoren@kernel.org wrote:
>>>
>>> From: Guo Ren <guoren@linux.alibaba.com>
>>>
>>> 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 <guoren@linux.alibaba.com>
>>> Cc: Anup Patel <anup@brainfault.org>
>>> Cc: Marc Zyngier <maz@kernel.org>
>>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
>>> Cc: Samuel Holland <samuel@sholland.org>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> ---
>>> drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
>>> 1 file changed, 23 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
>>> index 259065d271ef..245655928076 100644
>>> --- a/drivers/irqchip/irq-sifive-plic.c
>>> +++ b/drivers/irqchip/irq-sifive-plic.c
>>> @@ -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
>>> +};
>>
>> 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.
> No, pls see below comment.
>
>>
>>> +
>>> +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);
>>
>> 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.
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
2022-01-30 2:50 ` Samuel Holland
@ 2022-01-30 4:39 ` Guo Ren
-1 siblings, 0 replies; 22+ messages in thread
From: Guo Ren @ 2022-01-30 4:39 UTC (permalink / raw)
To: Samuel Holland
Cc: Marc Zyngier, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, Jan 30, 2022 at 10:50 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 1/29/22 8:08 PM, Guo Ren wrote:
> > On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> On Sat, 29 Jan 2022 16:27:26 +0000,
> >> guoren@kernel.org wrote:
> >>>
> >>> From: Guo Ren <guoren@linux.alibaba.com>
> >>>
> >>> 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 <guoren@linux.alibaba.com>
> >>> Cc: Anup Patel <anup@brainfault.org>
> >>> Cc: Marc Zyngier <maz@kernel.org>
> >>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> >>> Cc: Samuel Holland <samuel@sholland.org>
> >>> Cc: Thomas Gleixner <tglx@linutronix.de>
> >>> ---
> >>> drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> >>> 1 file changed, 23 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> >>> index 259065d271ef..245655928076 100644
> >>> --- a/drivers/irqchip/irq-sifive-plic.c
> >>> +++ b/drivers/irqchip/irq-sifive-plic.c
> >>> @@ -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
> >>> +};
> >>
> >> 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.
> > No, pls see below comment.
> >
> >>
> >>> +
> >>> +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);
> >>
> >> 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.
>
> 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.
>
> Regards,
> Samuel
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
@ 2022-01-30 4:39 ` Guo Ren
0 siblings, 0 replies; 22+ messages in thread
From: Guo Ren @ 2022-01-30 4:39 UTC (permalink / raw)
To: Samuel Holland
Cc: Marc Zyngier, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, Jan 30, 2022 at 10:50 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 1/29/22 8:08 PM, Guo Ren wrote:
> > On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
> >>
> >> On Sat, 29 Jan 2022 16:27:26 +0000,
> >> guoren@kernel.org wrote:
> >>>
> >>> From: Guo Ren <guoren@linux.alibaba.com>
> >>>
> >>> 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 <guoren@linux.alibaba.com>
> >>> Cc: Anup Patel <anup@brainfault.org>
> >>> Cc: Marc Zyngier <maz@kernel.org>
> >>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> >>> Cc: Samuel Holland <samuel@sholland.org>
> >>> Cc: Thomas Gleixner <tglx@linutronix.de>
> >>> ---
> >>> drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> >>> 1 file changed, 23 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> >>> index 259065d271ef..245655928076 100644
> >>> --- a/drivers/irqchip/irq-sifive-plic.c
> >>> +++ b/drivers/irqchip/irq-sifive-plic.c
> >>> @@ -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
> >>> +};
> >>
> >> 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.
> > No, pls see below comment.
> >
> >>
> >>> +
> >>> +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);
> >>
> >> 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.
>
> 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.
>
> Regards,
> Samuel
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
2022-01-30 4:39 ` Guo Ren
@ 2022-01-30 10:28 ` Marc Zyngier
-1 siblings, 0 replies; 22+ messages in thread
From: Marc Zyngier @ 2022-01-30 10:28 UTC (permalink / raw)
To: Guo Ren
Cc: Samuel Holland, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, 30 Jan 2022 04:39:34 +0000,
Guo Ren <guoren@kernel.org> wrote:
>
> On Sun, Jan 30, 2022 at 10:50 AM Samuel Holland <samuel@sholland.org> wrote:
> >
> > On 1/29/22 8:08 PM, Guo Ren wrote:
> > > On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
> > >>
> > >> On Sat, 29 Jan 2022 16:27:26 +0000,
> > >> guoren@kernel.org wrote:
> > >>>
> > >>> From: Guo Ren <guoren@linux.alibaba.com>
> > >>>
> > >>> 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 <guoren@linux.alibaba.com>
> > >>> Cc: Anup Patel <anup@brainfault.org>
> > >>> Cc: Marc Zyngier <maz@kernel.org>
> > >>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > >>> Cc: Samuel Holland <samuel@sholland.org>
> > >>> Cc: Thomas Gleixner <tglx@linutronix.de>
> > >>> ---
> > >>> drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> > >>> 1 file changed, 23 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > >>> index 259065d271ef..245655928076 100644
> > >>> --- a/drivers/irqchip/irq-sifive-plic.c
> > >>> +++ b/drivers/irqchip/irq-sifive-plic.c
> > >>> @@ -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
> > >>> +};
> > >>
> > >> 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.
> > > No, pls see below comment.
> > >
> > >>
> > >>> +
> > >>> +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);
> > >>
> > >> 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:
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?
> > > 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.
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?
> > 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.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic dt parse in opensbi
@ 2022-01-30 10:28 ` Marc Zyngier
0 siblings, 0 replies; 22+ messages in thread
From: Marc Zyngier @ 2022-01-30 10:28 UTC (permalink / raw)
To: Guo Ren
Cc: Samuel Holland, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, 30 Jan 2022 04:39:34 +0000,
Guo Ren <guoren@kernel.org> wrote:
>
> On Sun, Jan 30, 2022 at 10:50 AM Samuel Holland <samuel@sholland.org> wrote:
> >
> > On 1/29/22 8:08 PM, Guo Ren wrote:
> > > On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
> > >>
> > >> On Sat, 29 Jan 2022 16:27:26 +0000,
> > >> guoren@kernel.org wrote:
> > >>>
> > >>> From: Guo Ren <guoren@linux.alibaba.com>
> > >>>
> > >>> 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 <guoren@linux.alibaba.com>
> > >>> Cc: Anup Patel <anup@brainfault.org>
> > >>> Cc: Marc Zyngier <maz@kernel.org>
> > >>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > >>> Cc: Samuel Holland <samuel@sholland.org>
> > >>> Cc: Thomas Gleixner <tglx@linutronix.de>
> > >>> ---
> > >>> drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> > >>> 1 file changed, 23 insertions(+), 2 deletions(-)
> > >>>
> > >>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > >>> index 259065d271ef..245655928076 100644
> > >>> --- a/drivers/irqchip/irq-sifive-plic.c
> > >>> +++ b/drivers/irqchip/irq-sifive-plic.c
> > >>> @@ -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
> > >>> +};
> > >>
> > >> 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.
> > > No, pls see below comment.
> > >
> > >>
> > >>> +
> > >>> +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);
> > >>
> > >> 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:
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?
> > > 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.
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?
> > 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.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
2022-01-30 10:28 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic " Marc Zyngier
@ 2022-01-30 13:09 ` Guo Ren
-1 siblings, 0 replies; 22+ messages in thread
From: Guo Ren @ 2022-01-30 13:09 UTC (permalink / raw)
To: Marc Zyngier
Cc: Samuel Holland, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, Jan 30, 2022 at 6:28 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sun, 30 Jan 2022 04:39:34 +0000,
> Guo Ren <guoren@kernel.org> wrote:
> >
> > On Sun, Jan 30, 2022 at 10:50 AM Samuel Holland <samuel@sholland.org> wrote:
> > >
> > > On 1/29/22 8:08 PM, Guo Ren wrote:
> > > > On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >>
> > > >> On Sat, 29 Jan 2022 16:27:26 +0000,
> > > >> guoren@kernel.org wrote:
> > > >>>
> > > >>> From: Guo Ren <guoren@linux.alibaba.com>
> > > >>>
> > > >>> 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 <guoren@linux.alibaba.com>
> > > >>> Cc: Anup Patel <anup@brainfault.org>
> > > >>> Cc: Marc Zyngier <maz@kernel.org>
> > > >>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > >>> Cc: Samuel Holland <samuel@sholland.org>
> > > >>> Cc: Thomas Gleixner <tglx@linutronix.de>
> > > >>> ---
> > > >>> drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> > > >>> 1 file changed, 23 insertions(+), 2 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > >>> index 259065d271ef..245655928076 100644
> > > >>> --- a/drivers/irqchip/irq-sifive-plic.c
> > > >>> +++ b/drivers/irqchip/irq-sifive-plic.c
> > > >>> @@ -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
> > > >>> +};
> > > >>
> > > >> 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.
> > > > No, pls see below comment.
> > > >
> > > >>
> > > >>> +
> > > >>> +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);
> > > >>
> > > >> 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:
>
> 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.
>
> > > > 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.
>
> 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.
> 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.
> 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.
>
> > > 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).
It's not a branding exercise, the series just correct the broken code
& incorrect /proc/interrupts display.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
@ 2022-01-30 13:09 ` Guo Ren
0 siblings, 0 replies; 22+ messages in thread
From: Guo Ren @ 2022-01-30 13:09 UTC (permalink / raw)
To: Marc Zyngier
Cc: Samuel Holland, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, Jan 30, 2022 at 6:28 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sun, 30 Jan 2022 04:39:34 +0000,
> Guo Ren <guoren@kernel.org> wrote:
> >
> > On Sun, Jan 30, 2022 at 10:50 AM Samuel Holland <samuel@sholland.org> wrote:
> > >
> > > On 1/29/22 8:08 PM, Guo Ren wrote:
> > > > On Sun, Jan 30, 2022 at 2:32 AM Marc Zyngier <maz@kernel.org> wrote:
> > > >>
> > > >> On Sat, 29 Jan 2022 16:27:26 +0000,
> > > >> guoren@kernel.org wrote:
> > > >>>
> > > >>> From: Guo Ren <guoren@linux.alibaba.com>
> > > >>>
> > > >>> 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 <guoren@linux.alibaba.com>
> > > >>> Cc: Anup Patel <anup@brainfault.org>
> > > >>> Cc: Marc Zyngier <maz@kernel.org>
> > > >>> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> > > >>> Cc: Samuel Holland <samuel@sholland.org>
> > > >>> Cc: Thomas Gleixner <tglx@linutronix.de>
> > > >>> ---
> > > >>> drivers/irqchip/irq-sifive-plic.c | 25 +++++++++++++++++++++++--
> > > >>> 1 file changed, 23 insertions(+), 2 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/irqchip/irq-sifive-plic.c b/drivers/irqchip/irq-sifive-plic.c
> > > >>> index 259065d271ef..245655928076 100644
> > > >>> --- a/drivers/irqchip/irq-sifive-plic.c
> > > >>> +++ b/drivers/irqchip/irq-sifive-plic.c
> > > >>> @@ -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
> > > >>> +};
> > > >>
> > > >> 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.
> > > > No, pls see below comment.
> > > >
> > > >>
> > > >>> +
> > > >>> +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);
> > > >>
> > > >> 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:
>
> 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.
>
> > > > 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.
>
> 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.
> 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.
> 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.
>
> > > 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).
It's not a branding exercise, the series just correct the broken code
& incorrect /proc/interrupts display.
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
2022-01-30 13:09 ` Guo Ren
@ 2022-01-30 14:59 ` Marc Zyngier
-1 siblings, 0 replies; 22+ messages in thread
From: Marc Zyngier @ 2022-01-30 14:59 UTC (permalink / raw)
To: Guo Ren
Cc: Samuel Holland, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, 30 Jan 2022 13:09:21 +0000,
Guo Ren <guoren@kernel.org> wrote:
>
> On Sun, Jan 30, 2022 at 6:28 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Sun, 30 Jan 2022 04:39:34 +0000,
> > Guo Ren <guoren@kernel.org> wrote:
> > >
> > > > +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.
There is *nothing* incorrect in /proc/interrupt. If anything, changing
the string *is* an ABI change, for no good reason.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic dt parse in opensbi
@ 2022-01-30 14:59 ` Marc Zyngier
0 siblings, 0 replies; 22+ messages in thread
From: Marc Zyngier @ 2022-01-30 14:59 UTC (permalink / raw)
To: Guo Ren
Cc: Samuel Holland, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, 30 Jan 2022 13:09:21 +0000,
Guo Ren <guoren@kernel.org> wrote:
>
> On Sun, Jan 30, 2022 at 6:28 PM Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Sun, 30 Jan 2022 04:39:34 +0000,
> > Guo Ren <guoren@kernel.org> wrote:
> > >
> > > > +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.
There is *nothing* incorrect in /proc/interrupt. If anything, changing
the string *is* an ABI change, for no good reason.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
2022-01-30 14:59 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic " Marc Zyngier
@ 2022-01-30 15:11 ` Guo Ren
-1 siblings, 0 replies; 22+ messages in thread
From: Guo Ren @ 2022-01-30 15:11 UTC (permalink / raw)
To: Marc Zyngier
Cc: Samuel Holland, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, Jan 30, 2022 at 10:59 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sun, 30 Jan 2022 13:09:21 +0000,
> Guo Ren <guoren@kernel.org> wrote:
> >
> > On Sun, Jan 30, 2022 at 6:28 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Sun, 30 Jan 2022 04:39:34 +0000,
> > > Guo Ren <guoren@kernel.org> wrote:
> > > >
> > > > > +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.
>
> 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/20220130135634.1213301-3-guoren@kernel.org/T/#u
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi
@ 2022-01-30 15:11 ` Guo Ren
0 siblings, 0 replies; 22+ messages in thread
From: Guo Ren @ 2022-01-30 15:11 UTC (permalink / raw)
To: Marc Zyngier
Cc: Samuel Holland, Anup Patel, Thomas Gleixner, Palmer Dabbelt,
Linux Kernel Mailing List, linux-riscv, devicetree, Guo Ren
On Sun, Jan 30, 2022 at 10:59 PM Marc Zyngier <maz@kernel.org> wrote:
>
> On Sun, 30 Jan 2022 13:09:21 +0000,
> Guo Ren <guoren@kernel.org> wrote:
> >
> > On Sun, Jan 30, 2022 at 6:28 PM Marc Zyngier <maz@kernel.org> wrote:
> > >
> > > On Sun, 30 Jan 2022 04:39:34 +0000,
> > > Guo Ren <guoren@kernel.org> wrote:
> > > >
> > > > > +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.
>
> 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/20220130135634.1213301-3-guoren@kernel.org/T/#u
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-01-30 15:12 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-29 16:27 [PATCH V6 0/2] Add thead,c900-plic support guoren
2022-01-29 16:27 ` guoren
2022-01-29 16:27 ` [PATCH V6 1/2] dt-bindings: update riscv plic compatible string guoren
2022-01-29 16:27 ` guoren
2022-01-29 16:27 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic dt parse in opensbi guoren
2022-01-29 16:27 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic " guoren
2022-01-29 18:32 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic " Marc Zyngier
2022-01-29 18:32 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic " Marc Zyngier
2022-01-30 2:08 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic " Guo Ren
2022-01-30 2:08 ` Guo Ren
2022-01-30 2:50 ` Samuel Holland
2022-01-30 2:50 ` Samuel Holland
2022-01-30 4:39 ` Guo Ren
2022-01-30 4:39 ` Guo Ren
2022-01-30 10:28 ` Marc Zyngier
2022-01-30 10:28 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic " Marc Zyngier
2022-01-30 13:09 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic " Guo Ren
2022-01-30 13:09 ` Guo Ren
2022-01-30 14:59 ` Marc Zyngier
2022-01-30 14:59 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead, c900-plic " Marc Zyngier
2022-01-30 15:11 ` [PATCH V6 2/2] irqchip/sifive-plic: Fixup thead,c900-plic " Guo Ren
2022-01-30 15:11 ` Guo Ren
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.