Re: [PATCH 4/9] dt-bindings: Add RISC-V incoming MSI controller bindings

[Rob Herring]

On Mon, Nov 14, 2022 at 05:36:06PM +0530, Anup Patel wrote:
> On Mon, Nov 14, 2022 at 3:19 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >
> > On 11/11/2022 05:42, Anup Patel wrote:
> > > We add DT bindings document for RISC-V incoming MSI controller (IMSIC)
> > > defined by the RISC-V advanced interrupt architecture (AIA) specification.
> > >
> > > Signed-off-by: Anup Patel <apatel@xxxxxxxxxxxxxxxx>
> > > ---
> > >  .../interrupt-controller/riscv,imsic.yaml     | 174 ++++++++++++++++++
> > >  1 file changed, 174 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > > new file mode 100644
> > > index 000000000000..05106eb1955e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,imsic.yaml
> > > @@ -0,0 +1,174 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/interrupt-controller/riscv,imsic.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: RISC-V Incoming MSI Controller (IMSIC)
> > > +
> > > +maintainers:
> > > +  - Anup Patel <anup@xxxxxxxxxxxxxx>
> > > +
> > > +description:
> > > +  The RISC-V advanced interrupt architecture (AIA) defines a per-CPU incoming
> > > +  MSI controller (IMSIC) for handling MSIs in a RISC-V platform. The RISC-V
> > > +  AIA specification can be found at https://github.com/riscv/riscv-aia.
> > > +
> > > +  The IMSIC is a per-CPU (or per-HART) device with separate interrupt file
> > > +  for each privilege level (machine or supervisor). The configuration of
> > > +  a IMSIC interrupt file is done using AIA CSRs and it also has a 4KB MMIO
> > > +  space to receive MSIs from devices. Each IMSIC interrupt file supports a
> > > +  fixed number of interrupt identities (to distinguish MSIs from devices)
> > > +  which is same for given privilege level across CPUs (or HARTs).
> > > +
> > > +  The arrangement of IMSIC interrupt files in MMIO space of a RISC-V platform
> > > +  follows a particular scheme defined by the RISC-V AIA specification. A IMSIC
> > > +  group is a set of IMSIC interrupt files co-located in MMIO space and we can
> > > +  have multiple IMSIC groups (i.e. clusters, sockets, chiplets, etc) in a
> > > +  RISC-V platform. The MSI target address of a IMSIC interrupt file at given
> > > +  privilege level (machine or supervisor) encodes group index, HART index,
> > > +  and guest index (shown below).
> > > +
> > > +  XLEN-1           >=24                                 12    0
> > > +  |                  |                                  |     |
> > > +  -------------------------------------------------------------
> > > +  |xxxxxx|Group Index|xxxxxxxxxxx|HART Index|Guest Index|  0  |
> > > +  -------------------------------------------------------------
> > > +
> > > +  The device tree of a RISC-V platform will have one IMSIC device tree node
> > > +  for each privilege level (machine or supervisor) which collectively describe
> > > +  IMSIC interrupt files at that privilege level across CPUs (or HARTs).
> > > +
> > > +allOf:
> > > +  - $ref: /schemas/interrupt-controller.yaml#
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:
> > > +          - vendor,chip-imsics
> >
> > There is no such vendor... As Conor pointed out, this does not look
> > correct. Compatibles must be real and specific.
> 
> Previously, Rob had suggest to:
> 1) Mandate two compatible strings: one for implementation and
>     and second for specification
> 2) Since this is new specification with QEMU being the only
>     implementation, we add "vendor,chip-imsics" as dummy
>     implementation specific string for DT schema checkers
>     to pass the examples. Once we have an actual implementation,
>    we will replace this dummy string.

What will QEMU's DT use? That's an implementation we can and do run 
validation on. Your choices are define a QEMU specific compatible string 
or allow the fallback alone. I'm fine either way. With the latter, 
someone has to review that the fallback is not used alone in .dts files 
while doing the former allows the tools to check for you. It also 
encourages making every new difference a property rather than implied by 
compatible, but those should be caught in review.

If you go with the fallback only, just make it clear that it's for QEMU 
or s/w models only.

Rob