Re: [PATCH 1/6] dt-bindings: iio: xilinx: Add Documentation for Sysmon

[Michal Simek <michal.simek@amd.com>]

On 9/5/25 13:30, Jonathan Cameron wrote:
> On Fri, 5 Sep 2025 10:41:44 +0200
> Michal Simek <michal.simek@amd.com> wrote:
> 
>> From: Salih Erim <salih.erim@amd.com>
>>
>> Add devicetree documentation for Xilinx Sysmon IP which is used for
>> internal chip monitoring on Xilinx Versal SOCs.
>>
>> Co-developed-by: Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com>
>> Signed-off-by: Anand Ashok Dumbre <anand.ashok.dumbre@xilinx.com>
>> Co-developed-by: Anish Kadamathikuttiyil Karthikeyan Pillai <anish.kadamathikuttiyil-karthikeyan-pillai@amd.com>
>> Signed-off-by: Anish Kadamathikuttiyil Karthikeyan Pillai <anish.kadamathikuttiyil-karthikeyan-pillai@amd.com>
>> Signed-off-by: Salih Erim <salih.erim@amd.com>
>> Signed-off-by: Michal Simek <michal.simek@amd.com>
>> ---
>>
>>   .../bindings/iio/adc/xlnx,versal-sysmon.yaml  | 235 ++++++++++++++++++
>>   1 file changed, 235 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml b/Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
>> new file mode 100644
>> index 000000000000..a768395cade7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/xlnx,versal-sysmon.yaml
>> @@ -0,0 +1,235 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iio/adc/xlnx,versal-sysmon.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Xilinx Versal Sysmon
>> +
>> +maintainers:
>> +  - Salih Erim <salih.erim@amd.com>
>> +
>> +description:
>> +  The Xilinx Sysmon provides on-chip monitoring and control for the supply
>> +  voltages and temperatures across the chip. Since there are only 160 supply
>> +  voltage registers and 184 measurement points, there is no constant mapping
>> +  of supply voltage registers and the measurement points. User has to select
>> +  the voltages to monitor in design tool. Depending on the selection, a voltage
>> +  supply gets mapped to one of the supply registers. So, this mapping information
>> +  is provided via description which contain the information of name of
>> +   the supply enabled and the supply register it maps to.
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - const: xlnx,versal-sysmon
>> +
>> +  reg:
>> +    maxItems: 1
>> +    description: Sysmon Registers.
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +    description: Interrupt line for Sysmon.
>> +
>> +  '#address-cells':
>> +    const: 1
>> +
>> +  '#size-cells':
>> +    const: 0
>> +
>> +  '#io-channel-cells':
>> +    const: 0
>> +
>> +  xlnx,hbm:
>> +    type: boolean
>> +    description:
>> +      Exists if node refers to a HBM (High Bandwidth Memory) SLR (Super Logic Region).
>> +
>> +  xlnx,nodeid:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    description:
>> +      PLM specified sysmon node id.
>> +
>> +  xlnx,numaiechannels:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    maximum: 64
>> +    description:
>> +      Total number of sysmon satellites close to AI Engine exposed as channels.
> 
> Feels like some use - would make this easier to parse.  xlnx,num-aie-channels.
> Similar to the next one. How is this related to the number of child nodes?

it is number of childs below. They can be calculated to get this number.

> 
> 
>> +
>> +  xlnx,numchannels:
>> +    $ref: /schemas/types.yaml#/definitions/uint32
>> +    minimum: 1
>> +    maximum: 160
>> +    description:
>> +      Number of supply channels enabled in the design.
> 
> Given you have subnodes called supplyxxx why is a count
> of those needed or is this not counting those?

possible.


> 
>> +
>> +patternProperties:
>> +  "^supply@([0-9]{1,2}|1[0-5][0-9])$":
>> +    type: object
>> +    description:
>> +      Represents the supplies configured in the design.
>> +
>> +    properties:
>> +      reg:
>> +        $ref: /schemas/types.yaml#/definitions/uint32
>> +        minimum: 0
>> +        maximum: 159
>> +        description:
>> +          The supply number associated with the voltage.
>> +
>> +      xlnx,name:
>> +        $ref: /schemas/types.yaml#/definitions/string
>> +        description:
>> +          Name of the supply enabled
> 
> Would the generic property "label" be useable here?

label should be fine.


> 
>> +
>> +      xlnx,bipolar:
>> +        $ref: /schemas/types.yaml#/definitions/flag
>> +        description:
>> +          If the supply has a bipolar type and the output will be signed.
> 
> This is very generic.  We have it described for ADC channels already in
> bindings/iio/adc/adc.yaml.  Why can't we use that here?

no issue with it.
And likely
Documentation/devicetree/bindings/iio/adc/xlnx,zynqmp-ams.yaml
should deprecated it and start to use new one.



> That binding does rely on matching against 'channel' for node names though.
> Where a 'type of channel' has been relevant IIRC we've always added
> a separate property rather than using the child node name.

Is this related to supply/temp channel name?

I think one issue with the binding is that current schema allows to define
supply@1  and also temp@1
but both of them have reg = <1> which is not allowed (duplicate unit-address).

Salih: What does this reg value means? Is it physical address where that sensor 
is placed?

> 
>> +
>> +    required:
>> +      - reg
>> +      - xlnx,name
>> +
>> +    additionalProperties: false
>> +
>> +  "^temp@([1-9]|[1-5][0-9]|6[0-4])$":
>> +    type: object
>> +    description:
>> +      Represents the sysmon temperature satellites.
>> +
>> +    properties:
>> +      reg:
>> +        minimum: 1
>> +        maximum: 64
>> +        description:
>> +          The sysmon temperature satellite number.
>> +
>> +      xlnx,aie-temp:
>> +        $ref: /schemas/types.yaml#/definitions/flag
>> +        description:
>> +          If present it indicates the temperature satellite is in
>> +          close proximity with AI Engine
> 
> This one seems unusual.  I guess it makes a configuration difference
> of some type.  I'll look at the code to see if that answers the question.

it is supposed to be identify location of this sensor.

> 
>> +
>> +      xlnx,name:
>> +        $ref: /schemas/types.yaml#/definitions/string
>> +        description:
>> +          Name of temperature satellite exposed
> 
> As above. label tends to get used for things like this.

no issue with this.
>> +
>> +    required:
>> +      - reg
>> +      - xlnx,name
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - xlnx,numchannels
>> +
>> +additionalProperties: false
> 
Thanks,
Michal