Re: [PATCH v2 3/3] dt-bindings: thermal: Add yaml bindings for thermal zones

From: Rob Herring
Date: Wed Mar 11 2020 - 10:49:42 EST

On Thu, Mar 05, 2020 at 06:26:43PM +0530, Amit Kucheria wrote:
> As part of moving the thermal bindings to YAML, split it up into 3
> bindings: thermal sensors, cooling devices and thermal zones.
>
> The thermal-zone binding is a software abstraction to capture the
> properties of each zone - how often they should be checked, the
> temperature thresholds (trips) at which mitigation actions need to be
> taken and the level of mitigation needed at those thresholds.
>
> Signed-off-by: Amit Kucheria <amit.kucheria@xxxxxxxxxx>
> ---
> .../bindings/thermal/thermal-zones.yaml | 325 ++++++++++++++++++
> 1 file changed, 325 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/thermal/thermal-zones.yaml
>
> diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> new file mode 100644
> index 0000000000000..f8f3b72bc3119
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml
> @@ -0,0 +1,325 @@
> +# SPDX-License-Identifier: (GPL-2.0)
> +# Copyright 2020 Linaro Ltd.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/thermal/thermal-zones.yaml#
> +$schema: http://devicetree.org/meta-schemas/base.yaml#
> +
> +title: Thermal zone binding
> +
> +maintainers:
> + - Amit Kucheria <amitk@xxxxxxxxxx>
> +
> +description: |
> + Thermal management is achieved in devicetree by describing the sensor hardware
> + and the software abstraction of cooling devices and thermal zones required to
> + take appropriate action to mitigate thermal overloads.
> +
> + The following node types are used to completely describe a thermal management
> + system in devicetree:
> + - thermal-sensor: device that measures temperature, has SoC-specific bindings
> + - cooling-device: device used to dissipate heat either passively or actively
> + - thermal-zones: a container of the following node types used to describe all
> + thermal data for the platform
> +
> + This binding describes the thermal-zones.
> +
> + The polling-delay properties of a thermal-zone are bound to the maximum dT/dt
> + (temperature derivative over time) in two situations for a thermal zone:
> + 1. when passive cooling is activated (polling-delay-passive)
> + 2. when the zone just needs to be monitored (polling-delay) or when
> + active cooling is activated.
> +
> + The maximum dT/dt is highly bound to hardware power consumption and
> + dissipation capability. The delays should be chosen to account for said
> + max dT/dt, such that a device does not cross several trip boundaries
> + unexpectedly between polls. Choosing the right polling delays shall avoid
> + having the device in temperature ranges that may damage the silicon structures
> + and reduce silicon lifetime.
> +
> +properties:
> + thermal-zones:
> + type: object
> + description:
> + A /thermal-zones node is required in order to use the thermal framework to
> + manage input from the various thermal zones in the system in order to
> + mitigate thermal overload conditions. It does not represent a real device
> + in the system, but acts as a container to link thermal sensor devices,
> + platform-data regarding temperature thresholds and the mitigation actions
> + to take when the temperature crosses those thresholds.
> +

> + properties:
> + $nodename:
> + pattern: "^[a-zA-Z][a-zA-Z0-9,\\-]{1,12}-thermal$"

These 3 lines should be:

patternProperties:
"^[a-zA-Z][a-zA-Z0-9,\\-]{1,12}-thermal$":

Though we should drop ',' as well.

$nodename is only needed at the top level where it's kind of special.

> + type: object
> + description:
> + Each thermal zone node contains information about how frequently it
> + must be checked, the sensor responsible for reporting temperature for
> + this zone, one sub-node containing the various trip points for this
> + zone and one sub-node containing all the zone cooling-maps.
> +
> + properties:
> + polling-delay:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0

The type is unsigned, so the min is already 0.

> + description:
> + The maximum number of milliseconds to wait between polls when
> + checking this thermal zone. Setting this to 0 disables the polling
> + timers setup by the thermal framework and assumes that the thermal
> + sensors in this zone support interrupts.
> +
> + polling-delay-passive:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
> + description:
> + The maximum number of milliseconds to wait between polls when
> + checking this thermal zone while doing passive cooling. Setting
> + this to 0 disables the polling timers setup by the thermal
> + framework and assumes that the thermal sensors in this zone
> + support interrupts.
> +
> + thermal-sensors:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + description:
> + A list of thermal sensor phandles and sensor specifiers used to
> + monitor this thermal zone.
> +
> + trips:
> + type: object
> + description:
> + This node describes a set of points in the temperature domain at
> + which the thermal framework needs to takes action. The actions to
> + be taken are defined in another node called cooling-maps.
> +
> + patternProperties:
> + "^[a-zA-Z][a-zA-Z0-9,+\\._]{0,63}$":

Drop ',', '+', '.', and ideally '_'. Probably need to add '-'.

> + type: object
> +
> + properties:
> + temperature:
> + $ref: /schemas/types.yaml#/definitions/int32
> + minimum: -273000
> + maximum: 200000
> + description:
> + An integer expressing the trip temperature in millicelsius.
> +
> + hysteresis:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + An unsigned integer expressing the hysteresis delta with
> + respect to the trip temperature property above, also in
> + millicelsius.
> +
> + type:

Needs a type reference (string).

> + enum:
> + # active: enable active cooling e.g. fans
> + - active

Wrong indentation. Should be 2 less.

Also, I think this would be more readable:

- active # enable active cooling e.g. fans


> + # passive: enable passive cooling e.g. throttling cpu
> + - passive
> + # hot: send notification to driver if .notify
> + # callback registered
> + - hot
> + # critical: send notification to driver if .notify
> + # callback registered and trigger a shutdown
> + - critical
> + description: |
> + There are four valid trip types: active, passive, hot,
> + critical.
> +
> + The critical trip type is used to set the maximum
> + temperature threshold above which the HW becomes
> + unstable and underlying firmware might even trigger a
> + reboot. Hitting the critical threshold triggers a system
> + shutdown.
> +
> + The hot trip type can be used to send a notification to
> + the thermal driver (if a .notify callback is registered).
> + The action to be taken is left to the driver.
> +
> + The passive trip type can be used to slow down HW e.g. run
> + the CPU, GPU, bus at a lower frequency.
> +
> + The active trip type can be used to control other HW to
> + help in cooling e.g. fans can be sped up or slowed down
> +
> + required:
> + - temperature
> + - hysteresis
> + - type
> +
> + additionalProperties: false

You need 'additionalProperties' on the child nodes too, or are there
vendor specific properties allowed?

> +
> + cooling-maps:
> + type: object
> + description:
> + This node describes the action to be taken when a thermal zone
> + crosses one of the temperature thresholds described in the trips
> + node. The action takes the form of a mapping relation between a
> + trip and the target cooling device state.
> +
> + patternProperties:
> + "^map[0-9][-a-zA-Z0-9]*$":
> + type: object
> +
> + properties:
> + trip:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description:
> + A phandle of a trip point node within this thermal zone.
> +
> + cooling-device:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + description:
> + A list of cooling device phandles along with the minimum
> + and maximum cooling state specifiers for each cooling
> + device. Using the THERMAL_NO_LIMIT (-1UL) constant in the
> + cooling-device phandle limit specifier lets the framework
> + use the minimum and maximum cooling state for that cooling
> + device automatically.
> +
> + contribution:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + minimum: 0
> + maximum: 100
> + description:
> + The contribution of the cooling devices at the trip
> + temperature, both referenced in this map, to this thermal
> + zone as a percentage.
> +
> + required:
> + - trip
> + - cooling-device
> +

'additionalProperties' for map* nodes?

> + additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/thermal/thermal.h>
> +
> + // Example 1: SDM845 TSENS
> + soc: soc@0 {
> + #address-cells = <2>;
> + #size-cells = <2>;
> +
> + /* ... */
> +
> + tsens0: thermal-sensor@c263000 {
> + compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
> + reg = <0 0x0c263000 0 0x1ff>, /* TM */
> + <0 0x0c222000 0 0x1ff>; /* SROT */
> + #qcom,sensors = <13>;
> + interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 508 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "uplow", "critical";
> + #thermal-sensor-cells = <1>;
> + };
> +
> + tsens1: thermal-sensor@c265000 {
> + compatible = "qcom,sdm845-tsens", "qcom,tsens-v2";
> + reg = <0 0x0c265000 0 0x1ff>, /* TM */
> + <0 0x0c223000 0 0x1ff>; /* SROT */
> + #qcom,sensors = <8>;
> + interrupts = <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 509 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "uplow", "critical";
> + #thermal-sensor-cells = <1>;
> + };
> + };
> +
> + /* ... */
> +
> + thermal-zones {
> + cpu0-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = <&tsens0 1>;
> +
> + trips {
> + cpu0_alert0: trip-point0 {
> + temperature = <90000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu0_alert1: trip-point1 {
> + temperature = <95000>;
> + hysteresis = <2000>;
> + type = "passive";
> + };
> +
> + cpu0_crit: cpu_crit {
> + temperature = <110000>;
> + hysteresis = <1000>;
> + type = "critical";
> + };
> + };
> +
> + cooling-maps {
> + map0 {
> + trip = <&cpu0_alert0>;
> + cooling-device = <&CPU0 THERMAL_NO_LIMIT
> + THERMAL_NO_LIMIT>,
> + <&CPU1 THERMAL_NO_LIMIT
> + THERMAL_NO_LIMIT>,
> + <&CPU2 THERMAL_NO_LIMIT
> + THERMAL_NO_LIMIT>,
> + <&CPU3 THERMAL_NO_LIMIT
> + THERMAL_NO_LIMIT>;
> + };
> +
> + map1 {
> + trip = <&cpu0_alert1>;
> + cooling-device = <&CPU0 THERMAL_NO_LIMIT
> + THERMAL_NO_LIMIT>,
> + <&CPU1 THERMAL_NO_LIMIT
> + THERMAL_NO_LIMIT>,
> + <&CPU2 THERMAL_NO_LIMIT
> + THERMAL_NO_LIMIT>,
> + <&CPU3 THERMAL_NO_LIMIT
> + THERMAL_NO_LIMIT>;
> + };
> + };
> + };
> +
> + /* ... */
> +
> + cluster0-thermal {
> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = <&tsens0 5>;
> +
> + trips {
> + cluster0_alert0: trip-point0 {
> + temperature = <90000>;
> + hysteresis = <2000>;
> + type = "hot";
> + };
> + cluster0_crit: cluster0_crit {
> + temperature = <110000>;
> + hysteresis = <2000>;
> + type = "critical";
> + };
> + };
> + };
> +
> + /* ... */
> +
> + gpu-thermal-top {

This one is not going to match (which should cause an error).

> + polling-delay-passive = <250>;
> + polling-delay = <1000>;
> +
> + thermal-sensors = <&tsens0 11>;
> +
> + trips {
> + gpu1_alert0: trip-point0 {
> + temperature = <90000>;
> + hysteresis = <2000>;
> + type = "hot";
> + };
> + };
> + };
> + };
> +...
> --
> 2.20.1
>