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;
Date:   Wed, 4 Jan 2023 22:16:58 +0000
From:   Conor Dooley <conor@kernel.org>
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>,
        Atish Patra <atishp@atishpatra.org>,
        Alistair Francis <Alistair.Francis@wdc.com>,
        Anup Patel <anup@brainfault.org>,
        linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Subject: Re: [PATCH v2 6/9] dt-bindings: interrupt-controller: Add RISC-V
 advanced PLIC
Message-ID: <Y7X62v5Zp6+thx5A@spud>
References: <20230103141409.772298-1-apatel@ventanamicro.com>
 <20230103141409.772298-7-apatel@ventanamicro.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="CM+OG7HiznyhPXQ1"
Content-Disposition: inline
In-Reply-To: <20230103141409.772298-7-apatel@ventanamicro.com>
Precedence: bulk


--CM+OG7HiznyhPXQ1
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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.
>=20
> 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-controlle=
r/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 inco=
ming
> +      message signaled interrupt controller (IMSIC). This property shoul=
d 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 chi=
ld
> +      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 =3D "riscv,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,delegate =3D <&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 =3D "riscv,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 "riscv,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>;
> +    };


--CM+OG7HiznyhPXQ1
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCY7X62gAKCRB4tDGHoIJi
0jdPAQDE7jaXqDBSHkYz30DfCDF68e8vHV/nGNfwySX74TqkmAEA8kiQVjEAphNj
VrxoMV535/dnuFew0/H+emA4BYc1swU=
=Whsj
-----END PGP SIGNATURE-----

--CM+OG7HiznyhPXQ1--