diff mbox series

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

Message ID 20211124125506.2971069-1-daniel.lezcano@linaro.org (mailing list archive)
State Superseded, archived
Headers show
Series [1/5] dt-bindings: Powerzone new bindings | expand

Commit Message

Daniel Lezcano Nov. 24, 2021, 12:55 p.m. UTC 
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@mail.gmail.com/

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@arndb.de>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 .../devicetree/bindings/power/powerzones.yaml | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/powerzones.yaml

Comments

Ulf Hansson Nov. 24, 2021, 2:54 p.m. UTC | #1 
On Wed, 24 Nov 2021 at 13:55, Daniel Lezcano <daniel.lezcano@linaro.org> 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@mail.gmail.com/
>
> 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@arndb.de>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  .../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@linaro.org>
> +
> +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
Daniel Lezcano Nov. 24, 2021, 4:26 p.m. UTC | #2 
Hi Ulf,

thanks for the review

On 24/11/2021 15:54, Ulf Hansson wrote:

[ ... ]

>> +  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.

Do you mean a compatible string?

> 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.

Sure

>> +  "#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.

May be that could be needed in the future. No objection to rename it to
'powerzones'.

>> +    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"

Ok

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

I was unsure about adding it. With the actual description what would be
the benefit ?

>> +examples:
>> +  - |
>> +    powerzones {
>> +
>> +      SOC_PZ: soc {
>> +      };
> 
> This looks odd to me.
> 
> Why do we need an empty node? If this is the topmost power-zone, 

Yes it is

> it
> should still have the #powerzone-cells specifier, I think.

Ok, makes sense

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

Ok, may be 'powerzone-pkg' to be consistent with the power-domains pattern?

>> +        #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?

I'm not sure 'provider' or 'consumer' make sense in this context.

big@0 is a powerzone we can act on and its parent is the BIG_PZ powerzone.

However this description is correct but confusing.

Given big@0 and big@1 belong to the big 'cluster' and when we act on the
performance state of big@0, big@1 is also changed, the correct
description would be:

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

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

If in the future, there will be a performance domain per core, then the
former description above would make sense.
Ulf Hansson Nov. 24, 2021, 7:17 p.m. UTC | #3 
On Wed, 24 Nov 2021 at 17:26, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
>
> Hi Ulf,
>
> thanks for the review
>
> On 24/11/2021 15:54, Ulf Hansson wrote:
>
> [ ... ]
>
> >> +  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.
>
> Do you mean a compatible string?

Yes, but there is no need to specify that part of the powerzone
bindings, I think.

Although, let's see what Rob thinks here.

>
> > 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.
>
> Sure
>
> >> +  "#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.
>
> May be that could be needed in the future. No objection to rename it to
> 'powerzones'.
>
> >> +    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"
>
> Ok
>
> > Moreover, we probably need to allow additional properties? At least it
> > looks so from the last example below. Then:
> >
> > additionalProperties: true
>
> I was unsure about adding it. With the actual description what would be
> the benefit ?

A powerzone provider node is then allowed to have other properties
too. Like a compatible string, for example.

Assuming I also have understood the additionalProperties thingy correctly. Rob?

>
> >> +examples:
> >> +  - |
> >> +    powerzones {
> >> +
> >> +      SOC_PZ: soc {
> >> +      };
> >
> > This looks odd to me.
> >
> > Why do we need an empty node? If this is the topmost power-zone,
>
> Yes it is
>
> > it
> > should still have the #powerzone-cells specifier, I think.
>
> Ok, makes sense
>
> >> +
> >> +      PKG_PZ: pkg {
> >
> > As I stated above, I would prefer some kind of common pattern of the
> > subnode names. Maybe "pkg-powerzone"?
>
> Ok, may be 'powerzone-pkg' to be consistent with the power-domains pattern?

Sure, that seems reasonable.

>
> >> +        #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?
>
> I'm not sure 'provider' or 'consumer' make sense in this context.
>
> big@0 is a powerzone we can act on and its parent is the BIG_PZ powerzone.

I see.

Then it seems like we shouldn't have the toplevel "powerzones" node,
as it looks like a powerzone provider may very well be part of an
existing node.

>
> However this description is correct but confusing.
>
> Given big@0 and big@1 belong to the big 'cluster' and when we act on the
> performance state of big@0, big@1 is also changed, the correct
> description would be:
>
>     A57_0: big@0 {
>       compatible = "arm,cortex-a57";
>       reg = <0x0 0x0>;
>       device_type = "cpu";
>       #powerzone-cells = <0>;
>       powerzone = <&PKG_PZ>;
>     };
>
>     A57_1: big@1 {
>       compatible = "arm,cortex-a57";
>       reg = <0x0 0x0>;
>       device_type = "cpu";
>       #powerzone-cells = <0>;
>       powerzone = <&PKG_PZ>;
>     };
>
> If in the future, there will be a performance domain per core, then the
> former description above would make sense.

Okay, I see. Thanks for clarifying!

Kind regards
Uffe
Rob Herring (Arm) Nov. 25, 2021, 9:26 p.m. UTC | #4 
On Wed, 24 Nov 2021 13:55:00 +0100, Daniel Lezcano 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@mail.gmail.com/
> 
> 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@arndb.de>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  .../devicetree/bindings/power/powerzones.yaml | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/powerzones.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/powerzones.yaml: 'additionalProperties' is a required property
	hint: A schema without a "$ref" to another schema must define all properties and use "additionalProperties"
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/power/powerzones.yaml: ignoring, error in schema: 
warning: no schema found in file: ./Documentation/devicetree/bindings/power/powerzones.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1559118

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.
diff mbox series

Patch

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@linaro.org>
+
+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
+      
+  "#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:
+    description:
+      A phandle to a parent powerzone. If no powerzone attribute is set, the
+      described powerzone is the topmost in the hierarchy.
+
+examples:
+  - |
+    powerzones {
+
+      SOC_PZ: soc {
+      };
+
+      PKG_PZ: pkg {
+        #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>;
+    };
+
+    A57_1: big@1 {
+      compatible = "arm,cortex-a57";
+      reg = <0x0 0x0>;
+      device_type = "cpu";
+      #powerzone-cells = <0>;
+      powerzone = <&BIG_PZ>;
+    };
+...