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

From: Daniel Lezcano
Date: Wed Nov 24 2021 - 11:26:42 EST



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