diff mbox series

[01/24] Documentation: devicetree: bindings: Add GICv5 DT bindings

Message ID 20250408-gicv5-host-v1-1-1f26db465f8d@kernel.org (mailing list archive)
State New
Headers show
Series Arm GICv5: Host driver implementation | expand

Commit Message

Lorenzo Pieralisi April 8, 2025, 10:50 a.m. UTC
The GICv5 interrupt controller architecture is composed of:

- one or more Interrupt Routing Service (IRS)
- zero or more Interrupt Translation Service (ITS)
- zero or more Interrupt Wire Bridge (IWB)

Describe a GICv5 implementation by specifying a top level node
corresponding to the GICv5 system component.

IRS nodes are added as GICv5 system component children.

An ITS is associated with an IRS so ITS nodes are described
as IRS children - use the hierarchy explicitly in the device
tree to define the association.

IWB nodes are described as GICv5 system component children - to make it
explicit that are part of the GICv5 system component; an IWB is
connected to a single ITS but the connection is made explicit through
the msi-parent property and therefore is not required to be explicit
through a parent-child relationship in the device tree.

Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Rob Herring <robh@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
---
 .../bindings/interrupt-controller/arm,gic-v5.yaml  | 268 +++++++++++++++++++++
 MAINTAINERS                                        |   7 +
 2 files changed, 275 insertions(+)

Comments

Rob Herring April 8, 2025, 12:26 p.m. UTC | #1
On Tue, 08 Apr 2025 12:50:00 +0200, Lorenzo Pieralisi wrote:
> The GICv5 interrupt controller architecture is composed of:
> 
> - one or more Interrupt Routing Service (IRS)
> - zero or more Interrupt Translation Service (ITS)
> - zero or more Interrupt Wire Bridge (IWB)
> 
> Describe a GICv5 implementation by specifying a top level node
> corresponding to the GICv5 system component.
> 
> IRS nodes are added as GICv5 system component children.
> 
> An ITS is associated with an IRS so ITS nodes are described
> as IRS children - use the hierarchy explicitly in the device
> tree to define the association.
> 
> IWB nodes are described as GICv5 system component children - to make it
> explicit that are part of the GICv5 system component; an IWB is
> connected to a single ITS but the connection is made explicit through
> the msi-parent property and therefore is not required to be explicit
> through a parent-child relationship in the device tree.
> 
> Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> ---
>  .../bindings/interrupt-controller/arm,gic-v5.yaml  | 268 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 +
>  2 files changed, 275 insertions(+)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.example.dts:43.27-28 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1522: dt_binding_check] Error 2
make: *** [Makefile:248: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250408-gicv5-host-v1-1-1f26db465f8d@kernel.org

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Lorenzo Pieralisi April 8, 2025, 2:58 p.m. UTC | #2
On Tue, Apr 08, 2025 at 07:26:53AM -0500, Rob Herring (Arm) wrote:
> 
> On Tue, 08 Apr 2025 12:50:00 +0200, Lorenzo Pieralisi wrote:
> > The GICv5 interrupt controller architecture is composed of:
> > 
> > - one or more Interrupt Routing Service (IRS)
> > - zero or more Interrupt Translation Service (ITS)
> > - zero or more Interrupt Wire Bridge (IWB)
> > 
> > Describe a GICv5 implementation by specifying a top level node
> > corresponding to the GICv5 system component.
> > 
> > IRS nodes are added as GICv5 system component children.
> > 
> > An ITS is associated with an IRS so ITS nodes are described
> > as IRS children - use the hierarchy explicitly in the device
> > tree to define the association.
> > 
> > IWB nodes are described as GICv5 system component children - to make it
> > explicit that are part of the GICv5 system component; an IWB is
> > connected to a single ITS but the connection is made explicit through
> > the msi-parent property and therefore is not required to be explicit
> > through a parent-child relationship in the device tree.
> > 
> > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > ---
> >  .../bindings/interrupt-controller/arm,gic-v5.yaml  | 268 +++++++++++++++++++++
> >  MAINTAINERS                                        |   7 +
> >  2 files changed, 275 insertions(+)
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.example.dts:43.27-28 syntax error
> FATAL ERROR: Unable to parse input tree
> make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.example.dtb] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1522: dt_binding_check] Error 2
> make: *** [Makefile:248: __sub-make] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250408-gicv5-host-v1-1-1f26db465f8d@kernel.org
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.

Validated the bindings, not the example within, that caused this to trigger,
apologies.

Already fixed it - please review the bindings though, feedback on them
appreciated, thanks.

Lorenzo
Rob Herring April 8, 2025, 3:07 p.m. UTC | #3
On Tue, Apr 8, 2025 at 5:50 AM Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
>

No need to say DT bindings twice in the subject line. Follow the
subsystem conventions.

dt-bindings: interrupt-controller: Add Arm GICv5

> The GICv5 interrupt controller architecture is composed of:
>
> - one or more Interrupt Routing Service (IRS)
> - zero or more Interrupt Translation Service (ITS)
> - zero or more Interrupt Wire Bridge (IWB)
>
> Describe a GICv5 implementation by specifying a top level node
> corresponding to the GICv5 system component.
>
> IRS nodes are added as GICv5 system component children.
>
> An ITS is associated with an IRS so ITS nodes are described
> as IRS children - use the hierarchy explicitly in the device
> tree to define the association.
>
> IWB nodes are described as GICv5 system component children - to make it
> explicit that are part of the GICv5 system component; an IWB is
> connected to a single ITS but the connection is made explicit through
> the msi-parent property and therefore is not required to be explicit
> through a parent-child relationship in the device tree.
>
> Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Rob Herring <robh@kernel.org>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> ---
>  .../bindings/interrupt-controller/arm,gic-v5.yaml  | 268 +++++++++++++++++++++
>  MAINTAINERS                                        |   7 +
>  2 files changed, 275 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml
> new file mode 100644
> index 0000000000000000000000000000000000000000..5c78375c298a0115c55872f439eb04d4171c4381
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml
> @@ -0,0 +1,268 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/arm,gic-v5.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM Generic Interrupt Controller, version 5
> +
> +maintainers:
> +  - Lorenzo Pieralisi <lpieralisi@kernel.org>
> +  - Marc Zyngier <maz@kernel.org>
> +
> +description: |
> +  The GICv5 architecture defines the guidelines to implement GICv5
> +  compliant interrupt controllers for AArch64 systems.
> +
> +  The GICv5 specification can be found at
> +  https://developer.arm.com/documentation/aes0070
> +
> +  The GICv5 architecture is composed of multiple components:
> +    - one or more IRS (Interrupt Routing Service)
> +    - zero or more ITS (Interrupt Translation Service)
> +    - zero or more IWB (Interrupt Wire Bridge)
> +
> +  The architecture defines:
> +    - PE-Private Peripheral Interrupts (PPI)
> +    - Shared Peripheral Interrupts (SPI)
> +    - Logical Peripheral Interrupts (LPI)
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> +  compatible:
> +    const: arm,gic-v5
> +
> +  interrupt-controller: true
> +
> +  "#address-cells":
> +    enum: [ 1, 2 ]

blank line

> +  "#size-cells":
> +    enum: [ 1, 2 ]
> +
> +  ranges: true
> +
> +  "#interrupt-cells":
> +    description: |
> +      Specifies the number of cells needed to encode an interrupt source.
> +      Must be a single cell with a value 3.

Drop this paragraph. The first sentence is just describing a common
property. The 2nd is expressed as schema already.

> +
> +      The 1st cell corresponds to the INTID.Type field in the INTID; 1 for PPI,
> +      3 for SPI. LPI interrupts must not be described in the bindings since
> +      they are allocated dynamically by the software component managing them.
> +
> +      The 2nd cell contains the interrupt INTID.ID field.
> +
> +      The 3rd cell is the flags, encoded as follows:
> +      bits[3:0] trigger type and level flags.
> +
> +        1 = low-to-high edge triggered
> +        2 = high-to-low edge triggered
> +        4 = active high level-sensitive
> +        8 = active low level-sensitive
> +
> +      Cells 4 and beyond are reserved for future use and must have a value
> +      of 0 if present.

Drop. You can't have 4 or more cells because only 3 is allowed:

> +    const: 3
> +
> +  interrupts:
> +    description:
> +      Interrupt source of the VGIC maintenance interrupt.

Drop "Interrupt source of ".

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +
> +patternProperties:
> +  "^irs@[0-9a-f]+$":
> +    type: object
> +    description:
> +      GICv5 has one or more Interrupt Routing Services (IRS) that are
> +      responsible for handling IRQ state and routing.
> +
> +    additionalProperties: false

blank line

> +    properties:
> +      compatible:
> +        const: arm,gic-v5-irs
> +
> +      "#address-cells":
> +        enum: [ 1, 2 ]

blank line

> +      "#size-cells":
> +        enum: [ 1, 2 ]
> +
> +      ranges: true
> +
> +      dma-noncoherent:
> +        description:
> +          Present if the GIC IRS permits programming shareability and
> +          cacheability attributes but is connected to a non-coherent
> +          downstream interconnect.
> +
> +      reg:
> +        minItems: 1
> +        items:
> +          - description: IRS control frame
> +          - description: IRS setlpi frame
> +
> +      cpus:
> +        $ref: /schemas/types.yaml#/definitions/phandle-array

Already has a type, drop.

> +        description:
> +          Should be a list of phandles to CPU nodes (as described in
> +          Documentation/devicetree/bindings/arm/cpus.yaml) corresponding to
> +          CPUs managed by the IRS.

The actual cpu binding is outside the scope of this binding. Just
'CPUs managed by the IRS.' is enough.

Is there a maximum number of CPUs?

> +
> +      arm,iaffids:
> +        $ref: /schemas/types.yaml#/definitions/uint16-array
> +        description:
> +          Should be a list of u16 values representing IAFFID IDs associated

The type says it is 'a list of u16 values', so don't repeat that here.
IAFFID needs to be defined somewhere. Is the 2nd 'ID' redundant?

> +          with the CPU whose CPU node phandle is at the same index in the
> +          cpus array.
> +
> +    patternProperties:
> +      "^msi-controller@[0-9a-f]+$":
> +        type: object
> +        description:
> +          GICv5 has zero or more Interrupt Translation Services (ITS) that are
> +          used to route Message Signalled Interrupts (MSI) to the CPUs. Each
> +          ITS is connected to an IRS.
> +        additionalProperties: false

blank line

> +        properties:
> +          compatible:
> +            const: arm,gic-v5-its
> +
> +          dma-noncoherent:
> +            description:
> +              Present if the GIC ITS permits programming shareability and
> +              cacheability attributes but is connected to a non-coherent
> +              downstream interconnect.
> +
> +          msi-controller: true
> +
> +          "#msi-cells":
> +            description:
> +              The single msi-cell is the DeviceID of the device which will
> +              generate the MSI.
> +            const: 1
> +
> +          reg:
> +            items:
> +              - description: ITS control frame
> +              - description: ITS translate frame
> +
> +        required:
> +          - compatible
> +          - msi-controller
> +          - "#msi-cells"
> +          - reg
> +
> +    required:
> +      - compatible
> +      - reg
> +      - cpus
> +      - arm,iaffids
> +
> +  "^interrupt-controller@[0-9a-f]+$":
> +    type: object
> +    description:
> +      GICv5 has zero or more Interrupt Wire Bridges (IWB) that are responsible
> +      for translating wire signals into interrupt messages to the ITS.

I wonder if this should be a separate schema and not a child of the
GIC? Seems like these would be implemented standalone (even if the
arch doesn't define that) at arbitrary addresses that aren't within
the GIC's address range. To put it another way, there's nothing here
it is getting from the parent node.

> +
> +    additionalProperties: false
> +    properties:
> +      compatible:
> +        const: arm,gic-v5-iwb
> +
> +      interrupt-controller: true
> +
> +      "#address-cells":
> +        const: 0
> +
> +      "#interrupt-cells":
> +        description: |
> +          Specifies the number of cells needed to encode an interrupt source.
> +          Must be a single cell with a value 2.

Drop

> +
> +          The 1st cell corresponds to the IWB wire.
> +
> +          The 2nd cell is the flags, encoded as follows:
> +          bits[3:0] trigger type and level flags.
> +
> +          1 = low-to-high edge triggered
> +          2 = high-to-low edge triggered
> +          4 = active high level-sensitive
> +          8 = active low level-sensitive
> +
> +          Cells 3 and beyond are reserved for future use and must have a value
> +          of 0 if present.

Drop

> +        const: 2
> +
> +      reg:
> +        items:
> +          - description: IWB control frame
> +
> +      msi-parent: true

maxItems: 1

Because the common definition allows any number of parents.

> +
> +    required:
> +      - compatible
> +      - reg
> +      - msi-parent
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    interrupt-controller {
> +      compatible = "arm,gic-v5";
> +      #interrupt-cells = <3>;
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +      ranges;
> +
> +      interrupt-controller;
> +
> +      interrupts = <1 25 4>;
> +
> +      irs@2f1a0000 {
> +        compatible = "arm,gic-v5-irs";
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +        ranges;
> +
> +        reg = <0x0 0x2f1a0000 0x0 0x10000>;  // IRS_CONFIG_FRAME for NS
> +
> +        arm,iaffids = /bits 16 <0 1 2 3 4 5 6 7>;
> +        cpus = <&{/cpus/cpu@0}>, <&{/cpus/cpu@100}>, <&{/cpus/cpu@200}>,
> +               <&{/cpus/cpu@300}>, <&{/cpus/cpu@10000}>, <&{/cpus/cpu@10100}>,
> +               <&{/cpus/cpu@10200}>, <&{/cpus/cpu@10300}>;

Use labels instead of paths.

> +
> +        msi-controller@2f120000 {
> +          compatible = "arm,gic-v5-its";
> +
> +          msi-controller;
> +          #msi-cells = <1>;
> +
> +          reg = <0x0 0x2f120000 0x0 0x10000    // ITS_CONFIG_FRAME for NS
> +                 0x0 0x2f130000 0x0 0x10000>;  // ITS_TRANSLATE_FRAME
> +        };
> +      };
> +
> +      interrupt-controller@2f000000 {
> +        compatible = "arm,gic-v5-iwb";
> +        #address-cells = <0>;
> +
> +        interrupt-controller;
> +        #interrupt-cells = <2>;
> +
> +        reg = <0x0 0x2f000000 0x0 0x10000>;
> +
> +        msi-parent = <&its0 64>;
> +      };
> +    };
> +
> +    device@0 {

Drop. We don't put consumers in provider examples and vice-versa.

> +      reg = <0 4>;
> +      interrupts = <3 115 4>;
> +    };
> +
> +...
Lorenzo Pieralisi April 9, 2025, 8:20 a.m. UTC | #4
On Tue, Apr 08, 2025 at 10:07:06AM -0500, Rob Herring wrote:
> On Tue, Apr 8, 2025 at 5:50 AM Lorenzo Pieralisi <lpieralisi@kernel.org> wrote:
> >
> 
> No need to say DT bindings twice in the subject line. Follow the
> subsystem conventions.
> 
> dt-bindings: interrupt-controller: Add Arm GICv5

Will do.

> > The GICv5 interrupt controller architecture is composed of:
> >
> > - one or more Interrupt Routing Service (IRS)
> > - zero or more Interrupt Translation Service (ITS)
> > - zero or more Interrupt Wire Bridge (IWB)
> >
> > Describe a GICv5 implementation by specifying a top level node
> > corresponding to the GICv5 system component.
> >
> > IRS nodes are added as GICv5 system component children.
> >
> > An ITS is associated with an IRS so ITS nodes are described
> > as IRS children - use the hierarchy explicitly in the device
> > tree to define the association.
> >
> > IWB nodes are described as GICv5 system component children - to make it
> > explicit that are part of the GICv5 system component; an IWB is
> > connected to a single ITS but the connection is made explicit through
> > the msi-parent property and therefore is not required to be explicit
> > through a parent-child relationship in the device tree.
> >
> > Signed-off-by: Lorenzo Pieralisi <lpieralisi@kernel.org>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: Rob Herring <robh@kernel.org>
> > Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> > Cc: Marc Zyngier <maz@kernel.org>
> > ---
> >  .../bindings/interrupt-controller/arm,gic-v5.yaml  | 268 +++++++++++++++++++++
> >  MAINTAINERS                                        |   7 +
> >  2 files changed, 275 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..5c78375c298a0115c55872f439eb04d4171c4381
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml
> > @@ -0,0 +1,268 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interrupt-controller/arm,gic-v5.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ARM Generic Interrupt Controller, version 5
> > +
> > +maintainers:
> > +  - Lorenzo Pieralisi <lpieralisi@kernel.org>
> > +  - Marc Zyngier <maz@kernel.org>
> > +
> > +description: |
> > +  The GICv5 architecture defines the guidelines to implement GICv5
> > +  compliant interrupt controllers for AArch64 systems.
> > +
> > +  The GICv5 specification can be found at
> > +  https://developer.arm.com/documentation/aes0070
> > +
> > +  The GICv5 architecture is composed of multiple components:
> > +    - one or more IRS (Interrupt Routing Service)
> > +    - zero or more ITS (Interrupt Translation Service)
> > +    - zero or more IWB (Interrupt Wire Bridge)
> > +
> > +  The architecture defines:
> > +    - PE-Private Peripheral Interrupts (PPI)
> > +    - Shared Peripheral Interrupts (SPI)
> > +    - Logical Peripheral Interrupts (LPI)
> > +
> > +allOf:
> > +  - $ref: /schemas/interrupt-controller.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: arm,gic-v5
> > +
> > +  interrupt-controller: true
> > +
> > +  "#address-cells":
> > +    enum: [ 1, 2 ]
> 
> blank line
> 
> > +  "#size-cells":
> > +    enum: [ 1, 2 ]
> > +
> > +  ranges: true
> > +
> > +  "#interrupt-cells":
> > +    description: |
> > +      Specifies the number of cells needed to encode an interrupt source.
> > +      Must be a single cell with a value 3.
> 
> Drop this paragraph. The first sentence is just describing a common
> property. The 2nd is expressed as schema already.

Will do.

> > +
> > +      The 1st cell corresponds to the INTID.Type field in the INTID; 1 for PPI,
> > +      3 for SPI. LPI interrupts must not be described in the bindings since
> > +      they are allocated dynamically by the software component managing them.
> > +
> > +      The 2nd cell contains the interrupt INTID.ID field.
> > +
> > +      The 3rd cell is the flags, encoded as follows:
> > +      bits[3:0] trigger type and level flags.
> > +
> > +        1 = low-to-high edge triggered
> > +        2 = high-to-low edge triggered
> > +        4 = active high level-sensitive
> > +        8 = active low level-sensitive
> > +
> > +      Cells 4 and beyond are reserved for future use and must have a value
> > +      of 0 if present.
> 
> Drop. You can't have 4 or more cells because only 3 is allowed:

Nothing planned but what if this needs to be extended later ?

> > +    const: 3
> > +
> > +  interrupts:
> > +    description:
> > +      Interrupt source of the VGIC maintenance interrupt.
> 
> Drop "Interrupt source of ".
> 
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +
> > +patternProperties:
> > +  "^irs@[0-9a-f]+$":
> > +    type: object
> > +    description:
> > +      GICv5 has one or more Interrupt Routing Services (IRS) that are
> > +      responsible for handling IRQ state and routing.
> > +
> > +    additionalProperties: false
> 
> blank line
> 
> > +    properties:
> > +      compatible:
> > +        const: arm,gic-v5-irs
> > +
> > +      "#address-cells":
> > +        enum: [ 1, 2 ]
> 
> blank line
> 
> > +      "#size-cells":
> > +        enum: [ 1, 2 ]
> > +
> > +      ranges: true
> > +
> > +      dma-noncoherent:
> > +        description:
> > +          Present if the GIC IRS permits programming shareability and
> > +          cacheability attributes but is connected to a non-coherent
> > +          downstream interconnect.
> > +
> > +      reg:
> > +        minItems: 1
> > +        items:
> > +          - description: IRS control frame

On this, is there a way to specify that this has a fixed size ?

> > +          - description: IRS setlpi frame
> > +
> > +      cpus:
> > +        $ref: /schemas/types.yaml#/definitions/phandle-array
> 
> Already has a type, drop.
> 
> > +        description:
> > +          Should be a list of phandles to CPU nodes (as described in
> > +          Documentation/devicetree/bindings/arm/cpus.yaml) corresponding to
> > +          CPUs managed by the IRS.
> 
> The actual cpu binding is outside the scope of this binding. Just
> 'CPUs managed by the IRS.' is enough.
> 
> Is there a maximum number of CPUs?

Yes it is reported in the IRS_IDR1 configuration frame register IAFFID_BITS
field.

Should I add anything to the bindings related to this ?

> > +
> > +      arm,iaffids:
> > +        $ref: /schemas/types.yaml#/definitions/uint16-array
> > +        description:
> > +          Should be a list of u16 values representing IAFFID IDs associated
> 
> The type says it is 'a list of u16 values', so don't repeat that here.
> IAFFID needs to be defined somewhere. Is the 2nd 'ID' redundant?

I will add an IAFFID description (here ? or in the binding description
above ?)

> > +          with the CPU whose CPU node phandle is at the same index in the
> > +          cpus array.
> > +
> > +    patternProperties:
> > +      "^msi-controller@[0-9a-f]+$":
> > +        type: object
> > +        description:
> > +          GICv5 has zero or more Interrupt Translation Services (ITS) that are
> > +          used to route Message Signalled Interrupts (MSI) to the CPUs. Each
> > +          ITS is connected to an IRS.
> > +        additionalProperties: false
> 
> blank line
> 
> > +        properties:
> > +          compatible:
> > +            const: arm,gic-v5-its
> > +
> > +          dma-noncoherent:
> > +            description:
> > +              Present if the GIC ITS permits programming shareability and
> > +              cacheability attributes but is connected to a non-coherent
> > +              downstream interconnect.
> > +
> > +          msi-controller: true
> > +
> > +          "#msi-cells":
> > +            description:
> > +              The single msi-cell is the DeviceID of the device which will
> > +              generate the MSI.
> > +            const: 1
> > +
> > +          reg:
> > +            items:
> > +              - description: ITS control frame
> > +              - description: ITS translate frame
> > +
> > +        required:
> > +          - compatible
> > +          - msi-controller
> > +          - "#msi-cells"
> > +          - reg
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +      - cpus
> > +      - arm,iaffids
> > +
> > +  "^interrupt-controller@[0-9a-f]+$":
> > +    type: object
> > +    description:
> > +      GICv5 has zero or more Interrupt Wire Bridges (IWB) that are responsible
> > +      for translating wire signals into interrupt messages to the ITS.
> 
> I wonder if this should be a separate schema and not a child of the
> GIC? Seems like these would be implemented standalone (even if the
> arch doesn't define that) at arbitrary addresses that aren't within
> the GIC's address range. To put it another way, there's nothing here
> it is getting from the parent node.

I could move it to a separate schema even though I can't help thinking
that, by being a GICv5 *only* component, it is clearer for it to live
in this schema, that was my thinking when I drafted the bindings.

I feel like moving it to a different schema could give the wrong
impression, namely that an IWB can be plugged to something else
than an ITS, which is not really possible.

> > +
> > +    additionalProperties: false
> > +    properties:
> > +      compatible:
> > +        const: arm,gic-v5-iwb
> > +
> > +      interrupt-controller: true
> > +
> > +      "#address-cells":
> > +        const: 0
> > +
> > +      "#interrupt-cells":
> > +        description: |
> > +          Specifies the number of cells needed to encode an interrupt source.
> > +          Must be a single cell with a value 2.
> 
> Drop
> 
> > +
> > +          The 1st cell corresponds to the IWB wire.
> > +
> > +          The 2nd cell is the flags, encoded as follows:
> > +          bits[3:0] trigger type and level flags.
> > +
> > +          1 = low-to-high edge triggered
> > +          2 = high-to-low edge triggered
> > +          4 = active high level-sensitive
> > +          8 = active low level-sensitive
> > +
> > +          Cells 3 and beyond are reserved for future use and must have a value
> > +          of 0 if present.
> 
> Drop
> 
> > +        const: 2
> > +
> > +      reg:
> > +        items:
> > +          - description: IWB control frame
> > +
> > +      msi-parent: true
> 
> maxItems: 1
> 
> Because the common definition allows any number of parents.

I will add it.

> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +      - msi-parent
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    interrupt-controller {
> > +      compatible = "arm,gic-v5";
> > +      #interrupt-cells = <3>;
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +      ranges;
> > +
> > +      interrupt-controller;
> > +
> > +      interrupts = <1 25 4>;
> > +
> > +      irs@2f1a0000 {
> > +        compatible = "arm,gic-v5-irs";
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +        ranges;
> > +
> > +        reg = <0x0 0x2f1a0000 0x0 0x10000>;  // IRS_CONFIG_FRAME for NS
> > +
> > +        arm,iaffids = /bits 16 <0 1 2 3 4 5 6 7>;

/bits/

> > +        cpus = <&{/cpus/cpu@0}>, <&{/cpus/cpu@100}>, <&{/cpus/cpu@200}>,
> > +               <&{/cpus/cpu@300}>, <&{/cpus/cpu@10000}>, <&{/cpus/cpu@10100}>,
> > +               <&{/cpus/cpu@10200}>, <&{/cpus/cpu@10300}>;
> 
> Use labels instead of paths.

Yep, noticed, fixed already.

> > +
> > +        msi-controller@2f120000 {
> > +          compatible = "arm,gic-v5-its";
> > +
> > +          msi-controller;
> > +          #msi-cells = <1>;
> > +
> > +          reg = <0x0 0x2f120000 0x0 0x10000    // ITS_CONFIG_FRAME for NS
> > +                 0x0 0x2f130000 0x0 0x10000>;  // ITS_TRANSLATE_FRAME
> > +        };
> > +      };
> > +
> > +      interrupt-controller@2f000000 {
> > +        compatible = "arm,gic-v5-iwb";
> > +        #address-cells = <0>;
> > +
> > +        interrupt-controller;
> > +        #interrupt-cells = <2>;
> > +
> > +        reg = <0x0 0x2f000000 0x0 0x10000>;
> > +
> > +        msi-parent = <&its0 64>;
> > +      };
> > +    };
> > +
> > +    device@0 {
> 
> Drop. We don't put consumers in provider examples and vice-versa.

I will drop it.

Thanks a lot Rob for reviewing it.

Lorenzo

> > +      reg = <0 4>;
> > +      interrupts = <3 115 4>;
> > +    };
> > +
> > +...
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml
new file mode 100644
index 0000000000000000000000000000000000000000..5c78375c298a0115c55872f439eb04d4171c4381
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml
@@ -0,0 +1,268 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/arm,gic-v5.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ARM Generic Interrupt Controller, version 5
+
+maintainers:
+  - Lorenzo Pieralisi <lpieralisi@kernel.org>
+  - Marc Zyngier <maz@kernel.org>
+
+description: |
+  The GICv5 architecture defines the guidelines to implement GICv5
+  compliant interrupt controllers for AArch64 systems.
+
+  The GICv5 specification can be found at
+  https://developer.arm.com/documentation/aes0070
+
+  The GICv5 architecture is composed of multiple components:
+    - one or more IRS (Interrupt Routing Service)
+    - zero or more ITS (Interrupt Translation Service)
+    - zero or more IWB (Interrupt Wire Bridge)
+
+  The architecture defines:
+    - PE-Private Peripheral Interrupts (PPI)
+    - Shared Peripheral Interrupts (SPI)
+    - Logical Peripheral Interrupts (LPI)
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+properties:
+  compatible:
+    const: arm,gic-v5
+
+  interrupt-controller: true
+
+  "#address-cells":
+    enum: [ 1, 2 ]
+  "#size-cells":
+    enum: [ 1, 2 ]
+
+  ranges: true
+
+  "#interrupt-cells":
+    description: |
+      Specifies the number of cells needed to encode an interrupt source.
+      Must be a single cell with a value 3.
+
+      The 1st cell corresponds to the INTID.Type field in the INTID; 1 for PPI,
+      3 for SPI. LPI interrupts must not be described in the bindings since
+      they are allocated dynamically by the software component managing them.
+
+      The 2nd cell contains the interrupt INTID.ID field.
+
+      The 3rd cell is the flags, encoded as follows:
+      bits[3:0] trigger type and level flags.
+
+        1 = low-to-high edge triggered
+        2 = high-to-low edge triggered
+        4 = active high level-sensitive
+        8 = active low level-sensitive
+
+      Cells 4 and beyond are reserved for future use and must have a value
+      of 0 if present.
+    const: 3
+
+  interrupts:
+    description:
+      Interrupt source of the VGIC maintenance interrupt.
+    maxItems: 1
+
+required:
+  - compatible
+
+patternProperties:
+  "^irs@[0-9a-f]+$":
+    type: object
+    description:
+      GICv5 has one or more Interrupt Routing Services (IRS) that are
+      responsible for handling IRQ state and routing.
+
+    additionalProperties: false
+    properties:
+      compatible:
+        const: arm,gic-v5-irs
+
+      "#address-cells":
+        enum: [ 1, 2 ]
+      "#size-cells":
+        enum: [ 1, 2 ]
+
+      ranges: true
+
+      dma-noncoherent:
+        description:
+          Present if the GIC IRS permits programming shareability and
+          cacheability attributes but is connected to a non-coherent
+          downstream interconnect.
+
+      reg:
+        minItems: 1
+        items:
+          - description: IRS control frame
+          - description: IRS setlpi frame
+
+      cpus:
+        $ref: /schemas/types.yaml#/definitions/phandle-array
+        description:
+          Should be a list of phandles to CPU nodes (as described in
+          Documentation/devicetree/bindings/arm/cpus.yaml) corresponding to
+          CPUs managed by the IRS.
+
+      arm,iaffids:
+        $ref: /schemas/types.yaml#/definitions/uint16-array
+        description:
+          Should be a list of u16 values representing IAFFID IDs associated
+          with the CPU whose CPU node phandle is at the same index in the
+          cpus array.
+
+    patternProperties:
+      "^msi-controller@[0-9a-f]+$":
+        type: object
+        description:
+          GICv5 has zero or more Interrupt Translation Services (ITS) that are
+          used to route Message Signalled Interrupts (MSI) to the CPUs. Each
+          ITS is connected to an IRS.
+        additionalProperties: false
+        properties:
+          compatible:
+            const: arm,gic-v5-its
+
+          dma-noncoherent:
+            description:
+              Present if the GIC ITS permits programming shareability and
+              cacheability attributes but is connected to a non-coherent
+              downstream interconnect.
+
+          msi-controller: true
+
+          "#msi-cells":
+            description:
+              The single msi-cell is the DeviceID of the device which will
+              generate the MSI.
+            const: 1
+
+          reg:
+            items:
+              - description: ITS control frame
+              - description: ITS translate frame
+
+        required:
+          - compatible
+          - msi-controller
+          - "#msi-cells"
+          - reg
+
+    required:
+      - compatible
+      - reg
+      - cpus
+      - arm,iaffids
+
+  "^interrupt-controller@[0-9a-f]+$":
+    type: object
+    description:
+      GICv5 has zero or more Interrupt Wire Bridges (IWB) that are responsible
+      for translating wire signals into interrupt messages to the ITS.
+
+    additionalProperties: false
+    properties:
+      compatible:
+        const: arm,gic-v5-iwb
+
+      interrupt-controller: true
+
+      "#address-cells":
+        const: 0
+
+      "#interrupt-cells":
+        description: |
+          Specifies the number of cells needed to encode an interrupt source.
+          Must be a single cell with a value 2.
+
+          The 1st cell corresponds to the IWB wire.
+
+          The 2nd cell is the flags, encoded as follows:
+          bits[3:0] trigger type and level flags.
+
+          1 = low-to-high edge triggered
+          2 = high-to-low edge triggered
+          4 = active high level-sensitive
+          8 = active low level-sensitive
+
+          Cells 3 and beyond are reserved for future use and must have a value
+          of 0 if present.
+        const: 2
+
+      reg:
+        items:
+          - description: IWB control frame
+
+      msi-parent: true
+
+    required:
+      - compatible
+      - reg
+      - msi-parent
+
+additionalProperties: false
+
+examples:
+  - |
+    interrupt-controller {
+      compatible = "arm,gic-v5";
+      #interrupt-cells = <3>;
+      #address-cells = <2>;
+      #size-cells = <2>;
+      ranges;
+
+      interrupt-controller;
+
+      interrupts = <1 25 4>;
+
+      irs@2f1a0000 {
+        compatible = "arm,gic-v5-irs";
+        #address-cells = <2>;
+        #size-cells = <2>;
+        ranges;
+
+        reg = <0x0 0x2f1a0000 0x0 0x10000>;  // IRS_CONFIG_FRAME for NS
+
+        arm,iaffids = /bits 16 <0 1 2 3 4 5 6 7>;
+        cpus = <&{/cpus/cpu@0}>, <&{/cpus/cpu@100}>, <&{/cpus/cpu@200}>,
+               <&{/cpus/cpu@300}>, <&{/cpus/cpu@10000}>, <&{/cpus/cpu@10100}>,
+               <&{/cpus/cpu@10200}>, <&{/cpus/cpu@10300}>;
+
+        msi-controller@2f120000 {
+          compatible = "arm,gic-v5-its";
+
+          msi-controller;
+          #msi-cells = <1>;
+
+          reg = <0x0 0x2f120000 0x0 0x10000    // ITS_CONFIG_FRAME for NS
+                 0x0 0x2f130000 0x0 0x10000>;  // ITS_TRANSLATE_FRAME
+        };
+      };
+
+      interrupt-controller@2f000000 {
+        compatible = "arm,gic-v5-iwb";
+        #address-cells = <0>;
+
+        interrupt-controller;
+        #interrupt-cells = <2>;
+
+        reg = <0x0 0x2f000000 0x0 0x10000>;
+
+        msi-parent = <&its0 64>;
+      };
+    };
+
+    device@0 {
+      reg = <0 4>;
+      interrupts = <3 115 4>;
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 96b82704950184bd71623ff41fc4df31e4c7fe87..f3ed84466da19906953b5396a5f4b50e878c376e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1901,6 +1901,13 @@  F:	drivers/irqchip/irq-gic*.[ch]
 F:	include/linux/irqchip/arm-gic*.h
 F:	include/linux/irqchip/arm-vgic-info.h
 
+ARM GENERIC INTERRUPT CONTROLLER V5 DRIVERS
+M:	Lorenzo Pieralisi <lpieralisi@kernel.org>
+M:	Marc Zyngier <maz@kernel.org>
+L:	linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/interrupt-controller/arm,gic-v5.yaml
+
 ARM HDLCD DRM DRIVER
 M:	Liviu Dudau <liviu.dudau@arm.com>
 S:	Supported