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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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.

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