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

Hi Ulf,

thanks for the review

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

[ ... ]

quoted

+  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

quoted

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

quoted

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

quoted

+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

quoted

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

quoted

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

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog