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> <CAK9=C2UxoSzboraCTtBfg9mx9iTQChhkrDuJcOOGFMgiWg9bBQ@mail.gmail.com>
In-Reply-To: <CAK9=C2UxoSzboraCTtBfg9mx9iTQChhkrDuJcOOGFMgiWg9bBQ@mail.gmail.com>
From:   Vincent Chen <vincent.chen@sifive.com>
Date:   Mon, 14 Aug 2023 14:43:56 +0800
Message-ID: <CABvJ_xi3e5nCHOFvCQ=W48BbM5780pmB0auR3yLLiCH4hC0mRA@mail.gmail.com>
Subject: Re: [PATCH v7 11/15] dt-bindings: interrupt-controller: Add RISC-V
 advanced PLIC
To:     Anup Patel <apatel@ventanamicro.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 Sat, Aug 12, 2023 at 10:59=E2=80=AFAM Anup Patel <apatel@ventanamicro.co=
m> wrote:
>
> On Thu, Aug 10, 2023 at 4:59=E2=80=AFPM Vincent Chen <vincent.chen@sifive=
.com> wrote:
> >
> >
> >
> > On Thu, Aug 10, 2023 at 4:08=E2=80=AFPM Anup Patel <apatel@ventanamicro=
.com> wrote:
> >>
> >> On Thu, Aug 10, 2023 at 1:31=E2=80=AFPM Vincent Chen <vincent.chen@sif=
ive.com> wrote:
> >> >
> >> >
> >> >
> >> > On Wed, Aug 2, 2023 at 11:02=E2=80=AFPM Anup Patel <apatel@ventanami=
cro.com> wrote:
> >> >>
> >> >> We add DT bindings document for RISC-V advanced platform level inte=
rrupt
> >> >> controller (APLIC) defined by the RISC-V advanced interrupt archite=
cture
> >> >> (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-con=
troller/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..190a6499c932
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,=
aplic.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,apli=
c.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 adva=
nced
> >> >> +  platform level interrupt controller (APLIC) for handling wired i=
nterrupts
> >> >> +  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 wh=
ere all
> >> >> +  interrupt sources connect to the root APLIC domain and a parent =
APLIC
> >> >> +  domain can delegate interrupt sources to it's child APLIC domain=
s. 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 ris=
cv,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 AI=
A incoming
> >> >> +      message signaled interrupt controller (IMSIC). If both "msi-=
parent" and
> >> >> +      "interrupts-extended" properties are present then it means t=
he 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. Ea=
ch child
> >> >> +      APLIC domain is assigned a child index in increasing order, =
with the
> >> >> +      first child APLIC domain assigned child index 0. The APLIC d=
omain child
> >> >> +      index is used by firmware to delegate interrupts from the gi=
ven 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 =
domain (inclusive)
> >> >> +        - description: last interrupt number of the parent APLIC d=
omain (inclusive)
> >> >> +    description:
> >> >> +      A interrupt delegation list where each entry is a triple con=
sisting
> >> >> +      of child APLIC domain phandle, first interrupt number of the=
 parent
> >> >> +      APLIC domain, and last interrupt number of the parent APLIC =
domain.
> >> >> +      Firmware must configure interrupt delegation registers based=
 on
> >> >> +      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 HA=
RTs):
> >> >> +
> >> >> +    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 understan=
d that it might be a bit late to discuss this matter within the v7 patch (s=
orry for this), I hope you could still consider the following idea.
> >> >
> >> > For example 1, my understanding is that each APLIC DTS node represen=
ts an interrupt domain. IIUC, in physical, these tree Interrupt domains sho=
uld belong to an APLIC device so the M-mode IRQ domain can delegate interru=
pts to the child domain. Given this perspective, wrapping all interrupt dom=
ain DTS nodes with another DTS node seems to present the real scene more cl=
early. Maybe we can add "simple-bus" to the compatible property of this wra=
pped DTS node, so it still can be compatible with your driver implementatio=
ns. 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-socke=
t
> >> 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 =
these interrupt domains belong to the same APLIC device. For this scenario,=
 one thing I am concerned about is that the MMIO region of other devices lo=
cates between interrupt domains. However, currently, I have not found that =
the 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.
>

I understood. Thanks for your reply.

Thanks,
Vincent

> 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