Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230802150018.327079-1-apatel@ventanamicro.com>
 <20230802150018.327079-12-apatel@ventanamicro.com> <CABvJ_xi5r-NL=22tJWfyQQSti4XgUwsx94B8mQ3LJU29kiQC8w@mail.gmail.com>
 <CAK9=C2UPfwJ+JccE+7zTcrqdOZ8np0UkL6P8n-Zofbj5NZ=WXQ@mail.gmail.com> <CABvJ_xiZY5RGMXOq0bWKRdkzD=b4ar6cFiujmPbUYmHUzSW5Qw@mail.gmail.com>
In-Reply-To: <CABvJ_xiZY5RGMXOq0bWKRdkzD=b4ar6cFiujmPbUYmHUzSW5Qw@mail.gmail.com>
From:   Anup Patel <apatel@ventanamicro.com>
Date:   Sat, 12 Aug 2023 08:29:12 +0530
Message-ID: <CAK9=C2UxoSzboraCTtBfg9mx9iTQChhkrDuJcOOGFMgiWg9bBQ@mail.gmail.com>
Subject: Re: [PATCH v7 11/15] dt-bindings: interrupt-controller: Add RISC-V
 advanced PLIC
To:     Vincent Chen <vincent.chen@sifive.com>
Cc:     Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Frank Rowand <frowand.list@gmail.com>,
        Conor Dooley <conor+dt@kernel.org>, devicetree@vger.kernel.org,
        Saravana Kannan <saravanak@google.com>,
        Anup Patel <anup@brainfault.org>, linux-kernel@vger.kernel.org,
        Conor Dooley <conor.dooley@microchip.com>,
        Atish Patra <atishp@atishpatra.org>,
        linux-riscv@lists.infradead.org,
        Andrew Jones <ajones@ventanamicro.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Aug 10, 2023 at 4:59=E2=80=AFPM Vincent Chen <vincent.chen@sifive.c=
om> wrote:
>
>
>
> On Thu, Aug 10, 2023 at 4:08=E2=80=AFPM Anup Patel <apatel@ventanamicro.c=
om> wrote:
>>
>> On Thu, Aug 10, 2023 at 1:31=E2=80=AFPM Vincent Chen <vincent.chen@sifiv=
e.com> wrote:
>> >
>> >
>> >
>> > On Wed, Aug 2, 2023 at 11:02=E2=80=AFPM Anup Patel <apatel@ventanamicr=
o.com> wrote:
>> >>
>> >> We add DT bindings document for RISC-V advanced platform level interr=
upt
>> >> controller (APLIC) defined by the RISC-V advanced interrupt architect=
ure
>> >> (AIA) specification.
>> >>
>> >> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>> >> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>> >> ---
>> >>  .../interrupt-controller/riscv,aplic.yaml     | 172 ++++++++++++++++=
++
>> >>  1 file changed, 172 insertions(+)
>> >>  create mode 100644 Documentation/devicetree/bindings/interrupt-contr=
oller/riscv,aplic.yaml
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/r=
iscv,aplic.yaml b/Documentation/devicetree/bindings/interrupt-controller/ri=
scv,aplic.yaml
>> >> new file mode 100644
>> >> index 000000000000..190a6499c932
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,ap=
lic.yaml
>> >> @@ -0,0 +1,172 @@
>> >> +# 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 advanc=
ed
>> >> +  platform level interrupt controller (APLIC) for handling wired int=
errupts
>> >> +  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 wher=
e all
>> >> +  interrupt sources connect to the root APLIC domain and a parent AP=
LIC
>> >> +  domain can delegate interrupt sources to it's child APLIC 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 s=
et of
>> >> +      RISC-V HARTS (or CPUs). Each node pointed to should be a riscv=
,cpu-intc
>> >> +      node, which has a CPU 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-pa=
rent" and
>> >> +      "interrupts-extended" properties are present then it means the=
 APLIC
>> >> +      domain supports both MSI mode and Direct mode in HW. In this c=
ase, 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 t=
his
>> >> +      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, wi=
th the
>> >> +      first child APLIC domain assigned child index 0. The APLIC dom=
ain child
>> >> +      index is used by firmware to delegate interrupts from the give=
n APLIC
>> >> +      domain to a particular child APLIC domain.
>> >> +
>> >> +  riscv,delegation:
>> >> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> >> +    minItems: 1
>> >> +    maxItems: 1024
>> >> +    items:
>> >> +      items:
>> >> +        - description: child APLIC domain phandle
>> >> +        - description: first interrupt number of the parent APLIC do=
main (inclusive)
>> >> +        - description: last interrupt number of the parent APLIC dom=
ain (inclusive)
>> >> +    description:
>> >> +      A interrupt delegation list where each entry is a triple consi=
sting
>> >> +      of child APLIC domain phandle, first interrupt number of the p=
arent
>> >> +      APLIC domain, and last interrupt number of the parent APLIC do=
main.
>> >> +      Firmware must configure interrupt delegation registers based o=
n
>> >> +      interrupt delegation list.
>> >> +
>> >> +dependencies:
>> >> +  riscv,delegation: [ "riscv,children" ]
>> >> +
>> >> +required:
>> >> +  - compatible
>> >> +  - reg
>> >> +  - interrupt-controller
>> >> +  - "#interrupt-cells"
>> >> +  - riscv,num-sources
>> >> +
>> >> +anyOf:
>> >> +  - required:
>> >> +      - interrupts-extended
>> >> +  - required:
>> >> +      - msi-parent
>> >> +
>> >> +unevaluatedProperties: false
>> >> +
>> >> +examples:
>> >> +  - |
>> >> +    // Example 1 (APLIC domains directly injecting interrupt to HART=
s):
>> >> +
>> >> +    interrupt-controller@c000000 {
>> >> +      compatible =3D "qemu,aplic", "riscv,aplic";
>> >> +      interrupts-extended =3D <&cpu1_intc 11>,
>> >> +                            <&cpu2_intc 11>,
>> >> +                            <&cpu3_intc 11>,
>> >> +                            <&cpu4_intc 11>;
>> >> +      reg =3D <0xc000000 0x4080>;
>> >> +      interrupt-controller;
>> >> +      #interrupt-cells =3D <2>;
>> >> +      riscv,num-sources =3D <63>;
>> >> +      riscv,children =3D <&aplic1>, <&aplic2>;
>> >> +      riscv,delegation =3D <&aplic1 1 63>;
>> >> +    };
>> >> +
>> >> +    aplic1: interrupt-controller@d000000 {
>> >> +      compatible =3D "qemu,aplic", "riscv,aplic";
>> >> +      interrupts-extended =3D <&cpu1_intc 9>,
>> >> +                            <&cpu2_intc 9>;
>> >> +      reg =3D <0xd000000 0x4080>;
>> >> +      interrupt-controller;
>> >> +      #interrupt-cells =3D <2>;
>> >> +      riscv,num-sources =3D <63>;
>> >> +    };
>> >> +
>> >> +    aplic2: interrupt-controller@e000000 {
>> >> +      compatible =3D "qemu,aplic", "riscv,aplic";
>> >> +      interrupts-extended =3D <&cpu3_intc 9>,
>> >> +                            <&cpu4_intc 9>;
>> >> +      reg =3D <0xe000000 0x4080>;
>> >> +      interrupt-controller;
>> >> +      #interrupt-cells =3D <2>;
>> >> +      riscv,num-sources =3D <63>;
>> >> +    };
>> >> +
>> >
>> >
>> > Hi Anup,
>> > I have some thoughts regarding the APLIC DTS node. While I understand =
that it might be a bit late to discuss this matter within the v7 patch (sor=
ry for this), I hope you could still consider the following idea.
>> >
>> > For example 1, my understanding is that each APLIC DTS node represents=
 an interrupt domain. IIUC, in physical, these tree Interrupt domains shoul=
d belong to an APLIC device so the M-mode IRQ domain can delegate interrupt=
s to the child domain. Given this perspective, wrapping all interrupt domai=
n DTS nodes with another DTS node seems to present the real scene more clea=
rly. Maybe we can add "simple-bus" to the compatible property of this wrapp=
ed DTS node, so it still can be compatible with your driver implementations=
. Therefore, example 1 may become the following.
>> >
>> > interrupt-controller {
>> >         compatible =3D "riscv,aplics", "simple-bus";
>> >         ranges;
>> >         aplic0: interrupt-domain@c000000 {
>> >                 compatible =3D "qemu,aplic", "riscv,aplic";
>> >                 interrupts-extended =3D <&cpu1_intc 11>,
>> >                                       <&cpu2_intc 11>,
>> >                                       <&cpu3_intc 11>,
>> >                                       <&cpu4_intc 11>;
>> >                 reg =3D <0xc000000 0x4080>;
>> >                 interrupt-controller;
>> >                 #interrupt-cells =3D <2>;
>> >                 riscv,num-sources =3D <63>;
>> >                 riscv,children =3D <&aplic1>, <&aplic2>;
>> >                 riscv,delegation =3D <&aplic1 1 63>;
>> >         };
>> >
>> >         aplic1: interrupt-domain@d000000 {
>> >                 compatible =3D "qemu,aplic", "riscv,aplic";
>> >                 interrupts-extended =3D <&cpu1_intc 9>,
>> >                                       <&cpu2_intc 9>;
>> >                 reg =3D <0xd000000 0x4080>;
>> >                 interrupt-controller;
>> >                 #interrupt-cells =3D <2>;
>> >                 riscv,num-sources =3D <63>;
>> >         };
>> >
>> >         aplic2: interrupt-domain@e000000 {
>> >                 compatible =3D "qemu,aplic", "riscv,aplic";
>> >                 interrupts-extended =3D <&cpu3_intc 9>,
>> >                                       <&cpu4_intc 9>;
>> >                 reg =3D <0xe000000 0x4080>;
>> >                 interrupt-controller;
>> >                 #interrupt-cells =3D <2>;
>> >                 riscv,num-sources =3D <63>;
>> >         };
>> > };
>> > Is it feasible for you?
>>
>> This clubbing of APLIC domains and placing them close to each
>> other is a platform implementation choice. On multi-die or multi-socket
>> systems the APIC domains can be really far apart on different physical
>> dies so the clubbing you suggest is not always true.
>>
>> Regards,
>> Anup
>>
> IIUC, I think my suggestion might be able to apply to this scenario if th=
ese interrupt domains belong to the same APLIC device. For this scenario, o=
ne thing I am concerned about is that the MMIO region of other devices loca=
tes between interrupt domains. However, currently, I have not found that th=
e DTS specification explicitly prohibits it. Do you have the same concern?

One APLIC device implementing multiple APLIC domains is perfectly fine
and there is nothing in this DT bindings which prevents that.

We can always group multiple APLIC domain DT nodes under another DT
node but we don't need any special DT bindings for such grouping and
the APLIC driver does not need to be aware of this grouping. Further,
the AIA spec does not mandate grouping of APLIC domains under one
APLIC device.

Like I said previously, the grouping of APLIC domains under one DT
node is implementation/platform specific.

Regards,
Anup

>
> Thanks,
> Vincent
>>
>> >
>> > Thanks,
>> > Vincent
>> >
>> >>
>> >> +  - |
>> >> +    // Example 2 (APLIC domains forwarding interrupts as MSIs):
>> >> +
>> >> +    interrupt-controller@c000000 {
>> >> +      compatible =3D "qemu,aplic", "riscv,aplic";
>> >> +      msi-parent =3D <&imsic_mlevel>;
>> >> +      reg =3D <0xc000000 0x4000>;
>> >> +      interrupt-controller;
>> >> +      #interrupt-cells =3D <2>;
>> >> +      riscv,num-sources =3D <63>;
>> >> +      riscv,children =3D <&aplic3>;
>> >> +      riscv,delegation =3D <&aplic3 1 63>;
>> >> +    };
>> >> +
>> >> +    aplic3: interrupt-controller@d000000 {
>> >> +      compatible =3D "qemu,aplic", "riscv,aplic";
>> >> +      msi-parent =3D <&imsic_slevel>;
>> >> +      reg =3D <0xd000000 0x4000>;
>> >> +      interrupt-controller;
>> >> +      #interrupt-cells =3D <2>;
>> >> +      riscv,num-sources =3D <63>;
>> >> +    };
>> >> +...
>> >> --
>> >> 2.34.1
>> >>
>> >>
>> >> _______________________________________________
>> >> linux-riscv mailing list
>> >> linux-riscv@lists.infradead.org
>> >> http://lists.infradead.org/mailman/listinfo/linux-riscv