Date:   Mon, 14 Nov 2022 13:21:53 +0100
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Anup Patel <apatel@...tanamicro.com>
Cc:     Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Atish Patra <atishp@...shpatra.org>,
        Alistair Francis <Alistair.Francis@....com>,
        Anup Patel <anup@...infault.org>,
        linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCH 4/9] dt-bindings: Add RISC-V incoming MSI controller
 bindings

On 14/11/2022 13:06, Anup Patel wrote:
> On Mon, Nov 14, 2022 at 3:19 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@...aro.org> 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@...tanamicro.com>
>>> ---
>>>  .../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@...infault.org>
>>> +
>>> +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.
> 
> Refer, https://www.spinics.net/lists/devicetree/msg442720.html

And Rob did not propose vendor as vendor and chip-imsics as device. Read
his message again.

> 
>>
>>> +      - const: riscv,imsics
>>> +
>>> +  reg:
>>> +    minItems: 1
>>> +    maxItems: 128
>>
>> Is there a DTS with 128 reg items?
> 
> Not at the moment since this is a new specification.
> 
> The value "128" is because maximum number of
> IMSIC groups on an system with both IMSIC and
> APLIC is 128 where each IMSIC group has a
> separate base address. This is not a hard limit so
> I am willing to drop it as well.

Is "separate base address" really a separate different range or just
spaced by few registers?

> 
>>
>>> +    description:
>>> +      Base address of each IMSIC group.
>>> +
>>> +  interrupt-controller: true
>>> +
>>> +  "#interrupt-cells":
>>> +    const: 0
>>> +
>>> +  msi-controller: true
>>
>> You want then msi-controller.yaml schema and you can drop properties
>> described there.
> 
> Okay, I will include msi-controller.yaml in the next revision.
> 
>>
>>> +
>>> +  interrupts-extended:
>>> +    minItems: 1
>>> +    maxItems: 32768
>>
>> I just wonder if you are not putting some random stuff here... just like
>> this "vendor" company.
>>
>> 32768 inputs it is quite a big chip. Are you sure you have so many pins
>> or internal connections?
> 
> The interrupts-extended property describes the association of IMSIC
> interrupt files with the HARTs. If there are N HARTs then we will have
> N entries in the interrupts-extended (just like the existing PLIC DT bindings).
> 
> For example, if the first entry points to HART1 and the second entry points
> to HART0 then the first interrupt file is associated with HART1 and the
> second interrupt file is associated with HART0.
> 
> Currently, the "maxItems" limit reflects the max IMSICs which an APLIC
> domain can target on a system with both IMSIC and APLIC.
> 
> Actually, there is a typo here. The "maxItems" should be 16384 as-per
> the frozen AIA specification. I will update "maxItems" accordingly in
> next patch revision.
> 
>>
>>> +    description:
>>> +      This property represents the set of CPUs (or HARTs) for which given
>>> +      device tree node describes the IMSIC interrupt files. Each node pointed
>>> +      to should be a riscv,cpu-intc node, which has a riscv node (i.e. RISC-V
>>> +      HART) as parent.
>>> +
>>> +  riscv,num-ids:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 63
>>> +    maximum: 2047
>>> +    description:
>>> +      Specifies how many interrupt identities are supported by IMSIC interrupt
>>> +      file.
>>> +
>>> +  riscv,num-guest-ids:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 63
>>> +    maximum: 2047
>>> +    description:
>>> +      Specifies how many interrupt identities are supported by IMSIC guest
>>> +      interrupt file. When not specified the number of interrupt identities
>>> +      supported by IMSIC guest file is assumed to be same as specified by
>>> +      the riscv,num-ids property.
>>> +
>>> +  riscv,slow-ipi:
>>> +    type: boolean
>>> +    description:
>>> +      The presence of this property implies that software interrupts (i.e.
>>> +      IPIs) using IMSIC software injected MSIs is slower compared to other
>>> +      software interrupt mechanisms (such as SBI IPI) on the underlying
>>> +      RISC-V platform.
>>
>> Is this a property of software or hardware?
> 
> This is a property of hardware (or implementation) because IPIs
> in IMSIC are software injected MSIs so if IMSIC is trap-n-emulated
> by a hypervisor then all writes to MSI register will trap to hypervisor
> in which case IPI injection via IMSIC is slow.
> 
> The presence of "riscv,slow-ipi" DT property provides a hint to
> driver that using IPIs through IMSIC is slow on this platform so
> if there are other IPI mechanisms (such as SBI IPI calls) then
> OS should prefer those mechanisms.

If this is specific to implementation, why it is not included already in
the compatible?

The name is anyway too vague. What is "slow"? Describe real
characteristics of hardware, e.g. trapped via hypervisor.
> 
>>
>>> +
>>> +  riscv,guest-index-bits:
>>> +    minimum: 0
>>> +    maximum: 7
>>> +    description:
>>> +      Specifies number of guest index bits in the MSI target address. When
>>> +      not specified it is assumed to be 0.
>>> +
>>> +  riscv,hart-index-bits:
>>> +    minimum: 0
>>> +    maximum: 15
>>> +    description:
>>> +      Specifies number of HART index bits in the MSI target address. When
>>> +      not specified it is estimated based on the interrupts-extended property.
>>> +
>>> +  riscv,group-index-bits:
>>> +    minimum: 0
>>> +    maximum: 7
>>> +    description:
>>> +      Specifies number of group index bits in the MSI target address. When
>>> +      not specified it is assumed to be 0.
>>
>> Then default: 0.
> 
> Okay, I will update.
> 
>>
>>> +
>>> +  riscv,group-index-shift:
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    minimum: 24
>>> +    maximum: 55
>>> +    description:
>>> +      Specifies the least significant bit of the group index bits in the
>>
>> Please drop everywhere "Specifies the" and instead just describe the
>> hardware.
> 
> Okay, I will update.
> 
>>
>>> +      MSI target address. When not specified it is assumed to be 24.
>>> +
>>> +additionalProperties: false
>>
>> unevaluatedProperties: false and drop all properties already described
>> by other schemas.
> 
> Okay, I will update.
> 
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - interrupt-controller
>>> +  - msi-controller
>>> +  - interrupts-extended
>>> +  - riscv,num-ids
>>> +
>>> +examples:
>>> +  - |
>>> +    // Example 1 (Machine-level IMSIC files with just one group):
>>> +
>>> +    imsic_mlevel: interrupt-controller@...00000 {
>>> +      compatible = "vendor,chip-imsics", "riscv,imsics";
>>> +      interrupts-extended = <&cpu1_intc 11>,
>>> +                            <&cpu2_intc 11>,
>>> +                            <&cpu3_intc 11>,
>>> +                            <&cpu4_intc 11>;
>>> +      reg = <0x28000000 0x4000>;
>>> +      interrupt-controller;
>>> +      #interrupt-cells = <0>;
>>> +      msi-controller;
>>> +      riscv,num-ids = <127>;
>>> +    };
>>> +
>>> +  - |
>>> +    // Example 2 (Supervisor-level IMSIC files with two groups):
>>> +
>>> +    imsic_slevel: interrupt-controller@...00000 {
>>> +      compatible = "vendor,chip-imsics", "riscv,imsics";
>>
>> Please run scripts/checkpatch.pl and fix reported warnings.
> 
> I did not see any warnings with ./scripts/checkpatch.pl.
> 
> Is there any parameter of checkpatch.pl which I should try ?

You should see here or with your DTS warnings about undocumented vendor
prefix.

Best regards,
Krzysztof

Powered by blists - more mailing lists