diff mbox series

[v2,6/9] dt-bindings: interrupt-controller: Add RISC-V advanced PLIC

Message ID 20230103141409.772298-7-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 Jan. 3, 2023, 2:14 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     | 159 ++++++++++++++++++
 1 file changed, 159 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml

Comments

Conor Dooley Jan. 4, 2023, 10:16 p.m. UTC | #1
Hey Anup,

On Tue, Jan 03, 2023 at 07:44:06PM +0530, Anup Patel wrote:
> 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     | 159 ++++++++++++++++++
>  1 file changed, 159 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml

> +  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). This property should be
> +      considered only when the interrupts-extended property is absent.

Considered by what?
On v1 you said:
<quote>
If both "interrupts-extended" and "msi-parent" 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.
<\quote>

The description is still pretty ambiguous IMO. Perhaps incorporate
some of that expanded comment into the property description?
Say, "If both foo and bar are present, the APLIC domain has hardware
support for both MSI and direct mode. Software may then chose either
mode".
Have I misunderstood your comment on v1? It read as if having both
present indicated that both were possible & that "should be considered
only..." was more of a suggestion and a comment about the Linux driver's
behaviour.
Apologies if I have misunderstood, but I suppose if I have then the
binding's description could be improved!!

> +  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 child index in increasing order with the

btw, missing article before child (& a comma after order I think).

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

Is it valid to have a delegate property without children? If not, the
binding should reflect that dependency IMO.

> +    items:
> +      items:
> +        - description: child APLIC domain phandle
> +        - description: first interrupt number (inclusive)
> +        - description: last interrupt number (inclusive)
> +    description:
> +      A interrupt delegation list where each entry is a triple consisting
> +      of child APLIC domain phandle, first interrupt number, and last
> +      interrupt number. The firmware will configure interrupt delegation

btw, drop the article before firmware here.
Also, "firmware will" or "firmware must"? Semantics perhaps, but they
are different!

Kinda for my own curiosity here, but do you expect these properties to
generally be dynamically filled in by the bootloader or read by the
bootloader to set up the configuration?

> +      registers based on interrupt delegation list.

I'm sorry Anup, but this child versus delegate thing is still not clear
to me binding wise. See below.

> +    aplic0: interrupt-controller@c000000 {
> +      compatible = "riscv,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>;

Is aplic2 here for demonstrative purposes only, since it has not been
delegated any interrupts?
I suppose it is hardware present on the SoC that is not being used by
the current configuration?

Thanks,
Conor.

> +    };
> +
> +    aplic1: interrupt-controller@d000000 {
> +      compatible = "riscv,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 = "riscv,qemu-aplic", "riscv,aplic";
> +      interrupts-extended = <&cpu3_intc 9>,
> +                            <&cpu4_intc 9>;
> +      reg = <0xe000000 0x4080>;
> +      interrupt-controller;
> +      #interrupt-cells = <2>;
> +      riscv,num-sources = <63>;
> +    };
Rob Herring Jan. 12, 2023, 9:02 p.m. UTC | #2
On Tue, Jan 03, 2023 at 07:44:06PM +0530, Anup Patel wrote:
> 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     | 159 ++++++++++++++++++
>  1 file changed, 159 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
> 
> 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..b7f20aad72c2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
> @@ -0,0 +1,159 @@
> +# 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:
> +          - riscv,qemu-aplic

Make 'qemu' the vendor.

> +      - 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). This property should be
> +      considered only when the interrupts-extended property is absent.
> +
> +  riscv,num-sources:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 1023
> +    description:
> +      Specifies how many wired interrupts are supported by this APLIC domain.

We don't normally need to how many interrupts, why here?

> +
> +  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 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 (inclusive)
> +        - description: last interrupt number (inclusive)
> +    description:
> +      A interrupt delegation list where each entry is a triple consisting
> +      of child APLIC domain phandle, first interrupt number, and last
> +      interrupt number. The firmware will configure interrupt delegation
> +      registers based on interrupt delegation list.

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?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupt-controller
> +  - "#interrupt-cells"
> +  - riscv,num-sources
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    // Example 1 (APLIC domains directly injecting interrupt to HARTs):
> +
> +    aplic0: interrupt-controller@c000000 {
> +      compatible = "riscv,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 = "riscv,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 = "riscv,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):
> +
> +    aplic3: interrupt-controller@c000000 {
> +      compatible = "riscv,qemu-aplic", "riscv,aplic";
> +      msi-parent = <&imsic_mlevel>;
> +      reg = <0xc000000 0x4000>;
> +      interrupt-controller;
> +      #interrupt-cells = <2>;
> +      riscv,num-sources = <63>;
> +      riscv,children = <&aplic4>;
> +      riscv,delegate = <&aplic4 1 63>;
> +    };
> +
> +    aplic4: interrupt-controller@d000000 {
> +      compatible = "riscv,qemu-aplic", "riscv,aplic";
> +      msi-parent = <&imsic_slevel>;
> +      reg = <0xd000000 0x4000>;
> +      interrupt-controller;
> +      #interrupt-cells = <2>;
> +      riscv,num-sources = <63>;
> +    };
> +...
> -- 
> 2.34.1
>
Vivian Wang Feb. 19, 2023, 11:48 a.m. UTC | #3
On 1/3/23 22:14, Anup Patel wrote:
> 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     | 159 ++++++++++++++++++
>  1 file changed, 159 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
>
> 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..b7f20aad72c2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
> @@ -0,0 +1,159 @@
>
> <snip>
>
> +  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 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 (inclusive)
> +        - description: last interrupt number (inclusive)
> +    description:
> +      A interrupt delegation list where each entry is a triple consisting
> +      of child APLIC domain phandle, first interrupt number, and last
> +      interrupt number. The firmware will configure interrupt delegation
> +      registers based on interrupt delegation list.
> +

I'm not sure if this is the right place to ask, since it could be more
of a OpenSBI/QEMU problem, but I think a more detailed description about
what 'the firmware' does is appropriate here.

My main confusion is how to describe wired interrupts connected to
APLICs. Say we have two APLIC nodes with labels aplic_m and aplic_s that
are the APLIC domains for M-mode and S-mode respectively. IIUC, wired
interrupts are connected directly to aplic_m. So how do I refer to it in
the device nodes?

 1. <&aplic_s num IRQ_TYPE_foo>, but it would be a lie to M-mode
    software, which could be a problem. QEMU 7.2.0 seems to take this
    approach. (I could also be misunderstanding QEMU and it actually
    does connect wired interrupts to the S-mode APLIC, but then
    riscv,children and riscv,delegate would be lies.)
 2. <&aplic_m ...>, and when M-mode software gives S-mode software
    access to devices, it delegates relevant interrupts and patches it
    into <&aplic_s num IRQ_TYPE_foo>. Seems to be the 'correct'
    approach, but pretty complicated.
 3. <&aplic_m ...>, S-mode software sees this, and sees that aplic_m has
    num in riscv,delegate, so goes to find the child it's been delegated
    to, which is (should be) aplic_s. A bit annoyingly abstraction
    breaking, since S-mode shouldn't even need to know about aplic_m.

I see that others are also confused by riscv,delegate and riscv,children
properties. It would be great if we could clarify the expected behavior
here rather than just saying 'the firmware will do the thing'.

> <snip>
> +...
Thanks,
Vivian
Anup Patel Feb. 20, 2023, 4:36 a.m. UTC | #4
On Thu, Jan 5, 2023 at 3:47 AM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Anup,
>
> On Tue, Jan 03, 2023 at 07:44:06PM +0530, Anup Patel wrote:
> > 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     | 159 ++++++++++++++++++
> >  1 file changed, 159 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
>
> > +  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). This property should be
> > +      considered only when the interrupts-extended property is absent.
>
> Considered by what?
> On v1 you said:
> <quote>
> If both "interrupts-extended" and "msi-parent" 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.
> <\quote>
>
> The description is still pretty ambiguous IMO. Perhaps incorporate
> some of that expanded comment into the property description?
> Say, "If both foo and bar are present, the APLIC domain has hardware
> support for both MSI and direct mode. Software may then chose either
> mode".
> Have I misunderstood your comment on v1? It read as if having both
> present indicated that both were possible & that "should be considered
> only..." was more of a suggestion and a comment about the Linux driver's
> behaviour.
> Apologies if I have misunderstood, but I suppose if I have then the
> binding's description could be improved!!

Yes, when both DT properties are present then it's up to Linux
APLIC driver to choose the appropriate APLIC mode.

I forgot to update the text here in v2 but I will update it in v3.
Thanks for pointing.

>
> > +  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 child index in increasing order with the
>
> btw, missing article before child (& a comma after order I think).

Okay, I will update.

>
> > +      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
>
> Is it valid to have a delegate property without children? If not, the
> binding should reflect that dependency IMO.

Okay, I will update.

>
> > +    items:
> > +      items:
> > +        - description: child APLIC domain phandle
> > +        - description: first interrupt number (inclusive)
> > +        - description: last interrupt number (inclusive)
> > +    description:
> > +      A interrupt delegation list where each entry is a triple consisting
> > +      of child APLIC domain phandle, first interrupt number, and last
> > +      interrupt number. The firmware will configure interrupt delegation
>
> btw, drop the article before firmware here.
> Also, "firmware will" or "firmware must"? Semantics perhaps, but they
> are different!

I think "firmware must" is better because APLIC M-mode domains are
not accessible to S-mode so firmware must configure delegation for
at least all APLIC M-mode domains.

>
> Kinda for my own curiosity here, but do you expect these properties to
> generally be dynamically filled in by the bootloader or read by the
> bootloader to set up the configuration?

Firmware (or bootloader) will look at this property and setup delegation
before booting the OS kernel.

>
> > +      registers based on interrupt delegation list.
>
> I'm sorry Anup, but this child versus delegate thing is still not clear
> to me binding wise. See below.

There are two different information in-context of APLIC domain:

1) HW child domain numbering: If an APLIC domain has N children
    then HW will have a fixed child index for each of the N children
    in the range 0 to N-1. This HW child index is required at the time
    of setting up interrupt delegation in sourcecfgX registers. The
    "riscv,children" DT property helps firmware (or bootloader) find
    the total number of child APLIC domains and corresponding
    HW child index number.

2) IRQ delegation to child domains: An APLIC domain can delegate
   any IRQ range(s) to a particular APLIC child domain. The
   "riscv,delegate" DT property is simply a table where we have
   one row for each IRQ range which is delegated to some child
   APLIC domain. This property is more of a system setting fixed
   by the RISC-V platform vendor.

>
> > +    aplic0: interrupt-controller@c000000 {
> > +      compatible = "riscv,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>;
>
> Is aplic2 here for demonstrative purposes only, since it has not been
> delegated any interrupts?

Yes, it's for demonstrative purposes only.

> I suppose it is hardware present on the SoC that is not being used by
> the current configuration?

Yes, in this example aplic2 is unused because it has no interrupts
delegated to it.

>
> Thanks,
> Conor.
>
> > +    };
> > +
> > +    aplic1: interrupt-controller@d000000 {
> > +      compatible = "riscv,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 = "riscv,qemu-aplic", "riscv,aplic";
> > +      interrupts-extended = <&cpu3_intc 9>,
> > +                            <&cpu4_intc 9>;
> > +      reg = <0xe000000 0x4080>;
> > +      interrupt-controller;
> > +      #interrupt-cells = <2>;
> > +      riscv,num-sources = <63>;
> > +    };
>

Regards,
Anup
Anup Patel Feb. 20, 2023, 5:09 a.m. UTC | #5
On Sun, Feb 19, 2023 at 5:18 PM Vivian Wang <uwu@dram.page> wrote:
>
> On 1/3/23 22:14, Anup Patel wrote:
> > 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     | 159 ++++++++++++++++++
> >  1 file changed, 159 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
> >
> > 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..b7f20aad72c2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
> > @@ -0,0 +1,159 @@
> >
> > <snip>
> >
> > +  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 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 (inclusive)
> > +        - description: last interrupt number (inclusive)
> > +    description:
> > +      A interrupt delegation list where each entry is a triple consisting
> > +      of child APLIC domain phandle, first interrupt number, and last
> > +      interrupt number. The firmware will configure interrupt delegation
> > +      registers based on interrupt delegation list.
> > +
>
> I'm not sure if this is the right place to ask, since it could be more
> of a OpenSBI/QEMU problem, but I think a more detailed description about
> what 'the firmware' does is appropriate here.
>
> My main confusion is how to describe wired interrupts connected to
> APLICs. Say we have two APLIC nodes with labels aplic_m and aplic_s that
> are the APLIC domains for M-mode and S-mode respectively. IIUC, wired
> interrupts are connected directly to aplic_m. So how do I refer to it in
> the device nodes?

Please see my previous reply to Conor about these DT properties.
The riscv,children DT property describes HW child numbering whereas
the riscv,delegate DT propert is a table of IRQ delegation.

In your example, let's assume we have N wired interrupts. This
means we will have devices connected to the root APLIC domain
(aplic_m). Now since aplic_s is a child of aplic_m, we will have
N wired interrupts going from from aplic_m to aplic_s where
aplic_m will route a wired/device interrupt x to aplic_s if
sourcecfg[x].D = 1 and sourcecfg[x].child = 0.

>
>  1. <&aplic_s num IRQ_TYPE_foo>, but it would be a lie to M-mode
>     software, which could be a problem. QEMU 7.2.0 seems to take this
>     approach. (I could also be misunderstanding QEMU and it actually
>     does connect wired interrupts to the S-mode APLIC, but then
>     riscv,children and riscv,delegate would be lies.)

No, it's not a lie. The <&aplic_s num IRQ_TYPE_foo> in a device DT
node is based on the IRQ delegation fixed by the RISC-V platform.
QEMU has its own strategy of delegating IRQs to APLIC S-mode
while other platforms can use a different strategy.

>  2. <&aplic_m ...>, and when M-mode software gives S-mode software
>     access to devices, it delegates relevant interrupts and patches it
>     into <&aplic_s num IRQ_TYPE_foo>. Seems to be the 'correct'
>     approach, but pretty complicated.

The APLIC M-mode domain is not accessible to S-mode software so
Linux cannot create an irqdomain using APLIC M-mode DT node. This
means device DT nodes must have <&aplic_s num IRQ_TYPE_foo>
which points to APLIC S-mode domain.

It is totally up to RISC-V firmware and platform if it wants to dynamically
add/patch <&aplic_s num IRQ_TYPE_foo> in device DT nodes. Currently,
we do not patch device DT nodes in OpenSBI and instead have the
device DT nodes point to correct APLIC domain based on the IRQ
delegation.

>  3. <&aplic_m ...>, S-mode software sees this, and sees that aplic_m has
>     num in riscv,delegate, so goes to find the child it's been delegated
>     to, which is (should be) aplic_s. A bit annoyingly abstraction
>     breaking, since S-mode shouldn't even need to know about aplic_m.

Yes, S-mode should know about aplic_m and if it tries to access aplic_m
then it will get an access fault.

This is exactly why device DT node should have "interrupts" DT property
pointing to the actual APLIC domain which is delivering interrupt to S-mode.

>
> I see that others are also confused by riscv,delegate and riscv,children
> properties. It would be great if we could clarify the expected behavior
> here rather than just saying 'the firmware will do the thing'.

Regards,
Anup
Conor Dooley Feb. 20, 2023, 10:32 a.m. UTC | #6
Hey Anup,

On Mon, Feb 20, 2023 at 10:06:49AM +0530, Anup Patel wrote:
> On Thu, Jan 5, 2023 at 3:47 AM Conor Dooley <conor@kernel.org> wrote:
> > On Tue, Jan 03, 2023 at 07:44:06PM +0530, Anup Patel wrote:
> > > 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     | 159 ++++++++++++++++++
> > >  1 file changed, 159 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml

> > I'm sorry Anup, but this child versus delegate thing is still not clear
> > to me binding wise. See below.
> 
> There are two different information in-context of APLIC domain:
> 
> 1) HW child domain numbering: If an APLIC domain has N children
>     then HW will have a fixed child index for each of the N children
>     in the range 0 to N-1. This HW child index is required at the time
>     of setting up interrupt delegation in sourcecfgX registers. The
>     "riscv,children" DT property helps firmware (or bootloader) find
>     the total number of child APLIC domains and corresponding
>     HW child index number.
> 
> 2) IRQ delegation to child domains: An APLIC domain can delegate
>    any IRQ range(s) to a particular APLIC child domain. The
>    "riscv,delegate" DT property is simply a table where we have
>    one row for each IRQ range which is delegated to some child
>    APLIC domain. This property is more of a system setting fixed
>    by the RISC-V platform vendor.

Thanks for the explanations. It's been a while since my brain swapped
this stuff out, but I think delegate/child makes sense to me now.
Just don't ask me to write the dt entry as proof...

Thanks,
Conor.
Conor Dooley Feb. 20, 2023, 10:56 a.m. UTC | #7
On Mon, Feb 20, 2023 at 10:32:57AM +0000, Conor Dooley wrote:
> On Mon, Feb 20, 2023 at 10:06:49AM +0530, Anup Patel wrote:
> > On Thu, Jan 5, 2023 at 3:47 AM Conor Dooley <conor@kernel.org> wrote:
> > > On Tue, Jan 03, 2023 at 07:44:06PM +0530, Anup Patel wrote:
> > > > 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     | 159 ++++++++++++++++++
> > > >  1 file changed, 159 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
> 
> > > I'm sorry Anup, but this child versus delegate thing is still not clear
> > > to me binding wise. See below.
> > 
> > There are two different information in-context of APLIC domain:
> > 
> > 1) HW child domain numbering: If an APLIC domain has N children
> >     then HW will have a fixed child index for each of the N children
> >     in the range 0 to N-1. This HW child index is required at the time
> >     of setting up interrupt delegation in sourcecfgX registers. The
> >     "riscv,children" DT property helps firmware (or bootloader) find
> >     the total number of child APLIC domains and corresponding
> >     HW child index number.
> > 
> > 2) IRQ delegation to child domains: An APLIC domain can delegate
> >    any IRQ range(s) to a particular APLIC child domain. The
> >    "riscv,delegate" DT property is simply a table where we have
> >    one row for each IRQ range which is delegated to some child
> >    APLIC domain. This property is more of a system setting fixed
> >    by the RISC-V platform vendor.
> 
> Thanks for the explanations. It's been a while since my brain swapped
> this stuff out, but I think delegate/child makes sense to me now.

> Just don't ask me to write the dt entry as proof...

Having looked at Dramforever's QEMU dtb dump a bit more and your
responses to her, I think that I have "come to terms" with it now
actually.
I suppose when the next version comes around I'll make sure that I
arrive in the same ballpark that QEMU does, based off the descriptions
etc in the binding.

Thanks!
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..b7f20aad72c2
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,aplic.yaml
@@ -0,0 +1,159 @@ 
+# 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:
+          - riscv,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). This property should be
+      considered only when the interrupts-extended property is absent.
+
+  riscv,num-sources:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 1
+    maximum: 1023
+    description:
+      Specifies how many wired interrupts are 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 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 (inclusive)
+        - description: last interrupt number (inclusive)
+    description:
+      A interrupt delegation list where each entry is a triple consisting
+      of child APLIC domain phandle, first interrupt number, and last
+      interrupt number. The firmware will 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):
+
+    aplic0: interrupt-controller@c000000 {
+      compatible = "riscv,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 = "riscv,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 = "riscv,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):
+
+    aplic3: interrupt-controller@c000000 {
+      compatible = "riscv,qemu-aplic", "riscv,aplic";
+      msi-parent = <&imsic_mlevel>;
+      reg = <0xc000000 0x4000>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+      riscv,children = <&aplic4>;
+      riscv,delegate = <&aplic4 1 63>;
+    };
+
+    aplic4: interrupt-controller@d000000 {
+      compatible = "riscv,qemu-aplic", "riscv,aplic";
+      msi-parent = <&imsic_slevel>;
+      reg = <0xd000000 0x4000>;
+      interrupt-controller;
+      #interrupt-cells = <2>;
+      riscv,num-sources = <63>;
+    };
+...