On Wed, 24 Nov 2021 at 17:26, Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> 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
