Re: [PATCH 1/5] dt-bindings: Powerzone new bindings

[Ulf Hansson]

On Wed, 24 Nov 2021 at 13:55, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>
> The proposed bindings are describing a set of powerzones.
>
> A power zone is the logical name for a component which is capable of
> power capping and where we can measure the power consumption.
>
> A power zone can aggregate several power zones in terms of power
> measurement and power limitations. That allows to apply power
> constraint to a group of components and let the system balance the
> allocated power in order to comply with the constraint.
>
> The ARM System Control and Management Interface (SCMI) can provide a
> power zone description.
>
> The powerzone semantic is also found on the Intel platform with the
> RAPL register.
>
> The Linux kernel powercap framework deals with the powerzones:
>
> https://www.kernel.org/doc/html/latest/power/powercap/powercap.html
>
> The powerzone can also represent a group of children powerzones, hence
> the description can result on a hierarchy. Such hierarchy already
> exists with the hardware or can be represented an computed from the
> kernel.
>
> The hierarchical description was initially proposed but not desired
> given there are other descriptions like the power domain proposing
> almost the same description.
>
> https://lore.kernel.org/all/CAL_JsqLuLcHj7525tTUmh7pLqe7T2j6UcznyhV7joS8ipyb_VQ@xxxxxxxxxxxxxx/
>
> The description gives the power constraint dependencies to apply on a
> specific group of logically or physically aggregated devices. They do
> not represent the physical location or the power domains of the SoC
> even if the description could be similar.
>
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> ---
>  .../devicetree/bindings/power/powerzones.yaml | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/powerzones.yaml
>
> diff --git a/Documentation/devicetree/bindings/power/powerzones.yaml b/Documentation/devicetree/bindings/power/powerzones.yaml
> new file mode 100644
> index 000000000000..1ae3f82ae29c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/powerzones.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: GPL-2.0
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/power/powerzones.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Power zones description
> +
> +maintainers:
> +  - Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> +
> +description: |+
> +
> +  A System on Chip contains a multitude of active components and each
> +  of them is a source of heat. Even if a temperature sensor is not
> +  present, a source of heat can be controlled by acting on the
> +  consumed power via different techniques.
> +
> +  A powerzone describes a component or a group of components where we
> +  can control the maximum power consumption. For instance, a group of
> +  CPUs via the performance domain, a LCD screen via the brightness,
> +  etc ...
> +
> +  Different components when they are used together can significantly
> +  increase the overall temperature, so the description needs to
> +  reflect this dependency in order to assign a power budget for a
> +  group of powerzones.
> +
> +  This description is done via a hierarchy and the DT reflects it. It
> +  does not represent the physical location or a topology, eg. on a
> +  big.Little system, the little CPUs may not be represented as they do
> +  not contribute significantly to the heat, however the GPU can be
> +  tied with the big CPUs as they usually have a connection for
> +  multimedia or game workloads.
> +
> +properties:
> +  $nodename:
> +    const: powerzones
> +

Do we really need a top-node like this? Can't that be left as a
platform/soc specific thing instead? Along the lines of how the last
example below looks like? Maybe we can have both options? I guess Rob
will tell us.

Moreover, maybe we should put some constraints on the names of
subnodes (provider nodes) with a "patternProperties". Something along
the lines of below.

patternProperties:
  "^(powerzone)([@-].*)?$":
    type: object
    description:
      Each node represents a powerzone.

> +  "#powerzone-cells":
> +    description:
> +      Number of cells in powerzone specifier. Typically 0 for nodes
> +      representing but it can be any number in the future to describe
> +      parameters of the powerzone.
> +
> +  powerzone:

Maybe "powerzones" instead of "powerzone". Unless we believe that we
never need to allow multiple parent-zones for a child-zone.

> +    description:
> +      A phandle to a parent powerzone. If no powerzone attribute is set, the
> +      described powerzone is the topmost in the hierarchy.
> +

We should probably state that the "#powerzone-cells"  are required. Like below:

required:
  - "#powerzone-cells"

Moreover, we probably need to allow additional properties? At least it
looks so from the last example below. Then:

additionalProperties: true

> +examples:
> +  - |
> +    powerzones {
> +
> +      SOC_PZ: soc {
> +      };

This looks odd to me.

Why do we need an empty node? If this is the topmost power-zone, it
should still have the #powerzone-cells specifier, I think.

> +
> +      PKG_PZ: pkg {

As I stated above, I would prefer some kind of common pattern of the
subnode names. Maybe "pkg-powerzone"?

> +        #powerzone-cells = <0>;
> +        powerzone = <&SOC_PZ>;
> +      };
> +
> +      BIG_PZ: big {
> +        #powerzone-cells = <0>;
> +        powerzone = <&PKG_PZ>;
> +      };
> +
> +      GPU_PZ: gpu {
> +        #powerzone-cells = <0>;
> +        powerzone = <&PKG_PZ>;
> +      };
> +
> +      MULTIMEDIA_PZ: multimedia {
> +        #powerzone-cells = <0>;
> +        powerzone = <&SOC_PZ>;
> +      };
> +    };
> +
> +  - |
> +    A57_0: big@0 {
> +      compatible = "arm,cortex-a57";
> +      reg = <0x0 0x0>;
> +      device_type = "cpu";
> +      #powerzone-cells = <0>;
> +      powerzone = <&BIG_PZ>;

Just to make sure I understand correctly. The big@0 node is a
powerzone provider too? Or did you mean to specify it as a consumer?

> +    };
> +
> +    A57_1: big@1 {
> +      compatible = "arm,cortex-a57";
> +      reg = <0x0 0x0>;
> +      device_type = "cpu";
> +      #powerzone-cells = <0>;
> +      powerzone = <&BIG_PZ>;
> +    };

Kind regards
Uffe