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

[Ulf Hansson]

On Wed, 1 Dec 2021 at 10:23, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> On Fri, 26 Nov 2021 at 19:15, 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>
> > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > Signed-off-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> > ---
> >    V1: Initial post
> >    V2:
> >      - Added pattern properties and stick to powerzone-*
> >      - Added required property compatible and powerzone-cells
> >      - Added additionnal property
> >      - Added compatible
> >      - Renamed to 'powerzones'
> >      - Added missing powerzone-cells to the topmost node
> >      - Fixed errors reported by 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > ---
> >  .../devicetree/bindings/power/powerzones.yaml | 109 ++++++++++++++++++
> >  1 file changed, 109 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..6e63bbc750c6
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/power/powerzones.yaml
> > @@ -0,0 +1,109 @@
> > +# 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
> > +
> > +  compatible:
> > +    const: powerzones
>
> This looks odd. Why do we need const compatible string? Shouldn't this
> be allowed to be an SoC-powerzone specific compatible?

Alright, after our recent discussions offlist, I believe the
compatible property should be entirely removed.

>
> > +
> > +patternProperties:
> > +  "^(powerzone)([@-].*)?$":
> > +    type: object
> > +    description:
> > +      A node representing a powerzone acting as an aggregator for all
> > +      its children powerzones.
> > +
> > +    properties:
> > +      "#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.
> > +
> > +      powerzones:
> > +        description:
> > +          A phandle to a parent powerzone. If no powerzone attribute is
> > +          set, the described powerzone is the topmost in the hierarchy.
> > +
> > +    required:
> > +      - "#powerzone-cells"
> > +
> > +required:
> > +  - compatible

This should be removed too, of course.

> > +
> > +additionalProperties: true

This should be set to "false", I think. There is no need for any
additional properties besides those that are being part of the binding
above.

> > +
> > +examples:
> > +  - |
> > +    powerzones {
> > +
> > +      compatible = "powerzones";
> > +
> > +      #powerzone-cells = <0>;

This toplevel "powerzones" node, should neither contain a compatible
nor a #powerzone-cells. Please drop this.

Instead we only need to describe the topology by using child nodes, as
in the example below.

> > +
> > +      SOC_PZ: powerzone-soc {
> > +        #powerzone-cells = <0>;
> > +      };
> > +
> > +      PKG_PZ: powerzone-pkg {
> > +        #powerzone-cells = <0>;
> > +        powerzones = <&SOC_PZ>;
> > +      };
> > +
> > +      GPU_PZ: powerzone-gpu {
> > +        #powerzone-cells = <0>;
> > +        powerzones = <&PKG_PZ>;
> > +      };
> > +    };
> > +
> > +  - |
> > +    A57_0: big@0 {
> > +      compatible = "arm,cortex-a57";
> > +      reg = <0x0 0x0>;
> > +      device_type = "cpu";
> > +      #powerzone-cells = <0>;
> > +      powerzones = <&PKG_PZ>;
> > +    };
>
> I think we discussed this in the earlier version too...
>
> The above example describes a powerzone provider, but it doesn't
> really conform to the binding. That's because the binding states that
> powerzone providers should be inside a top-level "powerzone {" node.

>From our offlist discussion, it seems like the cpu nodes should not
have a #powerzone-cells. Instead, the powerzones property should be
sufficient, as it allows you to describe what powerzone(s) the cpu
belongs to, which is exactly what you need.

This also means that we need to extend the DT bindings for CPUs
(Documentation/devicetree/bindings/arm/cpus.yaml), to allow cpu nodes
to have a "powerzones" property. I believe we can do that separately,
on top of $subject patch, as cpus.yaml has "additionalProperties:
true".

>
> I am wondering if we really need the toplevel "powerzone" node.

Please ignore this comment. It has become clear to me that the
toplevel node serves a purpose.

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

Ditto.

> > +      powerzones = <&PKG_PZ>;
> > +    };
> > +...
> > --
> > 2.25.1
> >
>

Kind regards
Uffe