2023-01-03 14:29:40

by Anup Patel

[permalink] [raw]
Subject: [PATCH v2 4/9] dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

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

Signed-off-by: Anup Patel <[email protected]>
---
.../interrupt-controller/riscv,imsics.yaml | 168 ++++++++++++++++++
1 file changed, 168 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
new file mode 100644
index 000000000000..b9db03b6e95f
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
@@ -0,0 +1,168 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V Incoming MSI Controller (IMSIC)
+
+maintainers:
+ - Anup Patel <[email protected]>
+
+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 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).
+
+ 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 |
+ -------------------------------------------------------------
+
+allOf:
+ - $ref: /schemas/interrupt-controller.yaml#
+ - $ref: /schemas/interrupt-controller/msi-controller.yaml#
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - riscv,qemu-imsics
+ - const: riscv,imsics
+
+ reg:
+ minItems: 1
+ maxItems: 16384
+ description:
+ Base address of each IMSIC group.
+
+ interrupt-controller: true
+
+ "#interrupt-cells":
+ const: 0
+
+ msi-controller: true
+
+ interrupts-extended:
+ minItems: 1
+ maxItems: 16384
+ 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:
+ Number of interrupt identities supported by IMSIC interrupt file.
+
+ riscv,num-guest-ids:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ minimum: 63
+ maximum: 2047
+ description:
+ Number of interrupt identities are supported by IMSIC guest interrupt
+ file. When not specified it is assumed to be same as specified by the
+ riscv,num-ids property.
+
+ riscv,guest-index-bits:
+ minimum: 0
+ maximum: 7
+ default: 0
+ description:
+ 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:
+ 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
+ default: 0
+ description:
+ 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: 0
+ maximum: 55
+ default: 24
+ description:
+ The least significant bit position of the group index bits in the
+ MSI target address. When not specified it is assumed to be 24.
+
+required:
+ - compatible
+ - reg
+ - interrupt-controller
+ - msi-controller
+ - interrupts-extended
+ - riscv,num-ids
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ // Example 1 (Machine-level IMSIC files with just one group):
+
+ imsic_mlevel: interrupt-controller@24000000 {
+ compatible = "riscv,qemu-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 = "riscv,qemu-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


2023-01-05 00:22:37

by Conor Dooley

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

Hey Anup,

On Tue, Jan 03, 2023 at 07:44:04PM +0530, Anup Patel wrote:
> We add DT bindings document for the RISC-V incoming MSI controller
> (IMSIC) defined by the RISC-V advanced interrupt architecture (AIA)
> specification.
>
> Signed-off-by: Anup Patel <[email protected]>
> + The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> + for each privilege level (machine or supervisor).

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

> +examples:
> + - |
> + // Example 1 (Machine-level IMSIC files with just one group):
> +
> + imsic_mlevel: interrupt-controller@24000000 {
> + compatible = "riscv,qemu-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 = "riscv,qemu-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>;
> + };

How is, say linux, meant to know which of the per-level imsic DT nodes
applies to it?
I had a quick look in the driver, but could see no mechanism for it.
Apologies if I missed something obvious!

Thanks,
Conor.


Attachments:
(No filename) (2.15 kB)
signature.asc (235.00 B)
Download all attachments

2023-01-12 21:50:26

by Rob Herring

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

On Tue, Jan 03, 2023 at 07:44:04PM +0530, Anup Patel wrote:
> We add DT bindings document for the RISC-V incoming MSI controller
> (IMSIC) defined by the RISC-V advanced interrupt architecture (AIA)
> specification.
>
> Signed-off-by: Anup Patel <[email protected]>
> ---
> .../interrupt-controller/riscv,imsics.yaml | 168 ++++++++++++++++++
> 1 file changed, 168 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> new file mode 100644
> index 000000000000..b9db03b6e95f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> @@ -0,0 +1,168 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RISC-V Incoming MSI Controller (IMSIC)
> +
> +maintainers:
> + - Anup Patel <[email protected]>
> +
> +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 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).
> +
> + 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 |
> + -------------------------------------------------------------
> +
> +allOf:
> + - $ref: /schemas/interrupt-controller.yaml#
> + - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - riscv,qemu-imsics

The implmentation/vendor is qemu, so: qemu,imsics (or qemu,riscv-imsics?)

> + - const: riscv,imsics
> +
> + reg:
> + minItems: 1
> + maxItems: 16384
> + description:
> + Base address of each IMSIC group.
> +
> + interrupt-controller: true
> +
> + "#interrupt-cells":
> + const: 0
> +
> + msi-controller: true
> +
> + interrupts-extended:
> + minItems: 1
> + maxItems: 16384
> + 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:
> + Number of interrupt identities supported by IMSIC interrupt file.
> +
> + riscv,num-guest-ids:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 63
> + maximum: 2047
> + description:
> + Number of interrupt identities are supported by IMSIC guest interrupt
> + file. When not specified it is assumed to be same as specified by the
> + riscv,num-ids property.
> +
> + riscv,guest-index-bits:
> + minimum: 0
> + maximum: 7
> + default: 0
> + description:
> + Number of guest index bits in the MSI target address. When not
> + specified it is assumed to be 0.

No need to repeat what 'default: 0' defines.

> +
> + riscv,hart-index-bits:
> + minimum: 0
> + maximum: 15
> + description:
> + Number of HART index bits in the MSI target address. When not
> + specified it is estimated based on the interrupts-extended property.

If guessing works, why do you need the property? Perhaps
s/estimated/calculated/?

> +
> + riscv,group-index-bits:
> + minimum: 0
> + maximum: 7
> + default: 0
> + description:
> + 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: 0
> + maximum: 55
> + default: 24
> + description:
> + The least significant bit position of the group index bits in the
> + MSI target address. When not specified it is assumed to be 24.
> +
> +required:
> + - compatible
> + - reg
> + - interrupt-controller
> + - msi-controller

#msi-cells should be defined (as 0) and required. Best to be explicit
#and not rely on the default.

> + - interrupts-extended
> + - riscv,num-ids
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + // Example 1 (Machine-level IMSIC files with just one group):
> +
> + imsic_mlevel: interrupt-controller@24000000 {

Drop unused labels.

> + compatible = "riscv,qemu-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 = "riscv,qemu-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
>

2023-02-19 11:26:58

by Vivian Wang

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

On 1/3/23 22:14, Anup Patel wrote:
> We add DT bindings document for the RISC-V incoming MSI controller
> (IMSIC) defined by the RISC-V advanced interrupt architecture (AIA)
> specification.
>
> Signed-off-by: Anup Patel <[email protected]>
> ---
> .../interrupt-controller/riscv,imsics.yaml | 168 ++++++++++++++++++
> 1 file changed, 168 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> new file mode 100644
> index 000000000000..b9db03b6e95f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> <snip>
> +
> + interrupts-extended:
> + minItems: 1
> + maxItems: 16384
> + 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.
> +

This property doesn't seem to describe guest external interrupts. Should
we add a reference to e.g. <&cpuN_intc 12> to indicate that IMSIC can
send a 'Supervisor guest external interrupt'? Or just an idea, maybe we
can add an additional interrupt controller to the CPU nodes to handle
SGEI: (Various properties omitted)

cpu0: cpu@N {
compatible = "riscv";

cpu0_intc: interrupt-controller {
compatible = "riscv,cpu-intc";

cpu0_gei: interrupt-controller {
/* intc for hart-local hgeie/hgeip */
compatible = "riscv,..."; /* Something here */
interrupt-parent = <&cpu0_intc>;
interrupts = <12>; /* SGEI */
interrupt-controller;
#interrupt-cells = <1>;
}
}
}

interrupt-controller@... {
compatible = "riscv,imsics";
interrupts-extended = <&cpu0_intc 11>, <&cpu0_gei 1>, <&cpu0_gei 2> /* ... */;
}

I feel that this would be more appropriate, since the guest external
interrupts are defined in the privileged architecture specification and
are not specific to AIA. Though please do suggest more appropriate ways
to formulate it.
> <snip>
> +...

2023-02-20 03:15:45

by Anup Patel

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

On Thu, Jan 5, 2023 at 4:51 AM Conor Dooley <[email protected]> wrote:
>
> Hey Anup,
>
> On Tue, Jan 03, 2023 at 07:44:04PM +0530, Anup Patel wrote:
> > We add DT bindings document for the RISC-V incoming MSI controller
> > (IMSIC) defined by the RISC-V advanced interrupt architecture (AIA)
> > specification.
> >
> > Signed-off-by: Anup Patel <[email protected]>
> > + The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> > + for each privilege level (machine or supervisor).
>
> > + 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).
>
> > +examples:
> > + - |
> > + // Example 1 (Machine-level IMSIC files with just one group):
> > +
> > + imsic_mlevel: interrupt-controller@24000000 {
> > + compatible = "riscv,qemu-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 = "riscv,qemu-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>;
> > + };
>
> How is, say linux, meant to know which of the per-level imsic DT nodes
> applies to it?
> I had a quick look in the driver, but could see no mechanism for it.
> Apologies if I missed something obvious!

This is very straightforward. We simply look at the local interrupt number
in the "interrupts-extended" DT property.

Currently, we use the same technique in PLIC driver to distinguish
M-mode PLIC context from S-mode PLIC context.

Regards,
Anup

2023-02-20 03:20:47

by Anup Patel

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

On Fri, Jan 13, 2023 at 2:19 AM Rob Herring <[email protected]> wrote:
>
> On Tue, Jan 03, 2023 at 07:44:04PM +0530, Anup Patel wrote:
> > We add DT bindings document for the RISC-V incoming MSI controller
> > (IMSIC) defined by the RISC-V advanced interrupt architecture (AIA)
> > specification.
> >
> > Signed-off-by: Anup Patel <[email protected]>
> > ---
> > .../interrupt-controller/riscv,imsics.yaml | 168 ++++++++++++++++++
> > 1 file changed, 168 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> > new file mode 100644
> > index 000000000000..b9db03b6e95f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> > @@ -0,0 +1,168 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsics.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RISC-V Incoming MSI Controller (IMSIC)
> > +
> > +maintainers:
> > + - Anup Patel <[email protected]>
> > +
> > +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 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).
> > +
> > + 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 |
> > + -------------------------------------------------------------
> > +
> > +allOf:
> > + - $ref: /schemas/interrupt-controller.yaml#
> > + - $ref: /schemas/interrupt-controller/msi-controller.yaml#
> > +
> > +properties:
> > + compatible:
> > + items:
> > + - enum:
> > + - riscv,qemu-imsics
>
> The implmentation/vendor is qemu, so: qemu,imsics (or qemu,riscv-imsics?)

Okay, I will update.

>
> > + - const: riscv,imsics
> > +
> > + reg:
> > + minItems: 1
> > + maxItems: 16384
> > + description:
> > + Base address of each IMSIC group.
> > +
> > + interrupt-controller: true
> > +
> > + "#interrupt-cells":
> > + const: 0
> > +
> > + msi-controller: true
> > +
> > + interrupts-extended:
> > + minItems: 1
> > + maxItems: 16384
> > + 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:
> > + Number of interrupt identities supported by IMSIC interrupt file.
> > +
> > + riscv,num-guest-ids:
> > + $ref: /schemas/types.yaml#/definitions/uint32
> > + minimum: 63
> > + maximum: 2047
> > + description:
> > + Number of interrupt identities are supported by IMSIC guest interrupt
> > + file. When not specified it is assumed to be same as specified by the
> > + riscv,num-ids property.
> > +
> > + riscv,guest-index-bits:
> > + minimum: 0
> > + maximum: 7
> > + default: 0
> > + description:
> > + Number of guest index bits in the MSI target address. When not
> > + specified it is assumed to be 0.
>
> No need to repeat what 'default: 0' defines.

Okay, I will update.

>
> > +
> > + riscv,hart-index-bits:
> > + minimum: 0
> > + maximum: 15
> > + description:
> > + Number of HART index bits in the MSI target address. When not
> > + specified it is estimated based on the interrupts-extended property.
>
> If guessing works, why do you need the property? Perhaps
> s/estimated/calculated/?

Okay, I will fix the wording.

We need this property because IMSIC files of two consecutive HARTs
are not required to be contiguous since there could be holes (unused
space) in between.

>
> > +
> > + riscv,group-index-bits:
> > + minimum: 0
> > + maximum: 7
> > + default: 0
> > + description:
> > + 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: 0
> > + maximum: 55
> > + default: 24
> > + description:
> > + The least significant bit position of the group index bits in the
> > + MSI target address. When not specified it is assumed to be 24.
> > +
> > +required:
> > + - compatible
> > + - reg
> > + - interrupt-controller
> > + - msi-controller
>
> #msi-cells should be defined (as 0) and required. Best to be explicit
> #and not rely on the default.

Okay, I will update.

>
> > + - interrupts-extended
> > + - riscv,num-ids
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > + - |
> > + // Example 1 (Machine-level IMSIC files with just one group):
> > +
> > + imsic_mlevel: interrupt-controller@24000000 {
>
> Drop unused labels.

Okay, I will update.

>
> > + compatible = "riscv,qemu-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 = "riscv,qemu-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
> >

Regards,
Anup

2023-02-20 03:31:58

by Anup Patel

[permalink] [raw]
Subject: Re: [PATCH v2 4/9] dt-bindings: interrupt-controller: Add RISC-V incoming MSI controller

On Sun, Feb 19, 2023 at 4:48 PM Vivian Wang <[email protected]> wrote:
>
> On 1/3/23 22:14, Anup Patel wrote:
> > We add DT bindings document for the RISC-V incoming MSI controller
> > (IMSIC) defined by the RISC-V advanced interrupt architecture (AIA)
> > specification.
> >
> > Signed-off-by: Anup Patel <[email protected]>
> > ---
> > .../interrupt-controller/riscv,imsics.yaml | 168 ++++++++++++++++++
> > 1 file changed, 168 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> > new file mode 100644
> > index 000000000000..b9db03b6e95f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsics.yaml
> > <snip>
> > +
> > + interrupts-extended:
> > + minItems: 1
> > + maxItems: 16384
> > + 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.
> > +
>
> This property doesn't seem to describe guest external interrupts. Should
> we add a reference to e.g. <&cpuN_intc 12> to indicate that IMSIC can
> send a 'Supervisor guest external interrupt'? Or just an idea, maybe we
> can add an additional interrupt controller to the CPU nodes to handle
> SGEI: (Various properties omitted)
>
> cpu0: cpu@N {
> compatible = "riscv";
>
> cpu0_intc: interrupt-controller {
> compatible = "riscv,cpu-intc";
>
> cpu0_gei: interrupt-controller {
> /* intc for hart-local hgeie/hgeip */
> compatible = "riscv,..."; /* Something here */
> interrupt-parent = <&cpu0_intc>;
> interrupts = <12>; /* SGEI */
> interrupt-controller;
> #interrupt-cells = <1>;
> }
> }
> }
>
> interrupt-controller@... {
> compatible = "riscv,imsics";
> interrupts-extended = <&cpu0_intc 11>, <&cpu0_gei 1>, <&cpu0_gei 2> /* ... */;
> }
>
> I feel that this would be more appropriate, since the guest external
> interrupts are defined in the privileged architecture specification and
> are not specific to AIA. Though please do suggest more appropriate ways
> to formulate it.

This is unnecessary because GEILEN can be detected by init time
writes to hgeie CSR. Please look at KVM RISC-V AIA implementation
for more details. We only need "riscv,guest-index-bits" DT property
for address space holes.

In fact, we have tested these DT bindings with a variety of NUMA
configurations containing different numbers of IMISC guest files
per-HART.

Regards,
Anup