Re: [PATCH v3 1/9] dt-binding: soc: xilinx: ai-engine: Add AI engine binding

[Jiaying Liang <wendy.liang@xilinx.com>]

On 12/8/20 3:41 PM, Rob Herring wrote:
> On Sun, Nov 29, 2020 at 11:48:17PM -0800, Wendy Liang wrote:
>> Xilinx AI engine array can be partitioned statically for different
>> applications. In the device tree, there will be device node for the AI
>> engine device, and device nodes for the statically configured AI engine
>> partitions. Each of the statically configured partition has a partition
>> ID in the system.
>>
>> Signed-off-by: Wendy Liang <wendy.liang@xilinx.com>
>> ---
>>   .../bindings/soc/xilinx/xlnx,ai-engine.yaml        | 126 +++++++++++++++++++++
>>   1 file changed, 126 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 0000000..1de5623
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/xilinx/xlnx,ai-engine.yaml
>> @@ -0,0 +1,126 @@
>> +# 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: Xilinx AI Engine
>> +
>> +maintainers:
>> +  - Wendy Liang <wendy.liang@xilinx.com>
>> +
>> +description: |+
> You don't need '|' unless there's formatting to preserve.
Will change
>
>> +  The Xilinx 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.
>> +
>> +properties:
>> +  compatible:
>> +    const: xlnx,ai-engine-v1.0
> This is soft logic? If not, don't use version numbers.

It is not soft logic, if there is a future version of the device, can we 
use version number

in compatible to describe the device version?

>
>> +
>> +  reg:
>> +    description: |
>> +      Physical base address and length of the device registers.
> That's every 'reg' property. Drop.
[Wendy] will drop it.
>
>> +      The AI engine address space assigned to Linux is defined by Xilinx
>> +      platform design tool.
>> +
>> +  '#address-cells':
>> +    enum: [2]
> const: 2
Will change
>
>> +    description: |
>> +      size of cell to describe AI engine range of tiles address.
>> +      It is the location of the starting tile of the range.
>> +      As the AI engine tiles are 2D array, the location of a tile
>> +      is presented as (column, row), the address cell is 2.
>> +
>> +  '#size-cells':
>> +    enum: [2]
>> +    description: |
>> +      size of cell to describe AI engine range of tiles size.
>> +      As the AI engine tiles are 2D array, the size cell is 2.
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +    description: phandle to the associated power domain
>> +
>> +  interrupts:
>> +    maxItems: 3
>> +
>> +  interrupt-names:
>> +    description: |
>> +      Should be "interrupt1", "interrupt2" or "interrupt3".
> Really, not useful names. If you do have names, they should be a schema,
> not freeform text.
>
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - '#address-cells'
>> +  - '#size-cells'
>> +  - power-domains
>> +  - interrupt-parent
> Generally, never required because it could be in the parent node.
will remove
>
>> +  - interrupts
>> +  - interrupt-names
>> +
>> +patternProperties:
>> +  "^aie_partition@[0-9]+$":
> aie-partition@
>
> The unit-address is just the 1st cell of reg (the row)? Or needs to be
> row and column, in which case you'd want something like '@0,0'. Also,
> unit-address values are typically hex, not decimal.
It will be col,row, will change to address format with starting column 
and row
>
>> +    type: object
>> +    description: |
>> +      AI engine partition which is a group of column based tiles of the AI
>> +      engine device. Each AI engine partition is isolated from the other
>> +      AI engine partitions. An AI engine partition is defined by Xilinx
>> +      platform design tools. Each partition has a SHIM row and core tiles rows.
>> +      A SHIM row contains SHIM tiles which are the interface to external
>> +      components. AXI master can access AI engine registers, push data to and
>> +      fetch data from AI engine through the SHIM tiles. Core tiles are the
>> +      compute tiles.
>> +
>> +    properties:
>> +      reg:
>> +        description: |
>> +          It describes the group of tiles of the AI engine partition. It needs
>> +          to include the SHIM row. The format is defined by the parent AI engine
>> +          device node's '#address-cells' and '#size-cells' properties. e.g. a v1
>> +          AI engine device has 2D tiles array, the first row is SHIM row. A
>> +          partition which has 50 columns and 8 rows of core tiles and 1 row of
>> +          SHIM tiles will be presented as <0 0 50 9>.
> You should be able to write some constraints like max row and column
> values?

The max row and columns depends on the underline hardware platform, the 
driver can

get the max allowed row and columns from the size field of the "reg" 
property in the parent

AI engine device node.

>
>> +
>> +      label:
>> +        maxItems: 1
> 'label' is not an array. Why do you need label?
>
>> +
>> +      xlnx,partition-id:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        description: |
>> +          AI engine partition ID, which is defined by Xilinx platform design
>> +          tool to identify the AI engine partition in the system.
> I find the use of 'reg' a bit odd here. Maybe using 'reg' for partition
> would make more sense? Which is more closely associated with how you
> address the partition?

I am not clear on this comments, are you referring to partition-id?

The partition id is generated by Xilinx tool to identify the AI engine 
partition in the firmware.

The "reg" of the partition device node is to describe the location of 
the partition within

the AI engine device node.


Thanks,

Wendy

>
>> +
>> +    required:
>> +      - reg
>> +      - xlnx,partition-id
>> +    additionalProperties: false
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    bus {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +
>> +      ai_engine: ai-engine@20000000000 {
>> +        compatible = "xlnx,ai-engine-v1.0";
>> +        reg = <0x200 0x0 0x1 0x0>;
>> +        #address-cells = <2>;
>> +        #size-cells = <2>;
>> +        power-domains = <&versal_firmware 0x18224072>;
>> +        interrupt-parent = <&gic>;
>> +        interrupts = <0x0 0x94 0x4>,
>> +                     <0x0 0x95 0x4>,
>> +                     <0x0 0x96 0x4>;
>> +        interrupt-names = "interrupt1", "interrupt2", "interrupt3";
>> +
>> +        aie_partition0: aie_partition@0 {
>> +                /* 50 columns and 8 core tile rows + 1 SHIM row */
>> +                reg = <0 0 50 9>;
>> +                xlnx,partition-id = <1>;
>> +        };
>> +      };
>> +    };
>> -- 
>> 2.7.4
>>