[V3,1/2] dt-bindings: update riscv plic compatible string

Commit Message

Guo Ren Oct. 13, 2021, 1:21 a.m. UTC

From: Guo Ren <[e-mail]>

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 <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Palmer Dabbelt <[e-mail]>
Cc: Anup Patel <[e-mail]>
Cc: Atish Patra <[e-mail]>

---

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

Comments

Anup Patel Oct. 13, 2021, 5:11 a.m. UTC | #1

On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
>
> From: Guo Ren <[e-mail]>
>
> 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 <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Palmer Dabbelt <[e-mail]>
> Cc: Anup Patel <[e-mail]>
> Cc: Atish Patra <[e-mail]>
>
> ---
>
> 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 <[e-mail]>
>    - Paul Walmsley  <[e-mail]>
> @@ -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

Heiko Stübner Oct. 13, 2021, 8:57 a.m. UTC | #2

Hi Anup,

Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> >
> > From: Guo Ren <[e-mail]>
> >
> > 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 <[e-mail]>
> > Cc: Rob Herring <[e-mail]>
> > Cc: Palmer Dabbelt <[e-mail]>
> > Cc: Anup Patel <[e-mail]>
> > Cc: Atish Patra <[e-mail]>
> >
> > ---
> >
> > 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 <[e-mail]>
> >    - Paul Walmsley  <[e-mail]>
> > @@ -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

Anup Patel Oct. 13, 2021, 9:11 a.m. UTC | #3

On Wed, Oct 13, 2021 at 2:27 PM Heiko Stübner <[e-mail]> wrote:
>
> Hi Anup,
>
> Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> > >
> > > From: Guo Ren <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Cc: Rob Herring <[e-mail]>
> > > Cc: Palmer Dabbelt <[e-mail]>
> > > Cc: Anup Patel <[e-mail]>
> > > Cc: Atish Patra <[e-mail]>
> > >
> > > ---
> > >
> > > 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 <[e-mail]>
> > >    - Paul Walmsley  <[e-mail]>
> > > @@ -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

Heiko Stübner Oct. 13, 2021, 9:14 a.m. UTC | #4

Am Mittwoch, 13. Oktober 2021, 11:11:26 CEST schrieb Anup Patel:
> On Wed, Oct 13, 2021 at 2:27 PM Heiko Stübner <[e-mail]> wrote:
> >
> > Hi Anup,
> >
> > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> > > >
> > > > From: Guo Ren <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Cc: Rob Herring <[e-mail]>
> > > > Cc: Palmer Dabbelt <[e-mail]>
> > > > Cc: Anup Patel <[e-mail]>
> > > > Cc: Atish Patra <[e-mail]>
> > > >
> > > > ---
> > > >
> > > > 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 <[e-mail]>
> > > >    - Paul Walmsley  <[e-mail]>
> > > > @@ -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
>

Anup Patel Oct. 13, 2021, 9:19 a.m. UTC | #5

On Wed, Oct 13, 2021 at 2:44 PM Heiko Stübner <[e-mail]> 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 <[e-mail]> wrote:
> > >
> > > Hi Anup,
> > >
> > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> > > > >
> > > > > From: Guo Ren <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> > > > > Cc: Rob Herring <[e-mail]>
> > > > > Cc: Palmer Dabbelt <[e-mail]>
> > > > > Cc: Anup Patel <[e-mail]>
> > > > > Cc: Atish Patra <[e-mail]>
> > > > >
> > > > > ---
> > > > >
> > > > > 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 <[e-mail]>
> > > > >    - Paul Walmsley  <[e-mail]>
> > > > > @@ -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
> >
>
>
>
>

Heiko Stübner Oct. 13, 2021, 9:43 a.m. UTC | #6

Am Mittwoch, 13. Oktober 2021, 11:19:53 CEST schrieb Anup Patel:
> On Wed, Oct 13, 2021 at 2:44 PM Heiko Stübner <[e-mail]> 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 <[e-mail]> wrote:
> > > >
> > > > Hi Anup,
> > > >
> > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> > > > > >
> > > > > > From: Guo Ren <[e-mail]>
> > > > > >
> > > > > > 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 <[e-mail]>
> > > > > > Cc: Rob Herring <[e-mail]>
> > > > > > Cc: Palmer Dabbelt <[e-mail]>
> > > > > > Cc: Anup Patel <[e-mail]>
> > > > > > Cc: Atish Patra <[e-mail]>
> > > > > >
> > > > > > ---
> > > > > >
> > > > > > 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 <[e-mail]>
> > > > > >    - Paul Walmsley  <[e-mail]>
> > > > > > @@ -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

Anup Patel Oct. 13, 2021, 9:49 a.m. UTC | #7

On Wed, Oct 13, 2021 at 3:13 PM Heiko Stübner <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> wrote:
> > > > >
> > > > > Hi Anup,
> > > > >
> > > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > > On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> > > > > > >
> > > > > > > From: Guo Ren <[e-mail]>
> > > > > > >
> > > > > > > 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 <[e-mail]>
> > > > > > > Cc: Rob Herring <[e-mail]>
> > > > > > > Cc: Palmer Dabbelt <[e-mail]>
> > > > > > > Cc: Anup Patel <[e-mail]>
> > > > > > > Cc: Atish Patra <[e-mail]>
> > > > > > >
> > > > > > > ---
> > > > > > >
> > > > > > > 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 <[e-mail]>
> > > > > > >    - Paul Walmsley  <[e-mail]>
> > > > > > > @@ -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
>
>

Heiko Stübner Oct. 13, 2021, 10:58 a.m. UTC | #8

Am Mittwoch, 13. Oktober 2021, 11:49:20 CEST schrieb Anup Patel:
> On Wed, Oct 13, 2021 at 3:13 PM Heiko Stübner <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> wrote:
> > > > > >
> > > > > > Hi Anup,
> > > > > >
> > > > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > > > On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > From: Guo Ren <[e-mail]>
> > > > > > > >
> > > > > > > > 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 <[e-mail]>
> > > > > > > > Cc: Rob Herring <[e-mail]>
> > > > > > > > Cc: Palmer Dabbelt <[e-mail]>
> > > > > > > > Cc: Anup Patel <[e-mail]>
> > > > > > > > Cc: Atish Patra <[e-mail]>
> > > > > > > >
> > > > > > > > ---
> > > > > > > >
> > > > > > > > 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 <[e-mail]>
> > > > > > > >    - Paul Walmsley  <[e-mail]>
> > > > > > > > @@ -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

Guo Ren Oct. 13, 2021, 12:34 p.m. UTC | #9

On Wed, Oct 13, 2021 at 4:57 PM Heiko Stübner <[e-mail]> wrote:
>
> Hi Anup,
>
> Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> > >
> > > From: Guo Ren <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Cc: Rob Herring <[e-mail]>
> > > Cc: Palmer Dabbelt <[e-mail]>
> > > Cc: Anup Patel <[e-mail]>
> > > Cc: Atish Patra <[e-mail]>
> > >
> > > ---
> > >
> > > 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 <[e-mail]>
> > >    - Paul Walmsley  <[e-mail]>
> > > @@ -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
>
>

Marc Zyngier Oct. 13, 2021, 12:39 p.m. UTC | #10

On 2021-10-13 02:21, [e-mail] wrote:
> From: Guo Ren <[e-mail]>
> 
> 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 <[e-mail]>
> Cc: Rob Herring <[e-mail]>
> Cc: Palmer Dabbelt <[e-mail]>
> Cc: Anup Patel <[e-mail]>
> Cc: Atish Patra <[e-mail]>

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.

Guo Ren Oct. 13, 2021, 12:42 p.m. UTC | #11

On Wed, Oct 13, 2021 at 8:39 PM Marc Zyngier <[e-mail]> wrote:
>
> On 2021-10-13 02:21, [e-mail] wrote:
> > From: Guo Ren <[e-mail]>
> >
> > 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 <[e-mail]>
> > Cc: Rob Herring <[e-mail]>
> > Cc: Palmer Dabbelt <[e-mail]>
> > Cc: Anup Patel <[e-mail]>
> > Cc: Atish Patra <[e-mail]>
>
> 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...

Guo Ren Oct. 13, 2021, 12:49 p.m. UTC | #12

On Wed, Oct 13, 2021 at 5:43 PM Heiko Stübner <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> wrote:
> > > > >
> > > > > Hi Anup,
> > > > >
> > > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > > On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> > > > > > >
> > > > > > > From: Guo Ren <[e-mail]>
> > > > > > >
> > > > > > > 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 <[e-mail]>
> > > > > > > Cc: Rob Herring <[e-mail]>
> > > > > > > Cc: Palmer Dabbelt <[e-mail]>
> > > > > > > Cc: Anup Patel <[e-mail]>
> > > > > > > Cc: Atish Patra <[e-mail]>
> > > > > > >
> > > > > > > ---
> > > > > > >
> > > > > > > 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 <[e-mail]>
> > > > > > >    - Paul Walmsley  <[e-mail]>
> > > > > > > @@ -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
>
>

Heiko Stübner Oct. 14, 2021, 12:25 a.m. UTC | #13

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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> wrote:
> > > > > >
> > > > > > Hi Anup,
> > > > > >
> > > > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > > > On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> > > > > > > >
> > > > > > > > From: Guo Ren <[e-mail]>
> > > > > > > >
> > > > > > > > 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 <[e-mail]>
> > > > > > > > Cc: Rob Herring <[e-mail]>
> > > > > > > > Cc: Palmer Dabbelt <[e-mail]>
> > > > > > > > Cc: Anup Patel <[e-mail]>
> > > > > > > > Cc: Atish Patra <[e-mail]>
> > > > > > > >
> > > > > > > > ---
> > > > > > > >
> > > > > > > > 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 <[e-mail]>
> > > > > > > >    - Paul Walmsley  <[e-mail]>
> > > > > > > > @@ -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

Guo Ren Oct. 14, 2021, 1:56 a.m. UTC | #14

On Thu, Oct 14, 2021 at 8:26 AM Heiko Stuebner <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> 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 <[e-mail]> wrote:
> > > > > > >
> > > > > > > Hi Anup,
> > > > > > >
> > > > > > > Am Mittwoch, 13. Oktober 2021, 07:11:46 CEST schrieb Anup Patel:
> > > > > > > > On Wed, Oct 13, 2021 at 6:52 AM <[e-mail]> wrote:
> > > > > > > > >
> > > > > > > > > From: Guo Ren <[e-mail]>
> > > > > > > > >
> > > > > > > > > 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 <[e-mail]>
> > > > > > > > > Cc: Rob Herring <[e-mail]>
> > > > > > > > > Cc: Palmer Dabbelt <[e-mail]>
> > > > > > > > > Cc: Anup Patel <[e-mail]>
> > > > > > > > > Cc: Atish Patra <[e-mail]>
> > > > > > > > >
> > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > 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 <[e-mail]>
> > > > > > > > >    - Paul Walmsley  <[e-mail]>
> > > > > > > > > @@ -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
>
>

Samuel Holland Oct. 14, 2021, 4:21 a.m. UTC | #15

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

Guo Ren Oct. 14, 2021, 6:17 a.m. UTC | #16

On Thu, Oct 14, 2021 at 12:21 PM Samuel Holland <[e-mail]> wrote:
>
> 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".
Thx, Got it.
> Otherwise, this looks to me like the right thing to do.
>
> Regards,
> Samuel

Patch

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.
+
 maintainers:
   - Sagar Kadam <[e-mail]>
   - Paul Walmsley  <[e-mail]>
@@ -46,6 +51,7 @@  properties:
       - enum:
           - sifive,fu540-c000-plic
           - canaan,k210-plic
+          - thead,c900-plic
       - const: sifive,plic-1.0.0
 
   reg: