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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <504f6660-4938-47b4-b1db-0a6fe0214e5f@kernel.org>
Date: Thu, 10 Jul 2025 23:38:26 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: "Williams, Gregory" <gregoryw@....com>,
 Gregory Williams <gregory.williams@....com>, ogabbay@...nel.org,
 michal.simek@....com, robh@...nel.org
Cc: dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH V1 4/9] dt-bindings: soc: xilinx: Add AI engine DT binding

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