Re: [RFC PATCH v3 05/11] dt-bindings: interrupt-controller: Add ACLINT MSWI and SSWI bindings

[Rob Herring <robh@kernel.org>]

On Fri, Sep 3, 2021 at 5:40 AM Anup Patel <anup@brainfault.org> wrote:
>
> On Thu, Sep 2, 2021 at 6:04 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Wed, Sep 1, 2021 at 6:56 AM Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Wed, Sep 1, 2021 at 6:54 AM Rob Herring <robh@kernel.org> wrote:
> > > >
> > > > On Mon, Aug 30, 2021 at 09:47:23AM +0530, Anup Patel wrote:
> > > > > We add DT bindings documentation for the ACLINT MSWI and SSWI
> > > > > devices found on RISC-V SOCs.
> > > > >
> > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com>
> > > > > Reviewed-by: Bin Meng <bmeng.cn@gmail.com>
> > > > > ---
> > > > >  .../riscv,aclint-swi.yaml                     | 95 +++++++++++++++++++
> > > > >  1 file changed, 95 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/riscv,aclint-swi.yaml
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/interrupt-controller/riscv,aclint-swi.yaml b/Documentation/devicetree/bindings/interrupt-controller/riscv,aclint-swi.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..68563259ae24
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/interrupt-controller/riscv,aclint-swi.yaml
> > > > > @@ -0,0 +1,95 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/interrupt-controller/riscv,aclint-swi.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: RISC-V ACLINT Software Interrupt Devices
> > > > > +
> > > > > +maintainers:
> > > > > +  - Anup Patel <anup.patel@wdc.com>
> > > > > +
> > > > > +description:
> > > > > +  RISC-V SOCs include an implementation of the M-level software interrupt
> > > > > +  (MSWI) device and the S-level software interrupt (SSWI) device defined
> > > > > +  in the RISC-V Advanced Core Local Interruptor (ACLINT) specification.
> > > > > +
> > > > > +  The ACLINT MSWI and SSWI devices are documented in the RISC-V ACLINT
> > > > > +  specification located at
> > > > > +  https://github.com/riscv/riscv-aclint/blob/main/riscv-aclint.adoc.
> > > > > +
> > > > > +  The ACLINT MSWI and SSWI devices directly connect to the M-level and
> > > > > +  S-level software interrupt lines of various HARTs (or CPUs) respectively
> > > > > +  so the RISC-V per-HART (or per-CPU) local interrupt controller is the
> > > > > +  parent interrupt controller for the ACLINT MSWI and SSWI devices.
> > > > > +
> > > > > +allOf:
> > > > > +  - $ref: /schemas/interrupt-controller.yaml#
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    oneOf:
> > > > > +      - items:
> > > > > +        - enum:
> > > > > +          - riscv,aclint-mswi
> > > > > +
> > > > > +      - items:
> > > > > +        - enum:
> > > > > +          - riscv,aclint-sswi
> > > >
> > > > All this can be just:
> > > >
> > > > enum:
> > > >   - riscv,aclint-mswi
> > > >   - riscv,aclint-sswi
> > > >
> > > > However...
> > > >
> > > > > +
> > > > > +    description:
> > > > > +      For ACLINT MSWI devices, it should be "riscv,aclint-mswi" OR
> > > > > +      "<vendor>,<chip>-aclint-mswi".
> > > > > +      For ACLINT SSWI devices, it should be "riscv,aclint-sswi" OR
> > > > > +      "<vendor>,<chip>-aclint-sswi".
> > > >
> > > > s/OR/AND/
> > > >
> > > > There must be a compatible for the implementation. Unless RiscV
> > > > implementations of specs are complete describing all clocks, power
> > > > domains, resets, etc. and are quirk free.
> > > >
> > > > But don't write free form constraints...
> > >
> > > It is possible that quite a few implementations (QEMU, FPGAs, and
> > > other simulators) will not require implementation specific compatible
> > > strings. Should we still mandate implementation specific compatible
> > > strings in DTS for such cases?
> >
> > No, but the schema says you only have those cases. Are there not any
> > actual implementations?
>
> All existing RISC-V boards have SiFive CLINT and ACLINT is backward
> compatible with SiFive CLINT so we do have actual implementations.

So there's a SiFive compatible you can add here?

> None of the existing RISC-V boards have special clocks, power domain,
> resets etc for these devices.
>
> >
> > Minimally make "<vendor>,<chip>-aclint-mswi" into a schema pattern for
> > the first entry and perhaps a note to replace with actual strings when
> > there are some. It's ultimately up to the RiscV maintainers to require
> > SoC specific compatibles here. Allowing a generic one alone makes that
> > harder because the schema can't enforce it.
>
> Can we have a common compatible string for QEMU, FPGAs, etc ?
>
> For example,
> compatible = "riscv,generic-aclint-mswi", "riscv,aclint-mswi";

This is not any better than just allowing "riscv,aclint-mswi" by
itself as someone could just use the above strings on their new
implementation to avoid warnings.

You could just not worry about the QEMU and FPGA cases. FPGAs are
probably not upstream and if they are, don't they need specific
compatibles tied to versions of FPGA images? QEMU generating its own
DT doesn't run schema validation though that could change. I'm looking
at enabling schema validation at runtime for purposes of firmware
testing and with that QEMU generated DT may be something we test.

Rob