Date:   Wed, 24 Nov 2021 20:17:22 +0100
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
Cc:     robh@...nel.org, arnd@...aro.org, heiko@...ech.de,
        rjw@...ysocki.net, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        lukasz.luba@....com, Arnd Bergmann <arnd@...db.de>,
        Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH 1/5] dt-bindings: Powerzone new bindings

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

Powered by blists - more mailing lists