LinuxLists.cc - [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

[permalink] [raw]

Subject: [PATCH V3 2/2] irqchip/sifive-plic: Add thead,c900-plic support

From: Guo Ren <[email protected]>

thead,c900-plic would mask IRQ with readl(claim), so it needn't
mask/unmask which needed in RISC-V PLIC.

When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
operation would cause a blocking irq bug in thead,c900-plic. Because
when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.

Signed-off-by: Guo Ren <[email protected]>
Cc: Anup Patel <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Palmer Dabbelt <[email protected]>
Cc: Atish Patra <[email protected]>

---

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

Changes since V2:
- Add a separate compatible string "thead,c9xx-plic"
- set irq_mask/unmask of "plic_chip" to NULL and point
irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
- Add a detailed comment block in plic_init() about the
differences in Claim/Completion process of RISC-V PLIC and C9xx
PLIC.
---
drivers/irqchip/irq-sifive-plic.c | 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 cf74cfa82045..5b806d823df7 100644
--- a/drivers/irqchip/irq-sifive-plic.c
+++ b/drivers/irqchip/irq-sifive-plic.c
@@ -166,7 +166,7 @@ static void plic_irq_eoi(struct irq_data *d)
writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
}

-static struct irq_chip plic_chip = {
+static struct irq_chip sifive_plic_chip = {
.name = "SiFive PLIC",
.irq_mask = plic_irq_mask,
.irq_unmask = plic_irq_unmask,
@@ -176,12 +176,24 @@ static struct irq_chip plic_chip = {
#endif
};

+static struct irq_chip thead_plic_chip = {
+ .name = "T-Head PLIC",
+ .irq_disable = plic_irq_mask,
+ .irq_enable = plic_irq_unmask,
+ .irq_eoi = plic_irq_eoi,
+#ifdef CONFIG_SMP
+ .irq_set_affinity = plic_set_affinity,
+#endif
+};
+
+static struct irq_chip *def_plic_chip = &sifive_plic_chip;
+
static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hwirq)
{
struct plic_priv *priv = d->host_data;

- irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
+ irq_domain_set_info(d, irq, hwirq, def_plic_chip, d->host_data,
handle_fasteoi_irq, NULL, NULL);
irq_set_noprobe(irq);
irq_set_affinity(irq, &priv->lmask);
@@ -390,5 +402,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

2021-10-13 05:13:26

[permalink] [raw]

Subject: Re: [PATCH V3 2/2] irqchip/sifive-plic: Add thead,c900-plic support

On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
>
> From: Guo Ren <[email protected]>
>
> thead,c900-plic would mask IRQ with readl(claim), so it needn't
> mask/unmask which needed in RISC-V PLIC.
>
> When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> operation would cause a blocking irq bug in thead,c900-plic. Because
> when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.

This is a totally incorrect description.

Instead of this, the commit description should say the following:

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

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

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

In general, non-compliance of the C9xx PLIC should be treated as
an errata so please don't project it as a feature.

>
> Signed-off-by: Guo Ren <[email protected]>
> Cc: Anup Patel <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Atish Patra <[email protected]>
>
> ---
>
> Changes since V3:
> - Rename "c9xx" to "c900"
> - Add sifive_plic_chip and thead_plic_chip for difference
>
> Changes since V2:
> - Add a separate compatible string "thead,c9xx-plic"
> - set irq_mask/unmask of "plic_chip" to NULL and point
> irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
> - Add a detailed comment block in plic_init() about the
> differences in Claim/Completion process of RISC-V PLIC and C9xx
> PLIC.
> ---
> drivers/irqchip/irq-sifive-plic.c | 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 cf74cfa82045..5b806d823df7 100644
> --- a/drivers/irqchip/irq-sifive-plic.c
> +++ b/drivers/irqchip/irq-sifive-plic.c
> @@ -166,7 +166,7 @@ static void plic_irq_eoi(struct irq_data *d)
> writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> }
>
> -static struct irq_chip plic_chip = {
> +static struct irq_chip sifive_plic_chip = {
> .name = "SiFive PLIC",
> .irq_mask = plic_irq_mask,
> .irq_unmask = plic_irq_unmask,
> @@ -176,12 +176,24 @@ static struct irq_chip plic_chip = {
> #endif
> };
>

Please add a detailed comment block as described by point#2
above.

> +static struct irq_chip thead_plic_chip = {
> + .name = "T-Head PLIC",
> + .irq_disable = plic_irq_mask,
> + .irq_enable = plic_irq_unmask,
> + .irq_eoi = plic_irq_eoi,
> +#ifdef CONFIG_SMP
> + .irq_set_affinity = plic_set_affinity,
> +#endif
> +};
> +
> +static struct irq_chip *def_plic_chip = &sifive_plic_chip;
> +
> static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> irq_hw_number_t hwirq)
> {
> struct plic_priv *priv = d->host_data;
>
> - irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> + irq_domain_set_info(d, irq, hwirq, def_plic_chip, d->host_data,
> handle_fasteoi_irq, NULL, NULL);
> irq_set_noprobe(irq);
> irq_set_affinity(irq, &priv->lmask);
> @@ -390,5 +402,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
>

Regards,
Anup

2021-10-13 05:13:46

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
>
> From: Guo Ren <[email protected]>
>
> Add the compatible string "thead,c900-plic" to the riscv plic
> bindings to support SOCs with thead,c9xx processor cores.
>
> Signed-off-by: Guo Ren <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Anup Patel <[email protected]>
> Cc: Atish Patra <[email protected]>
>
> ---
>
> Changes since V3:
> - Rename "c9xx" to "c900"
> - Add thead,c900-plic in the description section
> ---
> .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
>
> + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.

This is a totally incorrect description of the errata required for C9xx PLIC.

Please don't project non-compliance as a feature of C9xx PLIC.

> +
> maintainers:
> - Sagar Kadam <[email protected]>
> - Paul Walmsley <[email protected]>
> @@ -46,6 +51,7 @@ properties:
> - enum:
> - sifive,fu540-c000-plic
> - canaan,k210-plic
> + - thead,c900-plic
> - const: sifive,plic-1.0.0

The PLIC DT node requires two compatible string:
<implementation_compat>, <spec_compat>

The C9xx PLIC is not RISC-V PLIC so, the DT node should
be: "thead,c900-plic", "thead,c9xx-plic"

You need to change "- const: sifive,plic-1.0.0" to
- enum:
- sifive,plic-1.0.0
- thead,c9xx-plic

>
> reg:
> --
> 2.25.1
>

Regards,
Anup

2021-10-13 08:41:30

[permalink] [raw]

Subject: Re: [PATCH V3 2/2] irqchip/sifive-plic: Add thead,c900-plic support

On Wed, Oct 13, 2021 at 1:04 PM Anup Patel <[email protected]> wrote:
>
> On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> >
> > From: Guo Ren <[email protected]>
> >
> > thead,c900-plic would mask IRQ with readl(claim), so it needn't
> > mask/unmask which needed in RISC-V PLIC.
> >
> > When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED path, unnecessary mask
> > operation would cause a blocking irq bug in thead,c900-plic. Because
> > when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
>
> This is a totally incorrect description.
>
> Instead of this, the commit description should say the following:
>
> 1) The irq_mask/unmask() is used by handle_fasteoi_irq() is mostly
> for ONESHOT irqs and there is no limitation in the RISC-V PLIC driver
> due to use of irq_mask/unmask() callbacks. In fact, a lot of irqchip
> drivers using handle_fasteoi_irq() also implement irq_mask/unmask().
Add irq_mask/unmask in handle_fasteoi_irq() would decrease the
performance, we also need to add this fact in the description.
>
> 2) The C9xx PLIC does not comply with the interrupt claim/completion
> process defined by the RISC-V PLIC specification because C9xx PLIC
> will mask an IRQ when it is claimed by PLIC driver (i.e. readl(claim) and
> the IRQ will be unmasked upon completion by PLIC driver (i.e. writel(claim).
> This behaviour breaks the handling of IRQS_ONESHOT by the generic
> handle_fasteoi_irq() used in the PLIC driver.
>
> 3) This patch adds an errata fix for IRQS_ONESHOT handling on
> C9xx PLIC by using irq_enable/disable() callbacks instead of
> irq_mask/unmask().
>
> In general, non-compliance of the C9xx PLIC should be treated as
> an errata so please don't project it as a feature.
>
> >
> > Signed-off-by: Guo Ren <[email protected]>
> > Cc: Anup Patel <[email protected]>
> > Cc: Thomas Gleixner <[email protected]>
> > Cc: Marc Zyngier <[email protected]>
> > Cc: Palmer Dabbelt <[email protected]>
> > Cc: Atish Patra <[email protected]>
> >
> > ---
> >
> > Changes since V3:
> > - Rename "c9xx" to "c900"
> > - Add sifive_plic_chip and thead_plic_chip for difference
> >
> > Changes since V2:
> > - Add a separate compatible string "thead,c9xx-plic"
> > - set irq_mask/unmask of "plic_chip" to NULL and point
> > irq_enable/disable of "plic_chip" to plic_irq_mask/unmask
> > - Add a detailed comment block in plic_init() about the
> > differences in Claim/Completion process of RISC-V PLIC and C9xx
> > PLIC.
> > ---
> > drivers/irqchip/irq-sifive-plic.c | 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 cf74cfa82045..5b806d823df7 100644
> > --- a/drivers/irqchip/irq-sifive-plic.c
> > +++ b/drivers/irqchip/irq-sifive-plic.c
> > @@ -166,7 +166,7 @@ static void plic_irq_eoi(struct irq_data *d)
> > writel(d->hwirq, handler->hart_base + CONTEXT_CLAIM);
> > }
> >
> > -static struct irq_chip plic_chip = {
> > +static struct irq_chip sifive_plic_chip = {
> > .name = "SiFive PLIC",
> > .irq_mask = plic_irq_mask,
> > .irq_unmask = plic_irq_unmask,
> > @@ -176,12 +176,24 @@ static struct irq_chip plic_chip = {
> > #endif
> > };
> >
>
> Please add a detailed comment block as described by point#2
> above.
>
> > +static struct irq_chip thead_plic_chip = {
> > + .name = "T-Head PLIC",
> > + .irq_disable = plic_irq_mask,
> > + .irq_enable = plic_irq_unmask,
> > + .irq_eoi = plic_irq_eoi,
> > +#ifdef CONFIG_SMP
> > + .irq_set_affinity = plic_set_affinity,
> > +#endif
> > +};
> > +
> > +static struct irq_chip *def_plic_chip = &sifive_plic_chip;
> > +
> > static int plic_irqdomain_map(struct irq_domain *d, unsigned int irq,
> > irq_hw_number_t hwirq)
> > {
> > struct plic_priv *priv = d->host_data;
> >
> > - irq_domain_set_info(d, irq, hwirq, &plic_chip, d->host_data,
> > + irq_domain_set_info(d, irq, hwirq, def_plic_chip, d->host_data,
> > handle_fasteoi_irq, NULL, NULL);
> > irq_set_noprobe(irq);
> > irq_set_affinity(irq, &priv->lmask);
> > @@ -390,5 +402,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
> >
>
> Regards,
> Anup

--
Best Regards
Guo Ren

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

2021-10-13 08:59:58

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

Hi Anup,

Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> >
> > From: Guo Ren <[email protected]>
> >
> > Add the compatible string "thead,c900-plic" to the riscv plic
> > bindings to support SOCs with thead,c9xx processor cores.
> >
> > Signed-off-by: Guo Ren <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Palmer Dabbelt <[email protected]>
> > Cc: Anup Patel <[email protected]>
> > Cc: Atish Patra <[email protected]>
> >
> > ---
> >
> > Changes since V3:
> > - Rename "c9xx" to "c900"
> > - Add thead,c900-plic in the description section
> > ---
> > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> >
> > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
>
> This is a totally incorrect description of the errata required for C9xx PLIC.
>
> Please don't project non-compliance as a feature of C9xx PLIC.
>
> > +
> > maintainers:
> > - Sagar Kadam <[email protected]>
> > - Paul Walmsley <[email protected]>
> > @@ -46,6 +51,7 @@ properties:
> > - enum:
> > - sifive,fu540-c000-plic
> > - canaan,k210-plic
> > + - thead,c900-plic

we still want specific SoC names in the compatible, the "c900"
is still a sort-of placeholder.

> > - const: sifive,plic-1.0.0
>
> The PLIC DT node requires two compatible string:
> <implementation_compat>, <spec_compat>
>
> The C9xx PLIC is not RISC-V PLIC so, the DT node should
> be: "thead,c900-plic", "thead,c9xx-plic"
>
> You need to change "- const: sifive,plic-1.0.0" to
> - enum:
> - sifive,plic-1.0.0
> - thead,c9xx-plic

2021-10-13 09:13:10

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

On Wed, Oct 13, 2021 at 2:27 PM Heiko Stübner <[email protected]> wrote:
>
> Hi Anup,
>
> Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> > >
> > > From: Guo Ren <[email protected]>
> > >
> > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > bindings to support SOCs with thead,c9xx processor cores.
> > >
> > > Signed-off-by: Guo Ren <[email protected]>
> > > Cc: Rob Herring <[email protected]>
> > > Cc: Palmer Dabbelt <[email protected]>
> > > Cc: Anup Patel <[email protected]>
> > > Cc: Atish Patra <[email protected]>
> > >
> > > ---
> > >
> > > Changes since V3:
> > > - Rename "c9xx" to "c900"
> > > - Add thead,c900-plic in the description section
> > > ---
> > > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> > >
> > > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
> >
> > This is a totally incorrect description of the errata required for C9xx PLIC.
> >
> > Please don't project non-compliance as a feature of C9xx PLIC.
> >
> > > +
> > > maintainers:
> > > - Sagar Kadam <[email protected]>
> > > - Paul Walmsley <[email protected]>
> > > @@ -46,6 +51,7 @@ properties:
> > > - enum:
> > > - sifive,fu540-c000-plic
> > > - canaan,k210-plic
> > > + - thead,c900-plic
>
> we still want specific SoC names in the compatible, the "c900"
> is still a sort-of placeholder.

Yes, we need "c900" compatible string as well. The "c9xx"
compatible string is for the custom PLIC spec followed by T-HEAD.

>
>
> > > - const: sifive,plic-1.0.0
> >
> > The PLIC DT node requires two compatible string:
> > <implementation_compat>, <spec_compat>
> >
> > The C9xx PLIC is not RISC-V PLIC so, the DT node should
> > be: "thead,c900-plic", "thead,c9xx-plic"
> >
> > You need to change "- const: sifive,plic-1.0.0" to
> > - enum:
> > - sifive,plic-1.0.0
> > - thead,c9xx-plic
>
>

Regards,
Anup

2021-10-13 09:17:10

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

Am Mittwoch, 13. Oktober 2021, 11:11:26 CEST schrieb Anup Patel:
> On Wed, Oct 13, 2021 at 2:27 PM Heiko St?bner <[email protected]> wrote:
> >
> > Hi Anup,
> >
> > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> > > >
> > > > From: Guo Ren <[email protected]>
> > > >
> > > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > > bindings to support SOCs with thead,c9xx processor cores.
> > > >
> > > > Signed-off-by: Guo Ren <[email protected]>
> > > > Cc: Rob Herring <[email protected]>
> > > > Cc: Palmer Dabbelt <[email protected]>
> > > > Cc: Anup Patel <[email protected]>
> > > > Cc: Atish Patra <[email protected]>
> > > >
> > > > ---
> > > >
> > > > Changes since V3:
> > > > - Rename "c9xx" to "c900"
> > > > - Add thead,c900-plic in the description section
> > > > ---
> > > > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> > > >
> > > > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > > > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > > > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > > > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
> > >
> > > This is a totally incorrect description of the errata required for C9xx PLIC.
> > >
> > > Please don't project non-compliance as a feature of C9xx PLIC.
> > >
> > > > +
> > > > maintainers:
> > > > - Sagar Kadam <[email protected]>
> > > > - Paul Walmsley <[email protected]>
> > > > @@ -46,6 +51,7 @@ properties:
> > > > - enum:
> > > > - sifive,fu540-c000-plic
> > > > - canaan,k210-plic
> > > > + - thead,c900-plic
> >
> > we still want specific SoC names in the compatible, the "c900"
> > is still a sort-of placeholder.
>
> Yes, we need "c900" compatible string as well. The "c9xx"
> compatible string is for the custom PLIC spec followed by T-HEAD.

What I meant was that the soc-specific string should name the
actual SoC (c906, c910) and not some imaginary chip ;-)

See for example mali gpu bindings for a similar reference
in devicetree/bindings/gpu/arm,mali-bifrost.yaml .

>
> >
> >
> > > > - const: sifive,plic-1.0.0
> > >
> > > The PLIC DT node requires two compatible string:
> > > <implementation_compat>, <spec_compat>
> > >
> > > The C9xx PLIC is not RISC-V PLIC so, the DT node should
> > > be: "thead,c900-plic", "thead,c9xx-plic"
> > >
> > > You need to change "- const: sifive,plic-1.0.0" to
> > > - enum:
> > > - sifive,plic-1.0.0
> > > - thead,c9xx-plic
> >
> >
>
> Regards,
> Anup
>

2021-10-13 09:22:08

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

On Wed, Oct 13, 2021 at 2:44 PM Heiko Stübner <[email protected]> wrote:
>
> Am Mittwoch, 13. Oktober 2021, 11:11:26 CEST schrieb Anup Patel:
> > On Wed, Oct 13, 2021 at 2:27 PM Heiko Stübner <[email protected]> wrote:
> > >
> > > Hi Anup,
> > >
> > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> > > > >
> > > > > From: Guo Ren <[email protected]>
> > > > >
> > > > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > > > bindings to support SOCs with thead,c9xx processor cores.
> > > > >
> > > > > Signed-off-by: Guo Ren <[email protected]>
> > > > > Cc: Rob Herring <[email protected]>
> > > > > Cc: Palmer Dabbelt <[email protected]>
> > > > > Cc: Anup Patel <[email protected]>
> > > > > Cc: Atish Patra <[email protected]>
> > > > >
> > > > > ---
> > > > >
> > > > > Changes since V3:
> > > > > - Rename "c9xx" to "c900"
> > > > > - Add thead,c900-plic in the description section
> > > > > ---
> > > > > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > > > > 1 file changed, 6 insertions(+)
> > > > >
> > > > > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> > > > >
> > > > > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > > > > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > > > > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > > > > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
> > > >
> > > > This is a totally incorrect description of the errata required for C9xx PLIC.
> > > >
> > > > Please don't project non-compliance as a feature of C9xx PLIC.
> > > >
> > > > > +
> > > > > maintainers:
> > > > > - Sagar Kadam <[email protected]>
> > > > > - Paul Walmsley <[email protected]>
> > > > > @@ -46,6 +51,7 @@ properties:
> > > > > - enum:
> > > > > - sifive,fu540-c000-plic
> > > > > - canaan,k210-plic
> > > > > + - thead,c900-plic
> > >
> > > we still want specific SoC names in the compatible, the "c900"
> > > is still a sort-of placeholder.
> >
> > Yes, we need "c900" compatible string as well. The "c9xx"
> > compatible string is for the custom PLIC spec followed by T-HEAD.
>
> What I meant was that the soc-specific string should name the
> actual SoC (c906, c910) and not some imaginary chip ;-)

Ahh, yes. It should be an actual soc name in the compatible
string.

For example, SiFive uses "fu540" string to identify some of the
devices on both SiFive unleashed and SiFive unmatched boards.

I was under the impression that "c900" is an actual SoC name.

Regards,
Anup

>
> See for example mali gpu bindings for a similar reference
> in devicetree/bindings/gpu/arm,mali-bifrost.yaml .
>
>
>
>
> >
> > >
> > >
> > > > > - const: sifive,plic-1.0.0
> > > >
> > > > The PLIC DT node requires two compatible string:
> > > > <implementation_compat>, <spec_compat>
> > > >
> > > > The C9xx PLIC is not RISC-V PLIC so, the DT node should
> > > > be: "thead,c900-plic", "thead,c9xx-plic"
> > > >
> > > > You need to change "- const: sifive,plic-1.0.0" to
> > > > - enum:
> > > > - sifive,plic-1.0.0
> > > > - thead,c9xx-plic
> > >
> > >
> >
> > Regards,
> > Anup
> >
>
>
>
>

2021-10-13 09:45:15

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

Am Mittwoch, 13. Oktober 2021, 11:19:53 CEST schrieb Anup Patel:
> On Wed, Oct 13, 2021 at 2:44 PM Heiko St?bner <[email protected]> wrote:
> >
> > Am Mittwoch, 13. Oktober 2021, 11:11:26 CEST schrieb Anup Patel:
> > > On Wed, Oct 13, 2021 at 2:27 PM Heiko St?bner <[email protected]> wrote:
> > > >
> > > > Hi Anup,
> > > >
> > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> > > > > >
> > > > > > From: Guo Ren <[email protected]>
> > > > > >
> > > > > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > > > > bindings to support SOCs with thead,c9xx processor cores.
> > > > > >
> > > > > > Signed-off-by: Guo Ren <[email protected]>
> > > > > > Cc: Rob Herring <[email protected]>
> > > > > > Cc: Palmer Dabbelt <[email protected]>
> > > > > > Cc: Anup Patel <[email protected]>
> > > > > > Cc: Atish Patra <[email protected]>
> > > > > >
> > > > > > ---
> > > > > >
> > > > > > Changes since V3:
> > > > > > - Rename "c9xx" to "c900"
> > > > > > - Add thead,c900-plic in the description section
> > > > > > ---
> > > > > > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > > > > > 1 file changed, 6 insertions(+)
> > > > > >
> > > > > > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> > > > > >
> > > > > > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > > > > > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > > > > > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > > > > > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
> > > > >
> > > > > This is a totally incorrect description of the errata required for C9xx PLIC.
> > > > >
> > > > > Please don't project non-compliance as a feature of C9xx PLIC.
> > > > >
> > > > > > +
> > > > > > maintainers:
> > > > > > - Sagar Kadam <[email protected]>
> > > > > > - Paul Walmsley <[email protected]>
> > > > > > @@ -46,6 +51,7 @@ properties:
> > > > > > - enum:
> > > > > > - sifive,fu540-c000-plic
> > > > > > - canaan,k210-plic
> > > > > > + - thead,c900-plic
> > > >
> > > > we still want specific SoC names in the compatible, the "c900"
> > > > is still a sort-of placeholder.
> > >
> > > Yes, we need "c900" compatible string as well. The "c9xx"
> > > compatible string is for the custom PLIC spec followed by T-HEAD.
> >
> > What I meant was that the soc-specific string should name the
> > actual SoC (c906, c910) and not some imaginary chip ;-)
>
> Ahh, yes. It should be an actual soc name in the compatible
> string.
>
> For example, SiFive uses "fu540" string to identify some of the
> devices on both SiFive unleashed and SiFive unmatched boards.
>
> I was under the impression that "c900" is an actual SoC name.
>
> Regards,
> Anup
>
> >
> > See for example mali gpu bindings for a similar reference
> > in devicetree/bindings/gpu/arm,mali-bifrost.yaml .
> >
> >
> >
> >
> > >
> > > >
> > > >
> > > > > > - const: sifive,plic-1.0.0
> > > > >
> > > > > The PLIC DT node requires two compatible string:
> > > > > <implementation_compat>, <spec_compat>
> > > > >
> > > > > The C9xx PLIC is not RISC-V PLIC so, the DT node should
> > > > > be: "thead,c900-plic", "thead,c9xx-plic"
> > > > >
> > > > > You need to change "- const: sifive,plic-1.0.0" to
> > > > > - enum:
> > > > > - sifive,plic-1.0.0
> > > > > - thead,c9xx-plic

isn't XuanTie the series containing the c906 and c910?
So maybe
thead,xuantie-plic
for the spec compatible.

So doing in full
compatible = "thead,c906-plic", "thead,xuantie-plic"

Heiko

2021-10-13 09:53:13

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

On Wed, Oct 13, 2021 at 3:13 PM Heiko Stübner <[email protected]> wrote:
>
> Am Mittwoch, 13. Oktober 2021, 11:19:53 CEST schrieb Anup Patel:
> > On Wed, Oct 13, 2021 at 2:44 PM Heiko Stübner <[email protected]> wrote:
> > >
> > > Am Mittwoch, 13. Oktober 2021, 11:11:26 CEST schrieb Anup Patel:
> > > > On Wed, Oct 13, 2021 at 2:27 PM Heiko Stübner <[email protected]> wrote:
> > > > >
> > > > > Hi Anup,
> > > > >
> > > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > > On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> > > > > > >
> > > > > > > From: Guo Ren <[email protected]>
> > > > > > >
> > > > > > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > > > > > bindings to support SOCs with thead,c9xx processor cores.
> > > > > > >
> > > > > > > Signed-off-by: Guo Ren <[email protected]>
> > > > > > > Cc: Rob Herring <[email protected]>
> > > > > > > Cc: Palmer Dabbelt <[email protected]>
> > > > > > > Cc: Anup Patel <[email protected]>
> > > > > > > Cc: Atish Patra <[email protected]>
> > > > > > >
> > > > > > > ---
> > > > > > >
> > > > > > > Changes since V3:
> > > > > > > - Rename "c9xx" to "c900"
> > > > > > > - Add thead,c900-plic in the description section
> > > > > > > ---
> > > > > > > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > > > > > > 1 file changed, 6 insertions(+)
> > > > > > >
> > > > > > > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> > > > > > >
> > > > > > > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > > > > > > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > > > > > > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > > > > > > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
> > > > > >
> > > > > > This is a totally incorrect description of the errata required for C9xx PLIC.
> > > > > >
> > > > > > Please don't project non-compliance as a feature of C9xx PLIC.
> > > > > >
> > > > > > > +
> > > > > > > maintainers:
> > > > > > > - Sagar Kadam <[email protected]>
> > > > > > > - Paul Walmsley <[email protected]>
> > > > > > > @@ -46,6 +51,7 @@ properties:
> > > > > > > - enum:
> > > > > > > - sifive,fu540-c000-plic
> > > > > > > - canaan,k210-plic
> > > > > > > + - thead,c900-plic
> > > > >
> > > > > we still want specific SoC names in the compatible, the "c900"
> > > > > is still a sort-of placeholder.
> > > >
> > > > Yes, we need "c900" compatible string as well. The "c9xx"
> > > > compatible string is for the custom PLIC spec followed by T-HEAD.
> > >
> > > What I meant was that the soc-specific string should name the
> > > actual SoC (c906, c910) and not some imaginary chip ;-)
> >
> > Ahh, yes. It should be an actual soc name in the compatible
> > string.
> >
> > For example, SiFive uses "fu540" string to identify some of the
> > devices on both SiFive unleashed and SiFive unmatched boards.
> >
> > I was under the impression that "c900" is an actual SoC name.
> >
> > Regards,
> > Anup
> >
> > >
> > > See for example mali gpu bindings for a similar reference
> > > in devicetree/bindings/gpu/arm,mali-bifrost.yaml .
> > >
> > >
> > >
> > >
> > > >
> > > > >
> > > > >
> > > > > > > - const: sifive,plic-1.0.0
> > > > > >
> > > > > > The PLIC DT node requires two compatible string:
> > > > > > <implementation_compat>, <spec_compat>
> > > > > >
> > > > > > The C9xx PLIC is not RISC-V PLIC so, the DT node should
> > > > > > be: "thead,c900-plic", "thead,c9xx-plic"
> > > > > >
> > > > > > You need to change "- const: sifive,plic-1.0.0" to
> > > > > > - enum:
> > > > > > - sifive,plic-1.0.0
> > > > > > - thead,c9xx-plic
>
> isn't XuanTie the series containing the c906 and c910?
> So maybe
> thead,xuantie-plic
> for the spec compatible.
>
> So doing in full
> compatible = "thead,c906-plic", "thead,xuantie-plic"

This is a much better suggestion. I will let Guo decide.

Regards,
Anup

>
>
> Heiko
>
>

2021-10-13 11:02:02

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

Am Mittwoch, 13. Oktober 2021, 11:49:20 CEST schrieb Anup Patel:
> On Wed, Oct 13, 2021 at 3:13 PM Heiko St?bner <[email protected]> wrote:
> >
> > Am Mittwoch, 13. Oktober 2021, 11:19:53 CEST schrieb Anup Patel:
> > > On Wed, Oct 13, 2021 at 2:44 PM Heiko St?bner <[email protected]> wrote:
> > > >
> > > > Am Mittwoch, 13. Oktober 2021, 11:11:26 CEST schrieb Anup Patel:
> > > > > On Wed, Oct 13, 2021 at 2:27 PM Heiko St?bner <[email protected]> wrote:
> > > > > >
> > > > > > Hi Anup,
> > > > > >
> > > > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > > > On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> > > > > > > >
> > > > > > > > From: Guo Ren <[email protected]>
> > > > > > > >
> > > > > > > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > > > > > > bindings to support SOCs with thead,c9xx processor cores.
> > > > > > > >
> > > > > > > > Signed-off-by: Guo Ren <[email protected]>
> > > > > > > > Cc: Rob Herring <[email protected]>
> > > > > > > > Cc: Palmer Dabbelt <[email protected]>
> > > > > > > > Cc: Anup Patel <[email protected]>
> > > > > > > > Cc: Atish Patra <[email protected]>
> > > > > > > >
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Changes since V3:
> > > > > > > > - Rename "c9xx" to "c900"
> > > > > > > > - Add thead,c900-plic in the description section
> > > > > > > > ---
> > > > > > > > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > > > > > > > 1 file changed, 6 insertions(+)
> > > > > > > >
> > > > > > > > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> > > > > > > >
> > > > > > > > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > > > > > > > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > > > > > > > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > > > > > > > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
> > > > > > >
> > > > > > > This is a totally incorrect description of the errata required for C9xx PLIC.
> > > > > > >
> > > > > > > Please don't project non-compliance as a feature of C9xx PLIC.
> > > > > > >
> > > > > > > > +
> > > > > > > > maintainers:
> > > > > > > > - Sagar Kadam <[email protected]>
> > > > > > > > - Paul Walmsley <[email protected]>
> > > > > > > > @@ -46,6 +51,7 @@ properties:
> > > > > > > > - enum:
> > > > > > > > - sifive,fu540-c000-plic
> > > > > > > > - canaan,k210-plic
> > > > > > > > + - thead,c900-plic
> > > > > >
> > > > > > we still want specific SoC names in the compatible, the "c900"
> > > > > > is still a sort-of placeholder.
> > > > >
> > > > > Yes, we need "c900" compatible string as well. The "c9xx"
> > > > > compatible string is for the custom PLIC spec followed by T-HEAD.
> > > >
> > > > What I meant was that the soc-specific string should name the
> > > > actual SoC (c906, c910) and not some imaginary chip ;-)
> > >
> > > Ahh, yes. It should be an actual soc name in the compatible
> > > string.
> > >
> > > For example, SiFive uses "fu540" string to identify some of the
> > > devices on both SiFive unleashed and SiFive unmatched boards.
> > >
> > > I was under the impression that "c900" is an actual SoC name.
> > >
> > > Regards,
> > > Anup
> > >
> > > >
> > > > See for example mali gpu bindings for a similar reference
> > > > in devicetree/bindings/gpu/arm,mali-bifrost.yaml .
> > > >
> > > >
> > > >
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > > > - const: sifive,plic-1.0.0
> > > > > > >
> > > > > > > The PLIC DT node requires two compatible string:
> > > > > > > <implementation_compat>, <spec_compat>
> > > > > > >
> > > > > > > The C9xx PLIC is not RISC-V PLIC so, the DT node should
> > > > > > > be: "thead,c900-plic", "thead,c9xx-plic"
> > > > > > >
> > > > > > > You need to change "- const: sifive,plic-1.0.0" to
> > > > > > > - enum:
> > > > > > > - sifive,plic-1.0.0
> > > > > > > - thead,c9xx-plic
> >
> > isn't XuanTie the series containing the c906 and c910?
> > So maybe
> > thead,xuantie-plic
> > for the spec compatible.
> >
> > So doing in full
> > compatible = "thead,c906-plic", "thead,xuantie-plic"
>
> This is a much better suggestion. I will let Guo decide.

In any case, we'll also need a new entry in
devicetree/bindings/vendor-prefixes.yaml
for the "thead" prefix in a separate patch, as it looks like such a
thing is also still missing.

Heiko

2021-10-13 12:38:06

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

On Wed, Oct 13, 2021 at 4:57 PM Heiko Stübner <[email protected]> wrote:
>
> Hi Anup,
>
> Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> > >
> > > From: Guo Ren <[email protected]>
> > >
> > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > bindings to support SOCs with thead,c9xx processor cores.
> > >
> > > Signed-off-by: Guo Ren <[email protected]>
> > > Cc: Rob Herring <[email protected]>
> > > Cc: Palmer Dabbelt <[email protected]>
> > > Cc: Anup Patel <[email protected]>
> > > Cc: Atish Patra <[email protected]>
> > >
> > > ---
> > >
> > > Changes since V3:
> > > - Rename "c9xx" to "c900"
> > > - Add thead,c900-plic in the description section
> > > ---
> > > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> > >
> > > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
> >
> > This is a totally incorrect description of the errata required for C9xx PLIC.
> >
> > Please don't project non-compliance as a feature of C9xx PLIC.
> >
> > > +
> > > maintainers:
> > > - Sagar Kadam <[email protected]>
> > > - Paul Walmsley <[email protected]>
> > > @@ -46,6 +51,7 @@ properties:
> > > - enum:
> > > - sifive,fu540-c000-plic
> > > - canaan,k210-plic
> > > + - thead,c900-plic
>
> we still want specific SoC names in the compatible, the "c900"
> is still a sort-of placeholder.
c900 is not a SOC name, but a CPU name. For soc name, it should be
"allwinner,d1-plic".

>
>
> > > - const: sifive,plic-1.0.0
> >
> > The PLIC DT node requires two compatible string:
> > <implementation_compat>, <spec_compat>
> >
> > The C9xx PLIC is not RISC-V PLIC so, the DT node should
> > be: "thead,c900-plic", "thead,c9xx-plic"
> >
> > You need to change "- const: sifive,plic-1.0.0" to
> > - enum:
> > - sifive,plic-1.0.0
> > - thead,c9xx-plic
>
>

--
Best Regards
Guo Ren

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

2021-10-13 12:42:04

by Marc Zyngier

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

On 2021-10-13 02:21, [email protected] wrote:
> From: Guo Ren <[email protected]>
>
> Add the compatible string "thead,c900-plic" to the riscv plic
> bindings to support SOCs with thead,c9xx processor cores.
>
> Signed-off-by: Guo Ren <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Palmer Dabbelt <[email protected]>
> Cc: Anup Patel <[email protected]>
> Cc: Atish Patra <[email protected]>

Please add a cover letter when sending more than a single patch.

This isn't complicated, and git will do it for you if you kindly ask.

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

2021-10-13 12:44:51

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

On Wed, Oct 13, 2021 at 8:39 PM Marc Zyngier <[email protected]> wrote:
>
> On 2021-10-13 02:21, [email protected] wrote:
> > From: Guo Ren <[email protected]>
> >
> > Add the compatible string "thead,c900-plic" to the riscv plic
> > bindings to support SOCs with thead,c9xx processor cores.
> >
> > Signed-off-by: Guo Ren <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Palmer Dabbelt <[email protected]>
> > Cc: Anup Patel <[email protected]>
> > Cc: Atish Patra <[email protected]>
>
> Please add a cover letter when sending more than a single patch.
>
> This isn't complicated, and git will do it for you if you kindly ask.
Okay

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

--
Best Regards
Guo Ren

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

2021-10-13 12:52:18

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

On Wed, Oct 13, 2021 at 5:43 PM Heiko Stübner <[email protected]> wrote:
>
> Am Mittwoch, 13. Oktober 2021, 11:19:53 CEST schrieb Anup Patel:
> > On Wed, Oct 13, 2021 at 2:44 PM Heiko Stübner <[email protected]> wrote:
> > >
> > > Am Mittwoch, 13. Oktober 2021, 11:11:26 CEST schrieb Anup Patel:
> > > > On Wed, Oct 13, 2021 at 2:27 PM Heiko Stübner <[email protected]> wrote:
> > > > >
> > > > > Hi Anup,
> > > > >
> > > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > > On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> > > > > > >
> > > > > > > From: Guo Ren <[email protected]>
> > > > > > >
> > > > > > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > > > > > bindings to support SOCs with thead,c9xx processor cores.
> > > > > > >
> > > > > > > Signed-off-by: Guo Ren <[email protected]>
> > > > > > > Cc: Rob Herring <[email protected]>
> > > > > > > Cc: Palmer Dabbelt <[email protected]>
> > > > > > > Cc: Anup Patel <[email protected]>
> > > > > > > Cc: Atish Patra <[email protected]>
> > > > > > >
> > > > > > > ---
> > > > > > >
> > > > > > > Changes since V3:
> > > > > > > - Rename "c9xx" to "c900"
> > > > > > > - Add thead,c900-plic in the description section
> > > > > > > ---
> > > > > > > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > > > > > > 1 file changed, 6 insertions(+)
> > > > > > >
> > > > > > > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> > > > > > >
> > > > > > > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > > > > > > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > > > > > > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > > > > > > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
> > > > > >
> > > > > > This is a totally incorrect description of the errata required for C9xx PLIC.
> > > > > >
> > > > > > Please don't project non-compliance as a feature of C9xx PLIC.
> > > > > >
> > > > > > > +
> > > > > > > maintainers:
> > > > > > > - Sagar Kadam <[email protected]>
> > > > > > > - Paul Walmsley <[email protected]>
> > > > > > > @@ -46,6 +51,7 @@ properties:
> > > > > > > - enum:
> > > > > > > - sifive,fu540-c000-plic
> > > > > > > - canaan,k210-plic
> > > > > > > + - thead,c900-plic
> > > > >
> > > > > we still want specific SoC names in the compatible, the "c900"
> > > > > is still a sort-of placeholder.
> > > >
> > > > Yes, we need "c900" compatible string as well. The "c9xx"
> > > > compatible string is for the custom PLIC spec followed by T-HEAD.
> > >
> > > What I meant was that the soc-specific string should name the
> > > actual SoC (c906, c910) and not some imaginary chip ;-)
> >
> > Ahh, yes. It should be an actual soc name in the compatible
> > string.
> >
> > For example, SiFive uses "fu540" string to identify some of the
> > devices on both SiFive unleashed and SiFive unmatched boards.
> >
> > I was under the impression that "c900" is an actual SoC name.
> >
> > Regards,
> > Anup
> >
> > >
> > > See for example mali gpu bindings for a similar reference
> > > in devicetree/bindings/gpu/arm,mali-bifrost.yaml .
> > >
> > >
> > >
> > >
> > > >
> > > > >
> > > > >
> > > > > > > - const: sifive,plic-1.0.0
> > > > > >
> > > > > > The PLIC DT node requires two compatible string:
> > > > > > <implementation_compat>, <spec_compat>
> > > > > >
> > > > > > The C9xx PLIC is not RISC-V PLIC so, the DT node should
> > > > > > be: "thead,c900-plic", "thead,c9xx-plic"
> > > > > >
> > > > > > You need to change "- const: sifive,plic-1.0.0" to
> > > > > > - enum:
> > > > > > - sifive,plic-1.0.0
> > > > > > - thead,c9xx-plic
>
> isn't XuanTie the series containing the c906 and c910?
XuanTie contain two CPU series:
riscv: c906, c910
csky: c807, c810, c860

> So maybe
> thead,xuantie-plic
> for the spec compatible.
>
> So doing in full
> compatible = "thead,c906-plic", "thead,xuantie-plic"
How about:
compatible = "allwinner,d1-plic", "thead,c900-plic"

>
>
> Heiko
>
>

--
Best Regards
Guo Ren

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

2021-10-14 00:28:45

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

Hi,

Am Mittwoch, 13. Oktober 2021, 14:49:57 CEST schrieb Guo Ren:
> On Wed, Oct 13, 2021 at 5:43 PM Heiko St?bner <[email protected]> wrote:
> >
> > Am Mittwoch, 13. Oktober 2021, 11:19:53 CEST schrieb Anup Patel:
> > > On Wed, Oct 13, 2021 at 2:44 PM Heiko St?bner <[email protected]> wrote:
> > > >
> > > > Am Mittwoch, 13. Oktober 2021, 11:11:26 CEST schrieb Anup Patel:
> > > > > On Wed, Oct 13, 2021 at 2:27 PM Heiko St?bner <[email protected]> wrote:
> > > > > >
> > > > > > Hi Anup,
> > > > > >
> > > > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > > > On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> > > > > > > >
> > > > > > > > From: Guo Ren <[email protected]>
> > > > > > > >
> > > > > > > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > > > > > > bindings to support SOCs with thead,c9xx processor cores.
> > > > > > > >
> > > > > > > > Signed-off-by: Guo Ren <[email protected]>
> > > > > > > > Cc: Rob Herring <[email protected]>
> > > > > > > > Cc: Palmer Dabbelt <[email protected]>
> > > > > > > > Cc: Anup Patel <[email protected]>
> > > > > > > > Cc: Atish Patra <[email protected]>
> > > > > > > >
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Changes since V3:
> > > > > > > > - Rename "c9xx" to "c900"
> > > > > > > > - Add thead,c900-plic in the description section
> > > > > > > > ---
> > > > > > > > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > > > > > > > 1 file changed, 6 insertions(+)
> > > > > > > >
> > > > > > > > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> > > > > > > >
> > > > > > > > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > > > > > > > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > > > > > > > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > > > > > > > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
> > > > > > >
> > > > > > > This is a totally incorrect description of the errata required for C9xx PLIC.
> > > > > > >
> > > > > > > Please don't project non-compliance as a feature of C9xx PLIC.
> > > > > > >
> > > > > > > > +
> > > > > > > > maintainers:
> > > > > > > > - Sagar Kadam <[email protected]>
> > > > > > > > - Paul Walmsley <[email protected]>
> > > > > > > > @@ -46,6 +51,7 @@ properties:
> > > > > > > > - enum:
> > > > > > > > - sifive,fu540-c000-plic
> > > > > > > > - canaan,k210-plic
> > > > > > > > + - thead,c900-plic
> > > > > >
> > > > > > we still want specific SoC names in the compatible, the "c900"
> > > > > > is still a sort-of placeholder.
> > > > >
> > > > > Yes, we need "c900" compatible string as well. The "c9xx"
> > > > > compatible string is for the custom PLIC spec followed by T-HEAD.
> > > >
> > > > What I meant was that the soc-specific string should name the
> > > > actual SoC (c906, c910) and not some imaginary chip ;-)
> > >
> > > Ahh, yes. It should be an actual soc name in the compatible
> > > string.
> > >
> > > For example, SiFive uses "fu540" string to identify some of the
> > > devices on both SiFive unleashed and SiFive unmatched boards.
> > >
> > > I was under the impression that "c900" is an actual SoC name.
> > >
> > > Regards,
> > > Anup
> > >
> > > >
> > > > See for example mali gpu bindings for a similar reference
> > > > in devicetree/bindings/gpu/arm,mali-bifrost.yaml .
> > > >
> > > >
> > > >
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > > > - const: sifive,plic-1.0.0
> > > > > > >
> > > > > > > The PLIC DT node requires two compatible string:
> > > > > > > <implementation_compat>, <spec_compat>
> > > > > > >
> > > > > > > The C9xx PLIC is not RISC-V PLIC so, the DT node should
> > > > > > > be: "thead,c900-plic", "thead,c9xx-plic"
> > > > > > >
> > > > > > > You need to change "- const: sifive,plic-1.0.0" to
> > > > > > > - enum:
> > > > > > > - sifive,plic-1.0.0
> > > > > > > - thead,c9xx-plic
> >
> > isn't XuanTie the series containing the c906 and c910?
> XuanTie contain two CPU series:
> riscv: c906, c910
> csky: c807, c810, c860
>
> > So maybe
> > thead,xuantie-plic
> > for the spec compatible.
> >
> > So doing in full
> > compatible = "thead,c906-plic", "thead,xuantie-plic"
> How about:
> compatible = "allwinner,d1-plic", "thead,c900-plic"

This looks sensible.

- I guess the question in general is, is the PLIC part of the core spec
or part of the soc. In other words will all SoCs that use C9xx cores,
use this specific PLIC characteristic?

- If all C9xx-based SoCs will use this PLIC, I guess that thead,c900-plic
in your compatible above sounds pretty good.

- Should it be thead,* or t-head,* for the vendor-prefix?
(domain seems to be t-head.cn)

Heiko

2021-10-14 02:01:16

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

On Thu, Oct 14, 2021 at 8:26 AM Heiko Stuebner <[email protected]> wrote:
>
> Hi,
>
> Am Mittwoch, 13. Oktober 2021, 14:49:57 CEST schrieb Guo Ren:
> > On Wed, Oct 13, 2021 at 5:43 PM Heiko Stübner <[email protected]> wrote:
> > >
> > > Am Mittwoch, 13. Oktober 2021, 11:19:53 CEST schrieb Anup Patel:
> > > > On Wed, Oct 13, 2021 at 2:44 PM Heiko Stübner <[email protected]> wrote:
> > > > >
> > > > > Am Mittwoch, 13. Oktober 2021, 11:11:26 CEST schrieb Anup Patel:
> > > > > > On Wed, Oct 13, 2021 at 2:27 PM Heiko Stübner <[email protected]> wrote:
> > > > > > >
> > > > > > > Hi Anup,
> > > > > > >
> > > > > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > > > > On Wed, Oct 13, 2021 at 6:52 AM <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > From: Guo Ren <[email protected]>
> > > > > > > > >
> > > > > > > > > Add the compatible string "thead,c900-plic" to the riscv plic
> > > > > > > > > bindings to support SOCs with thead,c9xx processor cores.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Guo Ren <[email protected]>
> > > > > > > > > Cc: Rob Herring <[email protected]>
> > > > > > > > > Cc: Palmer Dabbelt <[email protected]>
> > > > > > > > > Cc: Anup Patel <[email protected]>
> > > > > > > > > Cc: Atish Patra <[email protected]>
> > > > > > > > >
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > Changes since V3:
> > > > > > > > > - Rename "c9xx" to "c900"
> > > > > > > > > - Add thead,c900-plic in the description section
> > > > > > > > > ---
> > > > > > > > > .../bindings/interrupt-controller/sifive,plic-1.0.0.yaml | 6 ++++++
> > > > > > > > > 1 file changed, 6 insertions(+)
> > > > > > > > >
> > > > > > > > > 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 08d5a57ce00f..82629832e5a5 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,11 @@ 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>.
> > > > > > > > >
> > > > > > > > > + While the "thead,c900-plic" would mask IRQ with readl(claim), so it needn't
> > > > > > > > > + mask/unmask which needed in RISC-V PLIC. When in IRQS_ONESHOT & IRQCHIP_EOI_THREADED
> > > > > > > > > + path, unnecessary mask operation would cause a blocking irq bug in thead,c900-plic.
> > > > > > > > > + Because when IRQ is disabled in c900, writel(hwirq, claim) would be invalid.
> > > > > > > >
> > > > > > > > This is a totally incorrect description of the errata required for C9xx PLIC.
> > > > > > > >
> > > > > > > > Please don't project non-compliance as a feature of C9xx PLIC.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > maintainers:
> > > > > > > > > - Sagar Kadam <[email protected]>
> > > > > > > > > - Paul Walmsley <[email protected]>
> > > > > > > > > @@ -46,6 +51,7 @@ properties:
> > > > > > > > > - enum:
> > > > > > > > > - sifive,fu540-c000-plic
> > > > > > > > > - canaan,k210-plic
> > > > > > > > > + - thead,c900-plic
> > > > > > >
> > > > > > > we still want specific SoC names in the compatible, the "c900"
> > > > > > > is still a sort-of placeholder.
> > > > > >
> > > > > > Yes, we need "c900" compatible string as well. The "c9xx"
> > > > > > compatible string is for the custom PLIC spec followed by T-HEAD.
> > > > >
> > > > > What I meant was that the soc-specific string should name the
> > > > > actual SoC (c906, c910) and not some imaginary chip ;-)
> > > >
> > > > Ahh, yes. It should be an actual soc name in the compatible
> > > > string.
> > > >
> > > > For example, SiFive uses "fu540" string to identify some of the
> > > > devices on both SiFive unleashed and SiFive unmatched boards.
> > > >
> > > > I was under the impression that "c900" is an actual SoC name.
> > > >
> > > > Regards,
> > > > Anup
> > > >
> > > > >
> > > > > See for example mali gpu bindings for a similar reference
> > > > > in devicetree/bindings/gpu/arm,mali-bifrost.yaml .
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > > - const: sifive,plic-1.0.0
> > > > > > > >
> > > > > > > > The PLIC DT node requires two compatible string:
> > > > > > > > <implementation_compat>, <spec_compat>
> > > > > > > >
> > > > > > > > The C9xx PLIC is not RISC-V PLIC so, the DT node should
> > > > > > > > be: "thead,c900-plic", "thead,c9xx-plic"
> > > > > > > >
> > > > > > > > You need to change "- const: sifive,plic-1.0.0" to
> > > > > > > > - enum:
> > > > > > > > - sifive,plic-1.0.0
> > > > > > > > - thead,c9xx-plic
> > >
> > > isn't XuanTie the series containing the c906 and c910?
> > XuanTie contain two CPU series:
> > riscv: c906, c910
> > csky: c807, c810, c860
> >
> > > So maybe
> > > thead,xuantie-plic
> > > for the spec compatible.
> > >
> > > So doing in full
> > > compatible = "thead,c906-plic", "thead,xuantie-plic"
> > How about:
> > compatible = "allwinner,d1-plic", "thead,c900-plic"
>
> This looks sensible.
>
> - I guess the question in general is, is the PLIC part of the core spec
> or part of the soc. In other words will all SoCs that use C9xx cores,
> use this specific PLIC characteristic?
Yes, unless soc wants to customize.

>
> - If all C9xx-based SoCs will use this PLIC, I guess that thead,c900-plic
> in your compatible above sounds pretty good.
>
> - Should it be thead,* or t-head,* for the vendor-prefix?
T-Head Semiconductor Co., Ltd.
ref: https://github.com/T-head-Semi

So it's "thead" for vendor-prefix

> (domain seems to be t-head.cn)
>
>
> Heiko
>
>

--
Best Regards
Guo Ren

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

2021-10-14 04:23:16

by Samuel Holland

[permalink] [raw]

Subject: Re: [PATCH V3 1/2] dt-bindings: update riscv plic compatible string

On 10/13/21 7:49 AM, Guo Ren wrote:
>>>>>>>> - const: sifive,plic-1.0.0
>>>>>>>
>>>>>>> The PLIC DT node requires two compatible string:
>>>>>>> <implementation_compat>, <spec_compat>
>>>>>>>
>>>>>>> The C9xx PLIC is not RISC-V PLIC so, the DT node should
>>>>>>> be: "thead,c900-plic", "thead,c9xx-plic"
>>>>>>>
>>>>>>> You need to change "- const: sifive,plic-1.0.0" to
>>>>>>> - enum:
>>>>>>> - sifive,plic-1.0.0
>>>>>>> - thead,c9xx-plic
>>
>> isn't XuanTie the series containing the c906 and c910?
> XuanTie contain two CPU series:
> riscv: c906, c910
> csky: c807, c810, c860
>
>> So maybe
>> thead,xuantie-plic
>> for the spec compatible.
>>
>> So doing in full
>> compatible = "thead,c906-plic", "thead,xuantie-plic"
> How about:
> compatible = "allwinner,d1-plic", "thead,c900-plic"

To follow the <family>-<soc>-<device> pattern of existing Allwinner
compatibles, the first string should be "allwinner,sun20i-d1-plic".
Otherwise, this looks to me like the right thing to do.

Regards,
Samuel

2021-10-14 06:20:25