[PATCH V1 4/9] dt-bindings: soc: xilinx: Add AI engine DT binding

[Williams, Gregory]

On 7/3/2025 12:48 AM, Krzysztof Kozlowski wrote:
> On 02/07/2025 17:56, Gregory Williams wrote:
>> In the device tree, there will be device node for the AI engine device,
>> and device nodes for the statically configured AI engine apertures.
> 
> No, describe the hardware, not DTS.
> 
>> Apertures are an isolated set of columns with in the AI engine device
>> with their own address space and interrupt.
>>
>> Signed-off-by: Gregory Williams <gregory.williams at amd.com>
>> ---
>>  .../bindings/soc/xilinx/xlnx,ai-engine.yaml   | 151 ++++++++++++++++++
>>  1 file changed, 151 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/soc/xilinx/xlnx,ai-engine.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/soc/xilinx/xlnx,ai-engine.yaml b/Documentation/devicetree/bindings/soc/xilinx/xlnx,ai-engine.yaml
>> new file mode 100644
>> index 000000000000..7d9a36c56366
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/xilinx/xlnx,ai-engine.yaml
> 
> Filename matching compatible.
> 
>> @@ -0,0 +1,151 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/soc/xilinx/xlnx,ai-engine.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: AMD AI Engine
> 
> That's really too generic...
> 
>> +
>> +maintainers:
>> +  - Gregory Williams <gregory.williams at amd.com>
>> +
>> +description:
>> +  The AMD AI Engine is a tile processor with many cores (up to 400) that
>> +  can run in parallel. The data routing between cores is configured through
>> +  internal switches, and shim tiles interface with external interconnect, such
>> +  as memory or PL. One AI engine device can have multiple apertures, each
>> +  has its own address space and interrupt. At runtime application can create
>> +  multiple partitions within an aperture which are groups of columns of AI
>> +  engine tiles. Each AI engine partition is the minimum resetable unit for an
>> +  AI engine application.
>> +
>> +properties:
>> +  compatible:
>> +    const: xlnx,ai-engine-v2.0
> 
> What does v2.0 stands for? Versioning is discouraged, unless mapping is
> well documented.

Sure, I will remove the versioning in V2 patch.

> 
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  '#address-cells':
>> +    const: 2
>> +
>> +  '#size-cells':
>> +    const: 2
>> +
>> +  power-domains:
> 
> Missing constraints.
> 
>> +    description:
>> +      Platform management node id used to request power management services
>> +      from the firmware driver.
> 
> Drop description, redundant.
> 
>> +
>> +  xlnx,aie-gen:
>> +    $ref: /schemas/types.yaml#/definitions/uint8
> 
> Why uint8?
> 
>> +    description:
>> +      Hardware generation of AI engine device. E.g. the current values supported
>> +      are 1 (AIE) and 2 (AIEML).
> 
> No clue what's that, but it is implied by compatible, isn't it?

The driver supports multiple hardware generations. During driver probe, this value is read from the device tree and hardware generation specific
data structures are loaded based on this value. The compatible string is the same between devices.

> 
> Missing constraints.
> 
>> +
>> +  xlnx,shim-rows:
>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>> +    description:
>> +      start row and the number of rows of SHIM tiles of the AI engine device
> 
> Implied by compatible.

The AI Engine device can have different configurations for number of rows and column (even if it is the same hardware generation). This property
tells the driver the size and layout of the array, this is not implied by compatible.

> 
> Missing constraints.
> 
> 
>> +
>> +  xlnx,core-rows:
>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>> +    description:
>> +      start row and the number of rows of core tiles of the AI engine device
>> +
>> +  xlnx,mem-rows:
>> +    $ref: /schemas/types.yaml#/definitions/uint8-array
>> +    description:
>> +      start row and the number of rows of memory tiles of the AI engine device
>> +
> 
> Same comments everywhere.
> 
>> +required:
>> +  - compatible
>> +  - reg
>> +  - power-domains
>> +  - xlnx,aie-gen
>> +  - xlnx,shim-rows
>> +  - xlnx,core-rows
>> +  - xlnx,mem-rows
>> +
>> +patternProperties:
> 
> This goes after properties.
> 
>> +  "^aperture@[0-9]+$":
>> +    type: object
>> +    description:
>> +      AI engine aperture which is a group of column based tiles of the
>> +      AI engine device. Each AI engine apertures isolated from the
>> +      other AI engine apertures. An AI engine aperture is defined by
>> +      AMD/Xilinx platform design tools.
>> +
>> +    properties:
>> +      compatible:
>> +        const: xlnx,ai-engine-aperture
>> +
>> +      reg:
>> +        description:
>> +          Physical base address and length of the aperture registers.
>> +          The AI engine address space assigned to Linux is defined by
>> +          Xilinx/AMD platform design tool.
> 
> Missing constraints. Description is redundant - can it be anything else?
> 
> Plus you clearly miss ranges.
> 
> 
>> +
>> +      interrupts:
>> +        maxItems: 3
>> +
>> +      interrupt-names:
>> +        items:
>> +          - const: interrupt1
>> +          - const: interrupt2
>> +          - const: interrupt3
> 
> Useless names, drop entirely.
> 
>> +
>> +      xlnx,columns:
>> +        $ref: /schemas/types.yaml#/definitions/uint32-array
>> +        description:
>> +          It describes the location of the aperture. It specifies the start
>> +          column and the number of columns. E.g. an aperture starts from
>> +          column 0 and there are 50 columns, it will be presented as <0 50>.
> 
> Same comments as before
> 
>> +
>> +      xlnx,node-id:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        description:
>> +          AI engine aperture node ID, which is defined by AMD/Xilinx platform
>> +          design tool to identify the AI engine aperture in the firmware.
> 
> No, you do not get node ID. Recently every day a patch comes for that...
> 
>> +
>> +    required:
>> +      - compatible
>> +      - reg
>> +      - xlnx,columns
>> +      - xlnx,node-id
>> +
>> +    additionalProperties: false
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/power/xlnx-versal-power.h>
>> +    bus {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +      ai_engine: ai-engine at 20000000000 {
>> +        compatible = "xlnx,ai-engine-v2.0";
>> +        reg = <0x200 0x00 0x01 0x00>;
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +        power-domains = <&versal_firmware PM_DEV_AI>;
>> +        xlnx,aie-gen = /bits/ 8 <0x1>;
>> +        xlnx,core-rows = /bits/ 8 <1 8>;
>> +        xlnx,mem-rows = /bits/ 8 <0 0>;
>> +        xlnx,shim-rows = /bits/ 8 <0 1>
> 
> This cannot be without ranges... I am surprised it actually works, but
> for sure was not tested and produces warnings.
> 
>> +
>> +        aperture0: aperture at 200000000000 {
>> +          /* 50 columns and 8 core tile rows + 1 SHIM row */
>> +          compatible = "xlnx,ai-engine-aperture";
>> +          reg = <0x200 0x0 0x1 0x0>;
>> +          interrupts = <0x0 0x94 0x4>,
>> +                       <0x0 0x95 0x4>,
>> +                       <0x0 0x96 0x4>;
> Use proper flags.
> 
> Best regards,
> Krzysztof

Thanks again for the review Krzysztof, I appreciate your time. I will address the remaining comments in a V2 patch.

Thanks,
Greg