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
