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

[Krzysztof Kozlowski]

On 10/07/2025 21:03, Williams, Gregory wrote:
> 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...

You did not answer to other comments here and other patches, so I just
assume you did not ignore them.

>>
>>> +
>>> +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.

This should be specific to product, so use the actual product/model name.

Is this part of a Soc? Then standard rules apply... but I could not
deduce it from the descriptions or commit msgs.


> 
>>
>>> +
>>> +  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

Bindings are about hardware, not driver, so your driver arguments are
not valid.

> data structures are loaded based on this value. The compatible string is the same between devices.

No. See writing bindings.

> 
>>
>> 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.

Wrap your emails correctly.

Again driver.. no, please describe the hardware, not your drivers.


Best regards,
Krzysztof