[4/9] dt-bindings: Add RISC-V incoming MSI controller bindings

  We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
defined by the RISC-V advanced interrupt architecture (AIA) specification.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
 1 file changed, 174 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
  

On Thu, Nov 10, 2022 at 8:43 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
> defined by the RISC-V advanced interrupt architecture (AIA) specification.
>
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
>  1 file changed, 174 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> new file mode 100644
> index 000000000000..05106eb1955e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> @@ -0,0 +1,174 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RISC-V Incoming MSI Controller (IMSIC)
> +
> +maintainers:
> +  - Anup Patel <anup@brainfault.org>
> +
> +description:
> +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
> +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
> +  AIA specification can be found at https://github.com/riscv/riscv-aia.
> +
> +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> +  for each privilege level (machine or supervisor). The configuration of
> +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
> +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
> +  fixed number of interrupt identities (to distinguish MSIs from devices)
> +  which is same for given privilege level across CPUs (or HARTs).
> +
> +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
> +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
> +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
> +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
> +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
> +  privilege level (machine or supervisor) encodes group index, HART index,
> +  and guest index (shown below).
> +
> +  XLEN-1           >=24                                 12    0
> +  |                  |                                  |     |
> +  -------------------------------------------------------------
> +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
> +  -------------------------------------------------------------
> +
> +  The device tree of a RISC-V platform will have one IMSIC device tree node
> +  for each privilege level (machine or supervisor) which collectively describe
> +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - vendor,chip-imsics
> +      - const: riscv,imsics
> +
> +  reg:
> +    minItems: 1
> +    maxItems: 128
> +    description:
> +      Base address of each IMSIC group.
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 0
> +
> +  msi-controller: true
> +
> +  interrupts-extended:
> +    minItems: 1
> +    maxItems: 32768
> +    description:
> +      This property represents the set of CPUs (or HARTs) for which given
> +      device tree node describes the IMSIC interrupt files. Each node pointed
> +      to should be a riscv,cpu-intc node, which has a riscv node (i.e. RISC-V
> +      HART) as parent.
> +
> +  riscv,num-ids:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 63
> +    maximum: 2047
> +    description:
> +      Specifies how many interrupt identities are supported by IMSIC interrupt
> +      file.
> +
> +  riscv,num-guest-ids:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 63
> +    maximum: 2047
> +    description:
> +      Specifies how many interrupt identities are supported by IMSIC guest
> +      interrupt file. When not specified the number of interrupt identities
> +      supported by IMSIC guest file is assumed to be same as specified by
> +      the riscv,num-ids property.
> +
> +  riscv,slow-ipi:
> +    type: boolean
> +    description:
> +      The presence of this property implies that software interrupts (i.e.
> +      IPIs) using IMSIC software injected MSIs is slower compared to other
> +      software interrupt mechanisms (such as SBI IPI) on the underlying
> +      RISC-V platform.
> +
> +  riscv,guest-index-bits:
> +    minimum: 0
> +    maximum: 7
> +    description:
> +      Specifies number of guest index bits in the MSI target address. When
> +      not specified it is assumed to be 0.
> +
> +  riscv,hart-index-bits:
> +    minimum: 0
> +    maximum: 15
> +    description:
> +      Specifies number of HART index bits in the MSI target address. When
> +      not specified it is estimated based on the interrupts-extended property.
> +
> +  riscv,group-index-bits:
> +    minimum: 0
> +    maximum: 7
> +    description:
> +      Specifies number of group index bits in the MSI target address. When
> +      not specified it is assumed to be 0.
> +
> +  riscv,group-index-shift:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 24
> +    maximum: 55
> +    description:
> +      Specifies the least significant bit of the group index bits in the
> +      MSI target address. When not specified it is assumed to be 24.
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - msi-controller
> +  - interrupts-extended
> +  - riscv,num-ids
> +
> +examples:
> +  - |
> +    // Example 1 (Machine-level IMSIC files with just one group):
> +
> +    imsic_mlevel: interrupt-controller@24000000 {
> +      compatible = "vendor,chip-imsics", "riscv,imsics";
> +      interrupts-extended = <&cpu1_intc 11>,
> +                            <&cpu2_intc 11>,
> +                            <&cpu3_intc 11>,
> +                            <&cpu4_intc 11>;
> +      reg = <0x28000000 0x4000>;

nit: /s/0x28000000/24000000

> +      interrupt-controller;
> +      #interrupt-cells = <0>;
> +      msi-controller;
> +      riscv,num-ids = <127>;
> +    };
> +
> +  - |
> +    // Example 2 (Supervisor-level IMSIC files with two groups):
> +
> +    imsic_slevel: interrupt-controller@28000000 {
> +      compatible = "vendor,chip-imsics", "riscv,imsics";
> +      interrupts-extended = <&cpu1_intc 9>,
> +                            <&cpu2_intc 9>,
> +                            <&cpu3_intc 9>,
> +                            <&cpu4_intc 9>;
> +      reg = <0x28000000 0x2000>, /* Group0 IMSICs */
> +            <0x29000000 0x2000>; /* Group1 IMSICs */
> +      interrupt-controller;
> +      #interrupt-cells = <0>;
> +      msi-controller;
> +      riscv,num-ids = <127>;
> +      riscv,group-index-bits = <1>;
> +      riscv,group-index-shift = <24>;
> +    };
> +...
> --
> 2.34.1
>
  

Hey Anup,

On Fri, Nov 11, 2022 at 10:12:02AM +0530, Anup Patel wrote:
> dt-bindings: Add RISC-V incoming MSI controller bindings

nit: it looks like the usual prefix here is "dt-bindings:
interrupt-controller".

> We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
> defined by the RISC-V advanced interrupt architecture (AIA) specification.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
>  1 file changed, 174 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> new file mode 100644
> index 000000000000..05106eb1955e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> @@ -0,0 +1,174 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RISC-V Incoming MSI Controller (IMSIC)
> +
> +maintainers:
> +  - Anup Patel <anup@brainfault.org>
> +
> +description:

Is this one of the situations where we want to have a | after
"description:" to preserve formatting?

> +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
> +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
> +  AIA specification can be found at https://github.com/riscv/riscv-aia.
> +
> +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> +  for each privilege level (machine or supervisor). The configuration of
> +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
> +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
> +  fixed number of interrupt identities (to distinguish MSIs from devices)
> +  which is same for given privilege level across CPUs (or HARTs).
> +
> +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
> +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
> +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
> +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
> +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
> +  privilege level (machine or supervisor) encodes group index, HART index,
> +  and guest index (shown below).
> +
> +  XLEN-1           >=24                                 12    0
> +  |                  |                                  |     |
> +  -------------------------------------------------------------
> +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
> +  -------------------------------------------------------------
> +
> +  The device tree of a RISC-V platform will have one IMSIC device tree node
> +  for each privilege level (machine or supervisor) which collectively describe
> +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - vendor,chip-imsics

Is it valid to have a dummy here? I did a bit of grepping & could not
see a single other yaml binding which used a placeholder like this -
other than the example schema itself. I assume you're trying to get
across the point that using the bare riscv,imsics is not okay and a
vendor should create a custom string for their implementation?

Also, the file name says "riscv,imsic", the description says "IMSIC" but
you've used "imsics" in the compatible. Is this a typo, or a plural?

Thanks,
Conor.

> +      - const: riscv,imsics
> +
> +  reg:
> +    minItems: 1
> +    maxItems: 128
> +    description:
> +      Base address of each IMSIC group.
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 0
> +
> +  msi-controller: true
> +
> +  interrupts-extended:
> +    minItems: 1
> +    maxItems: 32768
> +    description:
> +      This property represents the set of CPUs (or HARTs) for which given
> +      device tree node describes the IMSIC interrupt files. Each node pointed
> +      to should be a riscv,cpu-intc node, which has a riscv node (i.e. RISC-V
> +      HART) as parent.
> +
> +  riscv,num-ids:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 63
> +    maximum: 2047
> +    description:
> +      Specifies how many interrupt identities are supported by IMSIC interrupt
> +      file.
> +
> +  riscv,num-guest-ids:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 63
> +    maximum: 2047
> +    description:
> +      Specifies how many interrupt identities are supported by IMSIC guest
> +      interrupt file. When not specified the number of interrupt identities
> +      supported by IMSIC guest file is assumed to be same as specified by
> +      the riscv,num-ids property.
> +
> +  riscv,slow-ipi:
> +    type: boolean
> +    description:
> +      The presence of this property implies that software interrupts (i.e.
> +      IPIs) using IMSIC software injected MSIs is slower compared to other
> +      software interrupt mechanisms (such as SBI IPI) on the underlying
> +      RISC-V platform.
> +
> +  riscv,guest-index-bits:
> +    minimum: 0
> +    maximum: 7
> +    description:
> +      Specifies number of guest index bits in the MSI target address. When
> +      not specified it is assumed to be 0.
> +
> +  riscv,hart-index-bits:
> +    minimum: 0
> +    maximum: 15
> +    description:
> +      Specifies number of HART index bits in the MSI target address. When
> +      not specified it is estimated based on the interrupts-extended property.
> +
> +  riscv,group-index-bits:
> +    minimum: 0
> +    maximum: 7
> +    description:
> +      Specifies number of group index bits in the MSI target address. When
> +      not specified it is assumed to be 0.
> +
> +  riscv,group-index-shift:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 24
> +    maximum: 55
> +    description:
> +      Specifies the least significant bit of the group index bits in the
> +      MSI target address. When not specified it is assumed to be 24.
> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - msi-controller
> +  - interrupts-extended
> +  - riscv,num-ids
> +
> +examples:
> +  - |
> +    // Example 1 (Machine-level IMSIC files with just one group):
> +
> +    imsic_mlevel: interrupt-controller@24000000 {
> +      compatible = "vendor,chip-imsics", "riscv,imsics";
> +      interrupts-extended = <&cpu1_intc 11>,
> +                            <&cpu2_intc 11>,
> +                            <&cpu3_intc 11>,
> +                            <&cpu4_intc 11>;
> +      reg = <0x28000000 0x4000>;
> +      interrupt-controller;
> +      #interrupt-cells = <0>;
> +      msi-controller;
> +      riscv,num-ids = <127>;
> +    };
> +
> +  - |
> +    // Example 2 (Supervisor-level IMSIC files with two groups):
> +
> +    imsic_slevel: interrupt-controller@28000000 {
> +      compatible = "vendor,chip-imsics", "riscv,imsics";
> +      interrupts-extended = <&cpu1_intc 9>,
> +                            <&cpu2_intc 9>,
> +                            <&cpu3_intc 9>,
> +                            <&cpu4_intc 9>;
> +      reg = <0x28000000 0x2000>, /* Group0 IMSICs */
> +            <0x29000000 0x2000>; /* Group1 IMSICs */
> +      interrupt-controller;
> +      #interrupt-cells = <0>;
> +      msi-controller;
> +      riscv,num-ids = <127>;
> +      riscv,group-index-bits = <1>;
> +      riscv,group-index-shift = <24>;
> +    };
> +...
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
  

On 11/11/2022 05:42, Anup Patel wrote:
> We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
> defined by the RISC-V advanced interrupt architecture (AIA) specification.
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
>  1 file changed, 174 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> new file mode 100644
> index 000000000000..05106eb1955e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> @@ -0,0 +1,174 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RISC-V Incoming MSI Controller (IMSIC)
> +
> +maintainers:
> +  - Anup Patel <anup@brainfault.org>
> +
> +description:
> +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
> +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
> +  AIA specification can be found at https://github.com/riscv/riscv-aia.
> +
> +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> +  for each privilege level (machine or supervisor). The configuration of
> +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
> +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
> +  fixed number of interrupt identities (to distinguish MSIs from devices)
> +  which is same for given privilege level across CPUs (or HARTs).
> +
> +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
> +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
> +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
> +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
> +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
> +  privilege level (machine or supervisor) encodes group index, HART index,
> +  and guest index (shown below).
> +
> +  XLEN-1           >=24                                 12    0
> +  |                  |                                  |     |
> +  -------------------------------------------------------------
> +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
> +  -------------------------------------------------------------
> +
> +  The device tree of a RISC-V platform will have one IMSIC device tree node
> +  for each privilege level (machine or supervisor) which collectively describe
> +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - vendor,chip-imsics

There is no such vendor... As Conor pointed out, this does not look
correct. Compatibles must be real and specific.

> +      - const: riscv,imsics
> +
> +  reg:
> +    minItems: 1
> +    maxItems: 128

Is there a DTS with 128 reg items?

> +    description:
> +      Base address of each IMSIC group.
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 0
> +
> +  msi-controller: true

You want then msi-controller.yaml schema and you can drop properties
described there.

> +
> +  interrupts-extended:
> +    minItems: 1
> +    maxItems: 32768

I just wonder if you are not putting some random stuff here... just like
this "vendor" company.

32768 inputs it is quite a big chip. Are you sure you have so many pins
or internal connections?

> +    description:
> +      This property represents the set of CPUs (or HARTs) for which given
> +      device tree node describes the IMSIC interrupt files. Each node pointed
> +      to should be a riscv,cpu-intc node, which has a riscv node (i.e. RISC-V
> +      HART) as parent.
> +
> +  riscv,num-ids:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 63
> +    maximum: 2047
> +    description:
> +      Specifies how many interrupt identities are supported by IMSIC interrupt
> +      file.
> +
> +  riscv,num-guest-ids:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 63
> +    maximum: 2047
> +    description:
> +      Specifies how many interrupt identities are supported by IMSIC guest
> +      interrupt file. When not specified the number of interrupt identities
> +      supported by IMSIC guest file is assumed to be same as specified by
> +      the riscv,num-ids property.
> +
> +  riscv,slow-ipi:
> +    type: boolean
> +    description:
> +      The presence of this property implies that software interrupts (i.e.
> +      IPIs) using IMSIC software injected MSIs is slower compared to other
> +      software interrupt mechanisms (such as SBI IPI) on the underlying
> +      RISC-V platform.

Is this a property of software or hardware?

> +
> +  riscv,guest-index-bits:
> +    minimum: 0
> +    maximum: 7
> +    description:
> +      Specifies number of guest index bits in the MSI target address. When
> +      not specified it is assumed to be 0.
> +
> +  riscv,hart-index-bits:
> +    minimum: 0
> +    maximum: 15
> +    description:
> +      Specifies number of HART index bits in the MSI target address. When
> +      not specified it is estimated based on the interrupts-extended property.
> +
> +  riscv,group-index-bits:
> +    minimum: 0
> +    maximum: 7
> +    description:
> +      Specifies number of group index bits in the MSI target address. When
> +      not specified it is assumed to be 0.

Then default: 0.

> +
> +  riscv,group-index-shift:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 24
> +    maximum: 55
> +    description:
> +      Specifies the least significant bit of the group index bits in the

Please drop everywhere "Specifies the" and instead just describe the
hardware.

> +      MSI target address. When not specified it is assumed to be 24.
> +
> +additionalProperties: false

unevaluatedProperties: false and drop all properties already described
by other schemas.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - msi-controller
> +  - interrupts-extended
> +  - riscv,num-ids
> +
> +examples:
> +  - |
> +    // Example 1 (Machine-level IMSIC files with just one group):
> +
> +    imsic_mlevel: interrupt-controller@24000000 {
> +      compatible = "vendor,chip-imsics", "riscv,imsics";
> +      interrupts-extended = <&cpu1_intc 11>,
> +                            <&cpu2_intc 11>,
> +                            <&cpu3_intc 11>,
> +                            <&cpu4_intc 11>;
> +      reg = <0x28000000 0x4000>;
> +      interrupt-controller;
> +      #interrupt-cells = <0>;
> +      msi-controller;
> +      riscv,num-ids = <127>;
> +    };
> +
> +  - |
> +    // Example 2 (Supervisor-level IMSIC files with two groups):
> +
> +    imsic_slevel: interrupt-controller@28000000 {
> +      compatible = "vendor,chip-imsics", "riscv,imsics";

Please run scripts/checkpatch.pl and fix reported warnings.

Best regards,
Krzysztof
  

On Mon, Nov 14, 2022 at 3:19 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/11/2022 05:42, Anup Patel wrote:
> > We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
> > defined by the RISC-V advanced interrupt architecture (AIA) specification.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
> >  1 file changed, 174 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > new file mode 100644
> > index 000000000000..05106eb1955e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > @@ -0,0 +1,174 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RISC-V Incoming MSI Controller (IMSIC)
> > +
> > +maintainers:
> > +  - Anup Patel <anup@brainfault.org>
> > +
> > +description:
> > +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
> > +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
> > +  AIA specification can be found at https://github.com/riscv/riscv-aia.
> > +
> > +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> > +  for each privilege level (machine or supervisor). The configuration of
> > +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
> > +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
> > +  fixed number of interrupt identities (to distinguish MSIs from devices)
> > +  which is same for given privilege level across CPUs (or HARTs).
> > +
> > +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
> > +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
> > +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
> > +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
> > +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
> > +  privilege level (machine or supervisor) encodes group index, HART index,
> > +  and guest index (shown below).
> > +
> > +  XLEN-1           >=24                                 12    0
> > +  |                  |                                  |     |
> > +  -------------------------------------------------------------
> > +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
> > +  -------------------------------------------------------------
> > +
> > +  The device tree of a RISC-V platform will have one IMSIC device tree node
> > +  for each privilege level (machine or supervisor) which collectively describe
> > +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
> > +
> > +allOf:
> > +  - $ref: /schemas/interrupt-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - vendor,chip-imsics
>
> There is no such vendor... As Conor pointed out, this does not look
> correct. Compatibles must be real and specific.

Previously, Rob had suggest to:
1) Mandate two compatible strings: one for implementation and
    and second for specification
2) Since this is new specification with QEMU being the only
    implementation, we add "vendor,chip-imsics" as dummy
    implementation specific string for DT schema checkers
    to pass the examples. Once we have an actual implementation,
   we will replace this dummy string.

Refer, https://www.spinics.net/lists/devicetree/msg442720.html

>
> > +      - const: riscv,imsics
> > +
> > +  reg:
> > +    minItems: 1
> > +    maxItems: 128
>
> Is there a DTS with 128 reg items?

Not at the moment since this is a new specification.

The value "128" is because maximum number of
IMSIC groups on an system with both IMSIC and
APLIC is 128 where each IMSIC group has a
separate base address. This is not a hard limit so
I am willing to drop it as well.

>
> > +    description:
> > +      Base address of each IMSIC group.
> > +
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 0
> > +
> > +  msi-controller: true
>
> You want then msi-controller.yaml schema and you can drop properties
> described there.

Okay, I will include msi-controller.yaml in the next revision.

>
> > +
> > +  interrupts-extended:
> > +    minItems: 1
> > +    maxItems: 32768
>
> I just wonder if you are not putting some random stuff here... just like
> this "vendor" company.
>
> 32768 inputs it is quite a big chip. Are you sure you have so many pins
> or internal connections?

The interrupts-extended property describes the association of IMSIC
interrupt files with the HARTs. If there are N HARTs then we will have
N entries in the interrupts-extended (just like the existing PLIC DT bindings).

For example, if the first entry points to HART1 and the second entry points
to HART0 then the first interrupt file is associated with HART1 and the
second interrupt file is associated with HART0.

Currently, the "maxItems" limit reflects the max IMSICs which an APLIC
domain can target on a system with both IMSIC and APLIC.

Actually, there is a typo here. The "maxItems" should be 16384 as-per
the frozen AIA specification. I will update "maxItems" accordingly in
next patch revision.

>
> > +    description:
> > +      This property represents the set of CPUs (or HARTs) for which given
> > +      device tree node describes the IMSIC interrupt files. Each node pointed
> > +      to should be a riscv,cpu-intc node, which has a riscv node (i.e. RISC-V
> > +      HART) as parent.
> > +
> > +  riscv,num-ids:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 63
> > +    maximum: 2047
> > +    description:
> > +      Specifies how many interrupt identities are supported by IMSIC interrupt
> > +      file.
> > +
> > +  riscv,num-guest-ids:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 63
> > +    maximum: 2047
> > +    description:
> > +      Specifies how many interrupt identities are supported by IMSIC guest
> > +      interrupt file. When not specified the number of interrupt identities
> > +      supported by IMSIC guest file is assumed to be same as specified by
> > +      the riscv,num-ids property.
> > +
> > +  riscv,slow-ipi:
> > +    type: boolean
> > +    description:
> > +      The presence of this property implies that software interrupts (i.e.
> > +      IPIs) using IMSIC software injected MSIs is slower compared to other
> > +      software interrupt mechanisms (such as SBI IPI) on the underlying
> > +      RISC-V platform.
>
> Is this a property of software or hardware?

This is a property of hardware (or implementation) because IPIs
in IMSIC are software injected MSIs so if IMSIC is trap-n-emulated
by a hypervisor then all writes to MSI register will trap to hypervisor
in which case IPI injection via IMSIC is slow.

The presence of "riscv,slow-ipi" DT property provides a hint to
driver that using IPIs through IMSIC is slow on this platform so
if there are other IPI mechanisms (such as SBI IPI calls) then
OS should prefer those mechanisms.

>
> > +
> > +  riscv,guest-index-bits:
> > +    minimum: 0
> > +    maximum: 7
> > +    description:
> > +      Specifies number of guest index bits in the MSI target address. When
> > +      not specified it is assumed to be 0.
> > +
> > +  riscv,hart-index-bits:
> > +    minimum: 0
> > +    maximum: 15
> > +    description:
> > +      Specifies number of HART index bits in the MSI target address. When
> > +      not specified it is estimated based on the interrupts-extended property.
> > +
> > +  riscv,group-index-bits:
> > +    minimum: 0
> > +    maximum: 7
> > +    description:
> > +      Specifies number of group index bits in the MSI target address. When
> > +      not specified it is assumed to be 0.
>
> Then default: 0.

Okay, I will update.

>
> > +
> > +  riscv,group-index-shift:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 24
> > +    maximum: 55
> > +    description:
> > +      Specifies the least significant bit of the group index bits in the
>
> Please drop everywhere "Specifies the" and instead just describe the
> hardware.

Okay, I will update.

>
> > +      MSI target address. When not specified it is assumed to be 24.
> > +
> > +additionalProperties: false
>
> unevaluatedProperties: false and drop all properties already described
> by other schemas.

Okay, I will update.

>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupt-controller
> > +  - msi-controller
> > +  - interrupts-extended
> > +  - riscv,num-ids
> > +
> > +examples:
> > +  - |
> > +    // Example 1 (Machine-level IMSIC files with just one group):
> > +
> > +    imsic_mlevel: interrupt-controller@24000000 {
> > +      compatible = "vendor,chip-imsics", "riscv,imsics";
> > +      interrupts-extended = <&cpu1_intc 11>,
> > +                            <&cpu2_intc 11>,
> > +                            <&cpu3_intc 11>,
> > +                            <&cpu4_intc 11>;
> > +      reg = <0x28000000 0x4000>;
> > +      interrupt-controller;
> > +      #interrupt-cells = <0>;
> > +      msi-controller;
> > +      riscv,num-ids = <127>;
> > +    };
> > +
> > +  - |
> > +    // Example 2 (Supervisor-level IMSIC files with two groups):
> > +
> > +    imsic_slevel: interrupt-controller@28000000 {
> > +      compatible = "vendor,chip-imsics", "riscv,imsics";
>
> Please run scripts/checkpatch.pl and fix reported warnings.

I did not see any warnings with ./scripts/checkpatch.pl.

Is there any parameter of checkpatch.pl which I should try ?

Best Regards,
Anup
  

On Mon, Nov 14, 2022 at 05:36:06PM +0530, Anup Patel wrote:
> On Mon, Nov 14, 2022 at 3:19 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 11/11/2022 05:42, Anup Patel wrote:
> > > We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
> > > defined by the RISC-V advanced interrupt architecture (AIA) specification.
> > >
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > ---
> > >  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
> > >  1 file changed, 174 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > > new file mode 100644
> > > index 000000000000..05106eb1955e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > > @@ -0,0 +1,174 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: RISC-V Incoming MSI Controller (IMSIC)
> > > +
> > > +maintainers:
> > > +  - Anup Patel <anup@brainfault.org>
> > > +
> > > +description:
> > > +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
> > > +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
> > > +  AIA specification can be found at https://github.com/riscv/riscv-aia.
> > > +
> > > +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> > > +  for each privilege level (machine or supervisor). The configuration of
> > > +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
> > > +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
> > > +  fixed number of interrupt identities (to distinguish MSIs from devices)
> > > +  which is same for given privilege level across CPUs (or HARTs).
> > > +
> > > +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
> > > +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
> > > +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
> > > +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
> > > +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
> > > +  privilege level (machine or supervisor) encodes group index, HART index,
> > > +  and guest index (shown below).
> > > +
> > > +  XLEN-1           >=24                                 12    0
> > > +  |                  |                                  |     |
> > > +  -------------------------------------------------------------
> > > +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
> > > +  -------------------------------------------------------------
> > > +
> > > +  The device tree of a RISC-V platform will have one IMSIC device tree node
> > > +  for each privilege level (machine or supervisor) which collectively describe
> > > +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/interrupt-controller.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:
> > > +          - vendor,chip-imsics
> >
> > There is no such vendor... As Conor pointed out, this does not look
> > correct. Compatibles must be real and specific.
> 
> Previously, Rob had suggest to:
> 1) Mandate two compatible strings: one for implementation and
>     and second for specification
> 2) Since this is new specification with QEMU being the only
>     implementation, we add "vendor,chip-imsics" as dummy
>     implementation specific string for DT schema checkers
>     to pass the examples. Once we have an actual implementation,
>    we will replace this dummy string.
> 
> Refer, https://www.spinics.net/lists/devicetree/msg442720.html

AFAIU, <vendor> and <chip> are wildcards and do not have the same
meaning as vendor & chip. That's going off of the dt submitting patches
doc though and I don't know if the tooling supports this.
  

On 14/11/2022 13:06, Anup Patel wrote:
> On Mon, Nov 14, 2022 at 3:19 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/11/2022 05:42, Anup Patel wrote:
>>> We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
>>> defined by the RISC-V advanced interrupt architecture (AIA) specification.
>>>
>>> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>>> ---
>>>  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
>>>  1 file changed, 174 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
>>> new file mode 100644
>>> index 000000000000..05106eb1955e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
>>> @@ -0,0 +1,174 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: RISC-V Incoming MSI Controller (IMSIC)
>>> +
>>> +maintainers:
>>> +  - Anup Patel <anup@brainfault.org>
>>> +
>>> +description:
>>> +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
>>> +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
>>> +  AIA specification can be found at https://github.com/riscv/riscv-aia.
>>> +
>>> +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
>>> +  for each privilege level (machine or supervisor). The configuration of
>>> +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
>>> +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
>>> +  fixed number of interrupt identities (to distinguish MSIs from devices)
>>> +  which is same for given privilege level across CPUs (or HARTs).
>>> +
>>> +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
>>> +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
>>> +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
>>> +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
>>> +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
>>> +  privilege level (machine or supervisor) encodes group index, HART index,
>>> +  and guest index (shown below).
>>> +
>>> +  XLEN-1           >=24                                 12    0
>>> +  |                  |                                  |     |
>>> +  -------------------------------------------------------------
>>> +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
>>> +  -------------------------------------------------------------
>>> +
>>> +  The device tree of a RISC-V platform will have one IMSIC device tree node
>>> +  for each privilege level (machine or supervisor) which collectively describe
>>> +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
>>> +
>>> +allOf:
>>> +  - $ref: /schemas/interrupt-controller.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - vendor,chip-imsics
>>
>> There is no such vendor... As Conor pointed out, this does not look
>> correct. Compatibles must be real and specific.
> 
> Previously, Rob had suggest to:
> 1) Mandate two compatible strings: one for implementation and
>     and second for specification
> 2) Since this is new specification with QEMU being the only
>     implementation, we add "vendor,chip-imsics" as dummy
>     implementation specific string for DT schema checkers
>     to pass the examples. Once we have an actual implementation,
>    we will replace this dummy string.
> 
> Refer, https://www.spinics.net/lists/devicetree/msg442720.html

And Rob did not propose vendor as vendor and chip-imsics as device. Read
his message again.

> 
>>
>>> +      - const: riscv,imsics
>>> +
>>> +  reg:
>>> +    minItems: 1
>>> +    maxItems: 128
>>
>> Is there a DTS with 128 reg items?
> 
> Not at the moment since this is a new specification.
> 
> The value "128" is because maximum number of
> IMSIC groups on an system with both IMSIC and
> APLIC is 128 where each IMSIC group has a
> separate base address. This is not a hard limit so
> I am willing to drop it as well.

Is "separate base address" really a separate different range or just
spaced by few registers?

> 
>>
>>> +    description:
>>> +      Base address of each IMSIC group.
>>> +
>>> +  interrupt-controller: true
>>> +
>>> +  "#interrupt-cells":
>>> +    const: 0
>>> +
>>> +  msi-controller: true
>>
>> You want then msi-controller.yaml schema and you can drop properties
>> described there.
> 
> Okay, I will include msi-controller.yaml in the next revision.
> 
>>
>>> +
>>> +  interrupts-extended:
>>> +    minItems: 1
>>> +    maxItems: 32768
>>
>> I just wonder if you are not putting some random stuff here... just like
>> this "vendor" company.
>>
>> 32768 inputs it is quite a big chip. Are you sure you have so many pins
>> or internal connections?
> 
> The interrupts-extended property describes the association of IMSIC
> interrupt files with the HARTs. If there are N HARTs then we will have
> N entries in the interrupts-extended (just like the existing PLIC DT bindings).
> 
> For example, if the first entry points to HART1 and the second entry points
> to HART0 then the first interrupt file is associated with HART1 and the
> second interrupt file is associated with HART0.
> 
> Currently, the "maxItems" limit reflects the max IMSICs which an APLIC
> domain can target on a system with both IMSIC and APLIC.
> 
> Actually, there is a typo here. The "maxItems" should be 16384 as-per
> the frozen AIA specification. I will update "maxItems" accordingly in
> next patch revision.
> 
>>
>>> +    description:
>>> +      This property represents the set of CPUs (or HARTs) for which given
>>> +      device tree node describes the IMSIC interrupt files. Each node pointed
>>> +      to should be a riscv,cpu-intc node, which has a riscv node (i.e. RISC-V
>>> +      HART) as parent.
>>> +
>>> +  riscv,num-ids:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 63
>>> +    maximum: 2047
>>> +    description:
>>> +      Specifies how many interrupt identities are supported by IMSIC interrupt
>>> +      file.
>>> +
>>> +  riscv,num-guest-ids:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 63
>>> +    maximum: 2047
>>> +    description:
>>> +      Specifies how many interrupt identities are supported by IMSIC guest
>>> +      interrupt file. When not specified the number of interrupt identities
>>> +      supported by IMSIC guest file is assumed to be same as specified by
>>> +      the riscv,num-ids property.
>>> +
>>> +  riscv,slow-ipi:
>>> +    type: boolean
>>> +    description:
>>> +      The presence of this property implies that software interrupts (i.e.
>>> +      IPIs) using IMSIC software injected MSIs is slower compared to other
>>> +      software interrupt mechanisms (such as SBI IPI) on the underlying
>>> +      RISC-V platform.
>>
>> Is this a property of software or hardware?
> 
> This is a property of hardware (or implementation) because IPIs
> in IMSIC are software injected MSIs so if IMSIC is trap-n-emulated
> by a hypervisor then all writes to MSI register will trap to hypervisor
> in which case IPI injection via IMSIC is slow.
> 
> The presence of "riscv,slow-ipi" DT property provides a hint to
> driver that using IPIs through IMSIC is slow on this platform so
> if there are other IPI mechanisms (such as SBI IPI calls) then
> OS should prefer those mechanisms.

If this is specific to implementation, why it is not included already in
the compatible?

The name is anyway too vague. What is "slow"? Describe real
characteristics of hardware, e.g. trapped via hypervisor.
> 
>>
>>> +
>>> +  riscv,guest-index-bits:
>>> +    minimum: 0
>>> +    maximum: 7
>>> +    description:
>>> +      Specifies number of guest index bits in the MSI target address. When
>>> +      not specified it is assumed to be 0.
>>> +
>>> +  riscv,hart-index-bits:
>>> +    minimum: 0
>>> +    maximum: 15
>>> +    description:
>>> +      Specifies number of HART index bits in the MSI target address. When
>>> +      not specified it is estimated based on the interrupts-extended property.
>>> +
>>> +  riscv,group-index-bits:
>>> +    minimum: 0
>>> +    maximum: 7
>>> +    description:
>>> +      Specifies number of group index bits in the MSI target address. When
>>> +      not specified it is assumed to be 0.
>>
>> Then default: 0.
> 
> Okay, I will update.
> 
>>
>>> +
>>> +  riscv,group-index-shift:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 24
>>> +    maximum: 55
>>> +    description:
>>> +      Specifies the least significant bit of the group index bits in the
>>
>> Please drop everywhere "Specifies the" and instead just describe the
>> hardware.
> 
> Okay, I will update.
> 
>>
>>> +      MSI target address. When not specified it is assumed to be 24.
>>> +
>>> +additionalProperties: false
>>
>> unevaluatedProperties: false and drop all properties already described
>> by other schemas.
> 
> Okay, I will update.
> 
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupt-controller
>>> +  - msi-controller
>>> +  - interrupts-extended
>>> +  - riscv,num-ids
>>> +
>>> +examples:
>>> +  - |
>>> +    // Example 1 (Machine-level IMSIC files with just one group):
>>> +
>>> +    imsic_mlevel: interrupt-controller@24000000 {
>>> +      compatible = "vendor,chip-imsics", "riscv,imsics";
>>> +      interrupts-extended = <&cpu1_intc 11>,
>>> +                            <&cpu2_intc 11>,
>>> +                            <&cpu3_intc 11>,
>>> +                            <&cpu4_intc 11>;
>>> +      reg = <0x28000000 0x4000>;
>>> +      interrupt-controller;
>>> +      #interrupt-cells = <0>;
>>> +      msi-controller;
>>> +      riscv,num-ids = <127>;
>>> +    };
>>> +
>>> +  - |
>>> +    // Example 2 (Supervisor-level IMSIC files with two groups):
>>> +
>>> +    imsic_slevel: interrupt-controller@28000000 {
>>> +      compatible = "vendor,chip-imsics", "riscv,imsics";
>>
>> Please run scripts/checkpatch.pl and fix reported warnings.
> 
> I did not see any warnings with ./scripts/checkpatch.pl.
> 
> Is there any parameter of checkpatch.pl which I should try ?

You should see here or with your DTS warnings about undocumented vendor
prefix.

Best regards,
Krzysztof
  

On Sun, Nov 13, 2022 at 8:18 PM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Anup,
>
> On Fri, Nov 11, 2022 at 10:12:02AM +0530, Anup Patel wrote:
> > dt-bindings: Add RISC-V incoming MSI controller bindings
>
> nit: it looks like the usual prefix here is "dt-bindings:
> interrupt-controller".

Okay, I will update.

>
> > We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
> > defined by the RISC-V advanced interrupt architecture (AIA) specification.
> >
> > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > ---
> >  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
> >  1 file changed, 174 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > new file mode 100644
> > index 000000000000..05106eb1955e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > @@ -0,0 +1,174 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RISC-V Incoming MSI Controller (IMSIC)
> > +
> > +maintainers:
> > +  - Anup Patel <anup@brainfault.org>
> > +
> > +description:
>
> Is this one of the situations where we want to have a | after
> "description:" to preserve formatting?

Okay, I will update.

>
> > +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
> > +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
> > +  AIA specification can be found at https://github.com/riscv/riscv-aia.
> > +
> > +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> > +  for each privilege level (machine or supervisor). The configuration of
> > +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
> > +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
> > +  fixed number of interrupt identities (to distinguish MSIs from devices)
> > +  which is same for given privilege level across CPUs (or HARTs).
> > +
> > +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
> > +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
> > +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
> > +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
> > +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
> > +  privilege level (machine or supervisor) encodes group index, HART index,
> > +  and guest index (shown below).
> > +
> > +  XLEN-1           >=24                                 12    0
> > +  |                  |                                  |     |
> > +  -------------------------------------------------------------
> > +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
> > +  -------------------------------------------------------------
> > +
> > +  The device tree of a RISC-V platform will have one IMSIC device tree node
> > +  for each privilege level (machine or supervisor) which collectively describe
> > +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
> > +
> > +allOf:
> > +  - $ref: /schemas/interrupt-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - vendor,chip-imsics
>
> Is it valid to have a dummy here? I did a bit of grepping & could not
> see a single other yaml binding which used a placeholder like this -
> other than the example schema itself. I assume you're trying to get
> across the point that using the bare riscv,imsics is not okay and a
> vendor should create a custom string for their implementation?

Yes, this dummy is a placeholder to mandate two compatible strings.
The dummy can eventually be replaced by some actual implementation
compatible string.

>
> Also, the file name says "riscv,imsic", the description says "IMSIC" but
> you've used "imsics" in the compatible. Is this a typo, or a plural?

Yes, the file name should be consistent. I will update the file name.

>
> Thanks,
> Conor.
>
> > +      - const: riscv,imsics
> > +
> > +  reg:
> > +    minItems: 1
> > +    maxItems: 128
> > +    description:
> > +      Base address of each IMSIC group.
> > +
> > +  interrupt-controller: true
> > +
> > +  "#interrupt-cells":
> > +    const: 0
> > +
> > +  msi-controller: true
> > +
> > +  interrupts-extended:
> > +    minItems: 1
> > +    maxItems: 32768
> > +    description:
> > +      This property represents the set of CPUs (or HARTs) for which given
> > +      device tree node describes the IMSIC interrupt files. Each node pointed
> > +      to should be a riscv,cpu-intc node, which has a riscv node (i.e. RISC-V
> > +      HART) as parent.
> > +
> > +  riscv,num-ids:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 63
> > +    maximum: 2047
> > +    description:
> > +      Specifies how many interrupt identities are supported by IMSIC interrupt
> > +      file.
> > +
> > +  riscv,num-guest-ids:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 63
> > +    maximum: 2047
> > +    description:
> > +      Specifies how many interrupt identities are supported by IMSIC guest
> > +      interrupt file. When not specified the number of interrupt identities
> > +      supported by IMSIC guest file is assumed to be same as specified by
> > +      the riscv,num-ids property.
> > +
> > +  riscv,slow-ipi:
> > +    type: boolean
> > +    description:
> > +      The presence of this property implies that software interrupts (i.e.
> > +      IPIs) using IMSIC software injected MSIs is slower compared to other
> > +      software interrupt mechanisms (such as SBI IPI) on the underlying
> > +      RISC-V platform.
> > +
> > +  riscv,guest-index-bits:
> > +    minimum: 0
> > +    maximum: 7
> > +    description:
> > +      Specifies number of guest index bits in the MSI target address. When
> > +      not specified it is assumed to be 0.
> > +
> > +  riscv,hart-index-bits:
> > +    minimum: 0
> > +    maximum: 15
> > +    description:
> > +      Specifies number of HART index bits in the MSI target address. When
> > +      not specified it is estimated based on the interrupts-extended property.
> > +
> > +  riscv,group-index-bits:
> > +    minimum: 0
> > +    maximum: 7
> > +    description:
> > +      Specifies number of group index bits in the MSI target address. When
> > +      not specified it is assumed to be 0.
> > +
> > +  riscv,group-index-shift:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 24
> > +    maximum: 55
> > +    description:
> > +      Specifies the least significant bit of the group index bits in the
> > +      MSI target address. When not specified it is assumed to be 24.
> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupt-controller
> > +  - msi-controller
> > +  - interrupts-extended
> > +  - riscv,num-ids
> > +
> > +examples:
> > +  - |
> > +    // Example 1 (Machine-level IMSIC files with just one group):
> > +
> > +    imsic_mlevel: interrupt-controller@24000000 {
> > +      compatible = "vendor,chip-imsics", "riscv,imsics";
> > +      interrupts-extended = <&cpu1_intc 11>,
> > +                            <&cpu2_intc 11>,
> > +                            <&cpu3_intc 11>,
> > +                            <&cpu4_intc 11>;
> > +      reg = <0x28000000 0x4000>;
> > +      interrupt-controller;
> > +      #interrupt-cells = <0>;
> > +      msi-controller;
> > +      riscv,num-ids = <127>;
> > +    };
> > +
> > +  - |
> > +    // Example 2 (Supervisor-level IMSIC files with two groups):
> > +
> > +    imsic_slevel: interrupt-controller@28000000 {
> > +      compatible = "vendor,chip-imsics", "riscv,imsics";
> > +      interrupts-extended = <&cpu1_intc 9>,
> > +                            <&cpu2_intc 9>,
> > +                            <&cpu3_intc 9>,
> > +                            <&cpu4_intc 9>;
> > +      reg = <0x28000000 0x2000>, /* Group0 IMSICs */
> > +            <0x29000000 0x2000>; /* Group1 IMSICs */
> > +      interrupt-controller;
> > +      #interrupt-cells = <0>;
> > +      msi-controller;
> > +      riscv,num-ids = <127>;
> > +      riscv,group-index-bits = <1>;
> > +      riscv,group-index-shift = <24>;
> > +    };
> > +...
> > --
> > 2.34.1
> >
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv

Regards,
Anup
  

On Mon, Nov 14, 2022 at 5:52 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 14/11/2022 13:06, Anup Patel wrote:
> > On Mon, Nov 14, 2022 at 3:19 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 11/11/2022 05:42, Anup Patel wrote:
> >>> We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
> >>> defined by the RISC-V advanced interrupt architecture (AIA) specification.
> >>>
> >>> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> >>> ---
> >>>  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
> >>>  1 file changed, 174 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> >>> new file mode 100644
> >>> index 000000000000..05106eb1955e
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> >>> @@ -0,0 +1,174 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: RISC-V Incoming MSI Controller (IMSIC)
> >>> +
> >>> +maintainers:
> >>> +  - Anup Patel <anup@brainfault.org>
> >>> +
> >>> +description:
> >>> +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
> >>> +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
> >>> +  AIA specification can be found at https://github.com/riscv/riscv-aia.
> >>> +
> >>> +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> >>> +  for each privilege level (machine or supervisor). The configuration of
> >>> +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
> >>> +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
> >>> +  fixed number of interrupt identities (to distinguish MSIs from devices)
> >>> +  which is same for given privilege level across CPUs (or HARTs).
> >>> +
> >>> +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
> >>> +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
> >>> +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
> >>> +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
> >>> +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
> >>> +  privilege level (machine or supervisor) encodes group index, HART index,
> >>> +  and guest index (shown below).
> >>> +
> >>> +  XLEN-1           >=24                                 12    0
> >>> +  |                  |                                  |     |
> >>> +  -------------------------------------------------------------
> >>> +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
> >>> +  -------------------------------------------------------------
> >>> +
> >>> +  The device tree of a RISC-V platform will have one IMSIC device tree node
> >>> +  for each privilege level (machine or supervisor) which collectively describe
> >>> +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
> >>> +
> >>> +allOf:
> >>> +  - $ref: /schemas/interrupt-controller.yaml#
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - enum:
> >>> +          - vendor,chip-imsics
> >>
> >> There is no such vendor... As Conor pointed out, this does not look
> >> correct. Compatibles must be real and specific.
> >
> > Previously, Rob had suggest to:
> > 1) Mandate two compatible strings: one for implementation and
> >     and second for specification
> > 2) Since this is new specification with QEMU being the only
> >     implementation, we add "vendor,chip-imsics" as dummy
> >     implementation specific string for DT schema checkers
> >     to pass the examples. Once we have an actual implementation,
> >    we will replace this dummy string.
> >
> > Refer, https://www.spinics.net/lists/devicetree/msg442720.html
>
> And Rob did not propose vendor as vendor and chip-imsics as device. Read
> his message again.

Okay

>
> >
> >>
> >>> +      - const: riscv,imsics
> >>> +
> >>> +  reg:
> >>> +    minItems: 1
> >>> +    maxItems: 128
> >>
> >> Is there a DTS with 128 reg items?
> >
> > Not at the moment since this is a new specification.
> >
> > The value "128" is because maximum number of
> > IMSIC groups on an system with both IMSIC and
> > APLIC is 128 where each IMSIC group has a
> > separate base address. This is not a hard limit so
> > I am willing to drop it as well.
>
> Is "separate base address" really a separate different range or just
> spaced by few registers?

Yes, "separate base address" of an IMSIC group
means a separate different range.

We can think of an IMSIC group as a CPU cluster or
chiplet or die. The IMSIC files within a group are
located next to each other whereas the groups can
be far away from each other.

>
> >
> >>
> >>> +    description:
> >>> +      Base address of each IMSIC group.
> >>> +
> >>> +  interrupt-controller: true
> >>> +
> >>> +  "#interrupt-cells":
> >>> +    const: 0
> >>> +
> >>> +  msi-controller: true
> >>
> >> You want then msi-controller.yaml schema and you can drop properties
> >> described there.
> >
> > Okay, I will include msi-controller.yaml in the next revision.
> >
> >>
> >>> +
> >>> +  interrupts-extended:
> >>> +    minItems: 1
> >>> +    maxItems: 32768
> >>
> >> I just wonder if you are not putting some random stuff here... just like
> >> this "vendor" company.
> >>
> >> 32768 inputs it is quite a big chip. Are you sure you have so many pins
> >> or internal connections?
> >
> > The interrupts-extended property describes the association of IMSIC
> > interrupt files with the HARTs. If there are N HARTs then we will have
> > N entries in the interrupts-extended (just like the existing PLIC DT bindings).
> >
> > For example, if the first entry points to HART1 and the second entry points
> > to HART0 then the first interrupt file is associated with HART1 and the
> > second interrupt file is associated with HART0.
> >
> > Currently, the "maxItems" limit reflects the max IMSICs which an APLIC
> > domain can target on a system with both IMSIC and APLIC.
> >
> > Actually, there is a typo here. The "maxItems" should be 16384 as-per
> > the frozen AIA specification. I will update "maxItems" accordingly in
> > next patch revision.
> >
> >>
> >>> +    description:
> >>> +      This property represents the set of CPUs (or HARTs) for which given
> >>> +      device tree node describes the IMSIC interrupt files. Each node pointed
> >>> +      to should be a riscv,cpu-intc node, which has a riscv node (i.e. RISC-V
> >>> +      HART) as parent.
> >>> +
> >>> +  riscv,num-ids:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    minimum: 63
> >>> +    maximum: 2047
> >>> +    description:
> >>> +      Specifies how many interrupt identities are supported by IMSIC interrupt
> >>> +      file.
> >>> +
> >>> +  riscv,num-guest-ids:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    minimum: 63
> >>> +    maximum: 2047
> >>> +    description:
> >>> +      Specifies how many interrupt identities are supported by IMSIC guest
> >>> +      interrupt file. When not specified the number of interrupt identities
> >>> +      supported by IMSIC guest file is assumed to be same as specified by
> >>> +      the riscv,num-ids property.
> >>> +
> >>> +  riscv,slow-ipi:
> >>> +    type: boolean
> >>> +    description:
> >>> +      The presence of this property implies that software interrupts (i.e.
> >>> +      IPIs) using IMSIC software injected MSIs is slower compared to other
> >>> +      software interrupt mechanisms (such as SBI IPI) on the underlying
> >>> +      RISC-V platform.
> >>
> >> Is this a property of software or hardware?
> >
> > This is a property of hardware (or implementation) because IPIs
> > in IMSIC are software injected MSIs so if IMSIC is trap-n-emulated
> > by a hypervisor then all writes to MSI register will trap to hypervisor
> > in which case IPI injection via IMSIC is slow.
> >
> > The presence of "riscv,slow-ipi" DT property provides a hint to
> > driver that using IPIs through IMSIC is slow on this platform so
> > if there are other IPI mechanisms (such as SBI IPI calls) then
> > OS should prefer those mechanisms.
>
> If this is specific to implementation, why it is not included already in
> the compatible?
>
> The name is anyway too vague. What is "slow"? Describe real
> characteristics of hardware, e.g. trapped via hypervisor.

Okay, how about renaming it to "riscv,trap-n-emulated" ?

Alternately, we can add "riscv,soft-imsics" as an implementation
specific compatible string which hypervisors can use to describe
trap-n-emulated IMSICs. This "riscv,soft-imsics" can also replace
"vendor,chip-imsics" dummy string ?


> >
> >>
> >>> +
> >>> +  riscv,guest-index-bits:
> >>> +    minimum: 0
> >>> +    maximum: 7
> >>> +    description:
> >>> +      Specifies number of guest index bits in the MSI target address. When
> >>> +      not specified it is assumed to be 0.
> >>> +
> >>> +  riscv,hart-index-bits:
> >>> +    minimum: 0
> >>> +    maximum: 15
> >>> +    description:
> >>> +      Specifies number of HART index bits in the MSI target address. When
> >>> +      not specified it is estimated based on the interrupts-extended property.
> >>> +
> >>> +  riscv,group-index-bits:
> >>> +    minimum: 0
> >>> +    maximum: 7
> >>> +    description:
> >>> +      Specifies number of group index bits in the MSI target address. When
> >>> +      not specified it is assumed to be 0.
> >>
> >> Then default: 0.
> >
> > Okay, I will update.
> >
> >>
> >>> +
> >>> +  riscv,group-index-shift:
> >>> +    $ref: /schemas/types.yaml#/definitions/uint32
> >>> +    minimum: 24
> >>> +    maximum: 55
> >>> +    description:
> >>> +      Specifies the least significant bit of the group index bits in the
> >>
> >> Please drop everywhere "Specifies the" and instead just describe the
> >> hardware.
> >
> > Okay, I will update.
> >
> >>
> >>> +      MSI target address. When not specified it is assumed to be 24.
> >>> +
> >>> +additionalProperties: false
> >>
> >> unevaluatedProperties: false and drop all properties already described
> >> by other schemas.
> >
> > Okay, I will update.
> >
> >>
> >>> +
> >>> +required:
> >>> +  - compatible
> >>> +  - reg
> >>> +  - interrupt-controller
> >>> +  - msi-controller
> >>> +  - interrupts-extended
> >>> +  - riscv,num-ids
> >>> +
> >>> +examples:
> >>> +  - |
> >>> +    // Example 1 (Machine-level IMSIC files with just one group):
> >>> +
> >>> +    imsic_mlevel: interrupt-controller@24000000 {
> >>> +      compatible = "vendor,chip-imsics", "riscv,imsics";
> >>> +      interrupts-extended = <&cpu1_intc 11>,
> >>> +                            <&cpu2_intc 11>,
> >>> +                            <&cpu3_intc 11>,
> >>> +                            <&cpu4_intc 11>;
> >>> +      reg = <0x28000000 0x4000>;
> >>> +      interrupt-controller;
> >>> +      #interrupt-cells = <0>;
> >>> +      msi-controller;
> >>> +      riscv,num-ids = <127>;
> >>> +    };
> >>> +
> >>> +  - |
> >>> +    // Example 2 (Supervisor-level IMSIC files with two groups):
> >>> +
> >>> +    imsic_slevel: interrupt-controller@28000000 {
> >>> +      compatible = "vendor,chip-imsics", "riscv,imsics";
> >>
> >> Please run scripts/checkpatch.pl and fix reported warnings.
> >
> > I did not see any warnings with ./scripts/checkpatch.pl.
> >
> > Is there any parameter of checkpatch.pl which I should try ?
>
> You should see here or with your DTS warnings about undocumented vendor
> prefix.

Okay, I will check.

>
> Best regards,
> Krzysztof
>

Best Regards,
Anup
  

On 14/11/2022 16:04, Anup Patel wrote:
> On Mon, Nov 14, 2022 at 5:52 PM Krzysztof Kozlowski

>>>>> +  riscv,slow-ipi:
>>>>> +    type: boolean
>>>>> +    description:
>>>>> +      The presence of this property implies that software interrupts (i.e.
>>>>> +      IPIs) using IMSIC software injected MSIs is slower compared to other
>>>>> +      software interrupt mechanisms (such as SBI IPI) on the underlying
>>>>> +      RISC-V platform.
>>>>
>>>> Is this a property of software or hardware?
>>>
>>> This is a property of hardware (or implementation) because IPIs
>>> in IMSIC are software injected MSIs so if IMSIC is trap-n-emulated
>>> by a hypervisor then all writes to MSI register will trap to hypervisor
>>> in which case IPI injection via IMSIC is slow.
>>>
>>> The presence of "riscv,slow-ipi" DT property provides a hint to
>>> driver that using IPIs through IMSIC is slow on this platform so
>>> if there are other IPI mechanisms (such as SBI IPI calls) then
>>> OS should prefer those mechanisms.
>>
>> If this is specific to implementation, why it is not included already in
>> the compatible?
>>
>> The name is anyway too vague. What is "slow"? Describe real
>> characteristics of hardware, e.g. trapped via hypervisor.
> 
> Okay, how about renaming it to "riscv,trap-n-emulated" ?

Sounds ok.

> 
> Alternately, we can add "riscv,soft-imsics" as an implementation
> specific compatible string which hypervisors can use to describe
> trap-n-emulated IMSICs. This "riscv,soft-imsics" can also replace
> "vendor,chip-imsics" dummy string ?

soft-imsics would work only if it is a real device. My question was
rather whether this is something configurable or fixed in given
implementation.

Best regards,
Krzysztof
  

On Mon, Nov 14, 2022 at 05:59:00PM +0530, Anup Patel wrote:
> On Sun, Nov 13, 2022 at 8:18 PM Conor Dooley <conor@kernel.org> wrote:

> > Also, the file name says "riscv,imsic", the description says "IMSIC" but
> > you've used "imsics" in the compatible. Is this a typo, or a plural?
> 
> Yes, the file name should be consistent. I will update the file name.

Is there a reason why the compatible is plural when all of the other
mentions etc do not have an "s"? It really did look like a typo to me.

It's the "incoming MSI controller", so I am unsure as to where the "s"
actually even comes from. Why not just use "riscv,imsic"?

Thanks,
Conor.
  

On 15/11/2022 23:34, Conor Dooley wrote:
> On Mon, Nov 14, 2022 at 05:59:00PM +0530, Anup Patel wrote:
>> On Sun, Nov 13, 2022 at 8:18 PM Conor Dooley <conor@kernel.org> wrote:
> 
>>> Also, the file name says "riscv,imsic", the description says "IMSIC" but
>>> you've used "imsics" in the compatible. Is this a typo, or a plural?
>>
>> Yes, the file name should be consistent. I will update the file name.
> 
> Is there a reason why the compatible is plural when all of the other
> mentions etc do not have an "s"? It really did look like a typo to me.
> 
> It's the "incoming MSI controller", so I am unsure as to where the "s"
> actually even comes from. Why not just use "riscv,imsic"?

Yep, should be rather consistent with all others, and IMSIC stands for
Integrated Circuit?

Best regards,
Krzysztof
  

On Wed, Nov 16, 2022 at 10:00:27AM +0100, Krzysztof Kozlowski wrote:
> On 15/11/2022 23:34, Conor Dooley wrote:
> > On Mon, Nov 14, 2022 at 05:59:00PM +0530, Anup Patel wrote:
> >> On Sun, Nov 13, 2022 at 8:18 PM Conor Dooley <conor@kernel.org> wrote:
> > 
> >>> Also, the file name says "riscv,imsic", the description says "IMSIC" but
> >>> you've used "imsics" in the compatible. Is this a typo, or a plural?
> >>
> >> Yes, the file name should be consistent. I will update the file name.
> > 
> > Is there a reason why the compatible is plural when all of the other
> > mentions etc do not have an "s"? It really did look like a typo to me.
> > 
> > It's the "incoming MSI controller", so I am unsure as to where the "s"
> > actually even comes from. Why not just use "riscv,imsic"?
> 
> Yep, should be rather consistent with all others, and IMSIC stands for
> Integrated Circuit?

Incoming Message Signalled Interrupts Controller, no?
  

On 16/11/2022 10:20, Conor Dooley wrote:
> On Wed, Nov 16, 2022 at 10:00:27AM +0100, Krzysztof Kozlowski wrote:
>> On 15/11/2022 23:34, Conor Dooley wrote:
>>> On Mon, Nov 14, 2022 at 05:59:00PM +0530, Anup Patel wrote:
>>>> On Sun, Nov 13, 2022 at 8:18 PM Conor Dooley <conor@kernel.org> wrote:
>>>
>>>>> Also, the file name says "riscv,imsic", the description says "IMSIC" but
>>>>> you've used "imsics" in the compatible. Is this a typo, or a plural?
>>>>
>>>> Yes, the file name should be consistent. I will update the file name.
>>>
>>> Is there a reason why the compatible is plural when all of the other
>>> mentions etc do not have an "s"? It really did look like a typo to me.
>>>
>>> It's the "incoming MSI controller", so I am unsure as to where the "s"
>>> actually even comes from. Why not just use "riscv,imsic"?
>>
>> Yep, should be rather consistent with all others, and IMSIC stands for
>> Integrated Circuit?
> 
> Incoming Message Signalled Interrupts Controller, no?

Ah, then still singular :)

Best regards,
Krzysztof
  

On Wed, Nov 16, 2022 at 2:30 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 15/11/2022 23:34, Conor Dooley wrote:
> > On Mon, Nov 14, 2022 at 05:59:00PM +0530, Anup Patel wrote:
> >> On Sun, Nov 13, 2022 at 8:18 PM Conor Dooley <conor@kernel.org> wrote:
> >
> >>> Also, the file name says "riscv,imsic", the description says "IMSIC" but
> >>> you've used "imsics" in the compatible. Is this a typo, or a plural?
> >>
> >> Yes, the file name should be consistent. I will update the file name.
> >
> > Is there a reason why the compatible is plural when all of the other
> > mentions etc do not have an "s"? It really did look like a typo to me.
> >
> > It's the "incoming MSI controller", so I am unsure as to where the "s"
> > actually even comes from. Why not just use "riscv,imsic"?
>
> Yep, should be rather consistent with all others, and IMSIC stands for
> Integrated Circuit?

This is intentionally plural because even though we have one
IMSIC per-CPU, Linux (and various OSes) expect one DT node
as MSI controller targeting all CPUs.

The plural compatible string "riscv,imsics" was chosen based
on consensus on RISC-V AIA Task Group meetings.

Regards,
Anup
  

On Wed, Nov 16, 2022 at 04:04:45PM +0530, Anup Patel wrote:
> On Wed, Nov 16, 2022 at 2:30 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 15/11/2022 23:34, Conor Dooley wrote:
> > > On Mon, Nov 14, 2022 at 05:59:00PM +0530, Anup Patel wrote:
> > >> On Sun, Nov 13, 2022 at 8:18 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > >>> Also, the file name says "riscv,imsic", the description says "IMSIC" but
> > >>> you've used "imsics" in the compatible. Is this a typo, or a plural?
> > >>
> > >> Yes, the file name should be consistent. I will update the file name.
> > >
> > > Is there a reason why the compatible is plural when all of the other
> > > mentions etc do not have an "s"? It really did look like a typo to me.
> > >
> > > It's the "incoming MSI controller", so I am unsure as to where the "s"
> > > actually even comes from. Why not just use "riscv,imsic"?
> >
> > Yep, should be rather consistent with all others, and IMSIC stands for
> > Integrated Circuit?
> 
> This is intentionally plural because even though we have one
> IMSIC per-CPU, Linux (and various OSes) expect one DT node
> as MSI controller targeting all CPUs.

Even still, calling it "riscv,imsic" would seem fair to me given the
multiple regs make the distinct regions clear.

I think I must have missed the bit at the end of the description though:

+  The device tree of a RISC-V platform will have one IMSIC device tree node
+  for each privilege level (machine or supervisor) which collectively describe
+  IMSIC interrupt files at that privilege level across CPUs (or HARTs).

Perhaps, for eejits like me, that paragraph should become paragraph 3
instead of hiding it below the register layout etc?

Anyways, existing name seems fine to me then w/ the filename update &
increased prominence of the many-controllers-in-one statement.

Maybe the devicetree gods think differently!

> The plural compatible string "riscv,imsics" was chosen based
> on consensus on RISC-V AIA Task Group meetings.

btw, I see the following in the example:

+      reg = <0x28000000 0x2000>, /* Group0 IMSICs */
+            <0x29000000 0x2000>; /* Group1 IMSICs */

And in the property:
+  reg:
+    minItems: 1
+    maxItems: 128
+    description:
+      Base address of each IMSIC group.

It would appear that the comment there conflicts with the description of
the reg property itself & it's that lack of consistency me confused (:
Should the comments be in the singular form?

Thanks,
Conor.
  

On Mon, Nov 14, 2022 at 05:36:06PM +0530, Anup Patel wrote:
> On Mon, Nov 14, 2022 at 3:19 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 11/11/2022 05:42, Anup Patel wrote:
> > > We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
> > > defined by the RISC-V advanced interrupt architecture (AIA) specification.
> > >
> > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > ---
> > >  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
> > >  1 file changed, 174 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > > new file mode 100644
> > > index 000000000000..05106eb1955e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > > @@ -0,0 +1,174 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: RISC-V Incoming MSI Controller (IMSIC)
> > > +
> > > +maintainers:
> > > +  - Anup Patel <anup@brainfault.org>
> > > +
> > > +description:
> > > +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
> > > +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
> > > +  AIA specification can be found at https://github.com/riscv/riscv-aia.
> > > +
> > > +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> > > +  for each privilege level (machine or supervisor). The configuration of
> > > +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
> > > +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
> > > +  fixed number of interrupt identities (to distinguish MSIs from devices)
> > > +  which is same for given privilege level across CPUs (or HARTs).
> > > +
> > > +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
> > > +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
> > > +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
> > > +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
> > > +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
> > > +  privilege level (machine or supervisor) encodes group index, HART index,
> > > +  and guest index (shown below).
> > > +
> > > +  XLEN-1           >=24                                 12    0
> > > +  |                  |                                  |     |
> > > +  -------------------------------------------------------------
> > > +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
> > > +  -------------------------------------------------------------
> > > +
> > > +  The device tree of a RISC-V platform will have one IMSIC device tree node
> > > +  for each privilege level (machine or supervisor) which collectively describe
> > > +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/interrupt-controller.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:
> > > +          - vendor,chip-imsics
> >
> > There is no such vendor... As Conor pointed out, this does not look
> > correct. Compatibles must be real and specific.
> 
> Previously, Rob had suggest to:
> 1) Mandate two compatible strings: one for implementation and
>     and second for specification
> 2) Since this is new specification with QEMU being the only
>     implementation, we add "vendor,chip-imsics" as dummy
>     implementation specific string for DT schema checkers
>     to pass the examples. Once we have an actual implementation,
>    we will replace this dummy string.

What will QEMU's DT use? That's an implementation we can and do run 
validation on. Your choices are define a QEMU specific compatible string 
or allow the fallback alone. I'm fine either way. With the latter, 
someone has to review that the fallback is not used alone in .dts files 
while doing the former allows the tools to check for you. It also 
encourages making every new difference a property rather than implied by 
compatible, but those should be caught in review.

If you go with the fallback only, just make it clear that it's for QEMU 
or s/w models only.

Rob
  

On Thu, Nov 17, 2022 at 12:44 AM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, Nov 14, 2022 at 05:36:06PM +0530, Anup Patel wrote:
> > On Mon, Nov 14, 2022 at 3:19 PM Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> > >
> > > On 11/11/2022 05:42, Anup Patel wrote:
> > > > We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
> > > > defined by the RISC-V advanced interrupt architecture (AIA) specification.
> > > >
> > > > Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> > > > ---
> > > >  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
> > > >  1 file changed, 174 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > > > new file mode 100644
> > > > index 000000000000..05106eb1955e
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > > > @@ -0,0 +1,174 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: RISC-V Incoming MSI Controller (IMSIC)
> > > > +
> > > > +maintainers:
> > > > +  - Anup Patel <anup@brainfault.org>
> > > > +
> > > > +description:
> > > > +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
> > > > +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
> > > > +  AIA specification can be found at https://github.com/riscv/riscv-aia.
> > > > +
> > > > +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> > > > +  for each privilege level (machine or supervisor). The configuration of
> > > > +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
> > > > +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
> > > > +  fixed number of interrupt identities (to distinguish MSIs from devices)
> > > > +  which is same for given privilege level across CPUs (or HARTs).
> > > > +
> > > > +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
> > > > +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
> > > > +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
> > > > +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
> > > > +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
> > > > +  privilege level (machine or supervisor) encodes group index, HART index,
> > > > +  and guest index (shown below).
> > > > +
> > > > +  XLEN-1           >=24                                 12    0
> > > > +  |                  |                                  |     |
> > > > +  -------------------------------------------------------------
> > > > +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
> > > > +  -------------------------------------------------------------
> > > > +
> > > > +  The device tree of a RISC-V platform will have one IMSIC device tree node
> > > > +  for each privilege level (machine or supervisor) which collectively describe
> > > > +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
> > > > +
> > > > +allOf:
> > > > +  - $ref: /schemas/interrupt-controller.yaml#
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - enum:
> > > > +          - vendor,chip-imsics
> > >
> > > There is no such vendor... As Conor pointed out, this does not look
> > > correct. Compatibles must be real and specific.
> >
> > Previously, Rob had suggest to:
> > 1) Mandate two compatible strings: one for implementation and
> >     and second for specification
> > 2) Since this is new specification with QEMU being the only
> >     implementation, we add "vendor,chip-imsics" as dummy
> >     implementation specific string for DT schema checkers
> >     to pass the examples. Once we have an actual implementation,
> >    we will replace this dummy string.
>
> What will QEMU's DT use? That's an implementation we can and do run
> validation on. Your choices are define a QEMU specific compatible string
> or allow the fallback alone. I'm fine either way. With the latter,
> someone has to review that the fallback is not used alone in .dts files
> while doing the former allows the tools to check for you. It also
> encourages making every new difference a property rather than implied by
> compatible, but those should be caught in review.
>
> If you go with the fallback only, just make it clear that it's for QEMU
> or s/w models only.

Sure, I will add  "riscv,qemu-imsics" as QEMU specific compatible string.

Regards,
Anup