diff mbox series

[v3,08/11] dt-bindings: interrupt-controller: Add RISC-V advanced PLIC

Message ID 20230508142842.854564-9-apatel@ventanamicro.com (mailing list archive)
State Superseded
Delegated to: Palmer Dabbelt
Headers show
Series Linux RISC-V AIA Support | expand

Checks

Context Check Description
conchuod/tree_selection fail Failed to apply to next/pending-fixes or riscv/for-next

Commit Message

Anup Patel May 8, 2023, 2:28 p.m. UTC 
We add DT bindings document for RISC-V advanced platform level interrupt
controller (APLIC) defined by the RISC-V advanced interrupt architecture
(AIA) specification.

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

Comments

Conor Dooley May 10, 2023, 3:41 p.m. UTC | #1 
Hey Anup,

On Mon, May 08, 2023 at 07:58:39PM +0530, Anup Patel wrote:
> +  interrupts-extended:
> +    minItems: 1
> +    maxItems: 16384
> +    description:
> +      Given APLIC domain directly injects external interrupts to a set of
> +      RISC-V HARTS (or CPUs). Each node pointed to should be a riscv,cpu-intc
> +      node, which has a riscv node (i.e. RISC-V HART) as parent.

Same nit here, s/riscv node/cpu node/?

> +
> +  msi-parent:
> +    description:
> +      Given APLIC domain forwards wired interrupts as MSIs to a AIA incoming
> +      message signaled interrupt controller (IMSIC). If both "msi-parent" and
> +      "interrupts-extended" properties are present then it means the APLIC
> +      domain supports both MSI mode and Direct mode in HW. In this case, the
> +      APLIC driver has to choose between MSI mode or Direct mode.
> +
> +  riscv,num-sources:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 1023
> +    description:
> +      Specifies the number of wired interrupt sources supported by this
> +      APLIC domain.

Rob asked:
| We don't normally need to how many interrupts, why here?

But I do not see an answer to that on lore.

> +
> +  riscv,children:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 1
> +    maxItems: 1024
> +    items:
> +      maxItems: 1
> +    description:
> +      A list of child APLIC domains for the given APLIC domain. Each child
> +      APLIC domain is assigned a child index in increasing order, with the
> +      first child APLIC domain assigned child index 0. The APLIC domain child
> +      index is used by firmware to delegate interrupts from the given APLIC
> +      domain to a particular child APLIC domain.
> +
> +  riscv,delegate:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 1
> +    maxItems: 1024
> +    items:
> +      items:
> +        - description: child APLIC domain phandle
> +        - description: first interrupt number of this APLIC domain (inclusive)
> +        - description: last interrupt number of this APLIC domain (inclusive)
> +    description:
> +      A interrupt delegation list where each entry is a triple consisting of
> +      child APLIC domain phandle, first interrupt number of this APLIC domain,
> +      and last interrupt number of this APLIC domain. Firmware must configure
> +      interrupt delegation registers based on interrupt delegation list.

I read back Rob's comments on this from last time. He said:
| The node's domain it delegating its interrupts to the child domain or 
| the other way around? The interrupt numbers here are this domain's or 
| the child's?

IMO it's ambiguous from the binding description whether the numbers
refer to the "first interrupt in the parent domain that is delegated
to the child" or to numbering of the interrupts within the child domain.
"This" can be quite an ambiguous word, and it's not totally obvious
whether the "this" refers to the "child domain" earlier in the sentence.

I also do not not think you have answered his question about the
directionality of the delegation either (it should just be a copy-paste
from riscv,children, no?).

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +  - riscv,num-sources

btw, do we need something like:

anyOf:
  - required:
      - interrupts-extended
  - required:
      - msi-parent

& hopefully I didn't previously ask this one:
dependencies:
  riscv,delegate: [ riscv,children ]

As an aside, I still think "riscv,delegate" looks strange here alongside
"riscv,children" since "delegate" is singular and "children" is plural.
The plural would be "delegates", but "delegation" would also fit better
than "delegate".

Cheers,
Conor.
Anup Patel June 13, 2023, 10:37 a.m. UTC | #2 
On Wed, May 10, 2023 at 9:11 PM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Anup,
>
> On Mon, May 08, 2023 at 07:58:39PM +0530, Anup Patel wrote:
> > +  interrupts-extended:
> > +    minItems: 1
> > +    maxItems: 16384
> > +    description:
> > +      Given APLIC domain directly injects external interrupts to a set of
> > +      RISC-V HARTS (or CPUs). Each node pointed to should be a riscv,cpu-intc
> > +      node, which has a riscv node (i.e. RISC-V HART) as parent.
>
> Same nit here, s/riscv node/cpu node/?

Okay, I will update.

>
> > +
> > +  msi-parent:
> > +    description:
> > +      Given APLIC domain forwards wired interrupts as MSIs to a AIA incoming
> > +      message signaled interrupt controller (IMSIC). If both "msi-parent" and
> > +      "interrupts-extended" properties are present then it means the APLIC
> > +      domain supports both MSI mode and Direct mode in HW. In this case, the
> > +      APLIC driver has to choose between MSI mode or Direct mode.
> > +
> > +  riscv,num-sources:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 1
> > +    maximum: 1023
> > +    description:
> > +      Specifies the number of wired interrupt sources supported by this
> > +      APLIC domain.
>
> Rob asked:
> | We don't normally need to how many interrupts, why here?
>
> But I do not see an answer to that on lore.

The APLIC spec defines maximum interrupt sources to be 1023.

>
> > +
> > +  riscv,children:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    minItems: 1
> > +    maxItems: 1024
> > +    items:
> > +      maxItems: 1
> > +    description:
> > +      A list of child APLIC domains for the given APLIC domain. Each child
> > +      APLIC domain is assigned a child index in increasing order, with the
> > +      first child APLIC domain assigned child index 0. The APLIC domain child
> > +      index is used by firmware to delegate interrupts from the given APLIC
> > +      domain to a particular child APLIC domain.
> > +
> > +  riscv,delegate:
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    minItems: 1
> > +    maxItems: 1024
> > +    items:
> > +      items:
> > +        - description: child APLIC domain phandle
> > +        - description: first interrupt number of this APLIC domain (inclusive)
> > +        - description: last interrupt number of this APLIC domain (inclusive)
> > +    description:
> > +      A interrupt delegation list where each entry is a triple consisting of
> > +      child APLIC domain phandle, first interrupt number of this APLIC domain,
> > +      and last interrupt number of this APLIC domain. Firmware must configure
> > +      interrupt delegation registers based on interrupt delegation list.
>
> I read back Rob's comments on this from last time. He said:
> | The node's domain it delegating its interrupts to the child domain or
> | the other way around? The interrupt numbers here are this domain's or
> | the child's?

The node's domain is delegating its interrupts to the child domain.

>
> IMO it's ambiguous from the binding description whether the numbers
> refer to the "first interrupt in the parent domain that is delegated
> to the child" or to numbering of the interrupts within the child domain.
> "This" can be quite an ambiguous word, and it's not totally obvious
> whether the "this" refers to the "child domain" earlier in the sentence.
>
> I also do not not think you have answered his question about the
> directionality of the delegation either (it should just be a copy-paste
> from riscv,children, no?).

For APLIC, the interrupt delegation is always from parent domain
to the child domain.

I will add a statement about this in the description.

>
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupt-controller
> > +  - "#interrupt-cells"
> > +  - riscv,num-sources
>
> btw, do we need something like:
>
> anyOf:
>   - required:
>       - interrupts-extended
>   - required:
>       - msi-parent

Okay, I will update.

>
> & hopefully I didn't previously ask this one:
> dependencies:
>   riscv,delegate: [ riscv,children ]
>
> As an aside, I still think "riscv,delegate" looks strange here alongside
> "riscv,children" since "delegate" is singular and "children" is plural.
> The plural would be "delegates", but "delegation" would also fit better
> than "delegate".

Okay, I will rename it to "riscv,delegation".

>
> Cheers,
> Conor.

Regards,
Anup
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
new file mode 100644
index 000000000000..b47a7987b774
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
@@ -0,0 +1,162 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/riscv,aplic.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V Advanced Platform Level Interrupt Controller (APLIC)
+
+maintainers:
+  - Anup Patel <anup@brainfault.org>
+
+description:
+  The RISC-V advanced interrupt architecture (AIA) defines an advanced
+  platform level interrupt controller (APLIC) for handling wired interrupts
+  in a RISC-V platform. The RISC-V AIA specification can be found at
+  https://github.com/riscv/riscv-aia.
+
+  The RISC-V APLIC is implemented as hierarchical APLIC domains where all
+  interrupt sources connect to the root domain which can further delegate
+  interrupts to child domains. There is one device tree node for each APLIC
+  domain.
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qemu,aplic
+      - const: riscv,aplic
+
+  reg:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts-extended:
+    minItems: 1
+    maxItems: 16384
+    description:
+      Given APLIC domain directly injects external interrupts to a set of
+      RISC-V HARTS (or CPUs). Each node pointed to should be a riscv,cpu-intc
+      node, which has a riscv node (i.e. RISC-V HART) as parent.
+
+  msi-parent:
+    description:
+      Given APLIC domain forwards wired interrupts as MSIs to a AIA incoming
+      message signaled interrupt controller (IMSIC). If both "msi-parent" and
+      "interrupts-extended" properties are present then it means the APLIC
+      domain supports both MSI mode and Direct mode in HW. In this case, the
+      APLIC driver has to choose between MSI mode or Direct mode.
+
+  riscv,num-sources:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 1023
+    description:
+      Specifies the number of wired interrupt sources supported by this
+      APLIC domain.
+
+  riscv,children:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 1
+    maxItems: 1024
+    items:
+      maxItems: 1
+    description:
+      A list of child APLIC domains for the given APLIC domain. Each child
+      APLIC domain is assigned a child index in increasing order, with the
+      first child APLIC domain assigned child index 0. The APLIC domain child
+      index is used by firmware to delegate interrupts from the given APLIC
+      domain to a particular child APLIC domain.
+
+  riscv,delegate:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 1
+    maxItems: 1024
+    items:
+      items:
+        - description: child APLIC domain phandle
+        - description: first interrupt number of this APLIC domain (inclusive)
+        - description: last interrupt number of this APLIC domain (inclusive)
+    description:
+      A interrupt delegation list where each entry is a triple consisting of
+      child APLIC domain phandle, first interrupt number of this APLIC domain,
+      and last interrupt number of this APLIC domain. Firmware must configure
+      interrupt delegation registers based on interrupt delegation list.
+
+required:
+  - compatible
+  - reg
+  - interrupt-controller
+  - "#interrupt-cells"
+  - riscv,num-sources
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    // Example 1 (APLIC domains directly injecting interrupt to HARTs):
+
+    interrupt-controller@c000000 {
+      compatible = "qemu,aplic", "riscv,aplic";
+      interrupts-extended = <&cpu1_intc 11>,
+                            <&cpu2_intc 11>,
+                            <&cpu3_intc 11>,
+                            <&cpu4_intc 11>;
+      reg = <0xc000000 0x4080>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+      riscv,children = <&aplic1>, <&aplic2>;
+      riscv,delegate = <&aplic1 1 63>;
+    };
+
+    aplic1: interrupt-controller@d000000 {
+      compatible = "qemu,aplic", "riscv,aplic";
+      interrupts-extended = <&cpu1_intc 9>,
+                            <&cpu2_intc 9>;
+      reg = <0xd000000 0x4080>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+    };
+
+    aplic2: interrupt-controller@e000000 {
+      compatible = "qemu,aplic", "riscv,aplic";
+      interrupts-extended = <&cpu3_intc 9>,
+                            <&cpu4_intc 9>;
+      reg = <0xe000000 0x4080>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+    };
+
+  - |
+    // Example 2 (APLIC domains forwarding interrupts as MSIs):
+
+    interrupt-controller@c000000 {
+      compatible = "qemu,aplic", "riscv,aplic";
+      msi-parent = <&imsic_mlevel>;
+      reg = <0xc000000 0x4000>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+      riscv,children = <&aplic3>;
+      riscv,delegate = <&aplic3 1 63>;
+    };
+
+    aplic3: interrupt-controller@d000000 {
+      compatible = "qemu,aplic", "riscv,aplic";
+      msi-parent = <&imsic_slevel>;
+      reg = <0xd000000 0x4000>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+    };
+...