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

["Williams, Gregory" <gregoryw@amd.com>]

On 7/10/2025 3:38 PM, Krzysztof Kozlowski wrote:
> 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@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.

No, they were not ignored. I will make sure to address in a V2 patch.

> 
>>>
>>>> +
>>>> +maintainers:
>>>> +  - Gregory Williams <gregory.williams@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.

Yes this is part of an SoC. I will be more descriptive 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
> 
> Bindings are about hardware, not driver, so your driver arguments are
> not valid.

Understood.

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

Ok so there should be a different compatible strings based on hardware
generation. I will fix this for a V2 patch.

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

I see in 'writing bindings' that I should use device-based compatible
string. I will do this and remove these nodes for V2 patch. 

Thanks again for your time,
Greg

> 
> 
> Best regards,
> Krzysztof