Re: [PATCH V3 2/7] PM / OPP: Introduce "domain-performance-state" binding to OPP nodes

[Rob Herring <robh@kernel.org>]

On Tue, Feb 28, 2017 at 12:57 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 27-02-17, 18:39, Rob Herring wrote:
>> On Fri, Feb 24, 2017 at 02:36:34PM +0530, Viresh Kumar wrote:
>> > If the consumers don't need the capability of switching to different
>> > domain performance states at runtime, then they can simply define their
>> > required domain performance state in their nodes directly.
>> >
>> > But if the device needs the capability of switching to different domain
>> > performance states, as they may need to support different clock rates,
>> > then the per OPP node can be used to contain that information.
>> >
>> > This patch introduces the domain-performance-state (already defined by
>> > Power Domain bindings) to the per OPP node.
>> >
>>
>> We already have OPP voltages, why are those not sufficient?
>
> Those are for the regulator that ONLY controls the device, and
> domain-performance-state belongs to the parent domain which controls many
> devices.
>
>> > +Example 7: domain-Performance-state:
>> > +(example: For 1GHz require domain state 1 and for 1.1 & 1.2 GHz require state 2)
>> > +
>> > +/ {
>> > +   cpu0_opp_table: opp_table0 {
>> > +           compatible = "operating-points-v2";
>> > +           opp-shared;
>> > +
>> > +           opp@1000000000 {
>> > +                   opp-hz = /bits/ 64 <1000000000>;
>>
>> Thinking about this some more, there's a problem here that you have no
>> link to foo_domain. I guess that resides in the cpu's node?
>
> Right, the "cpus" node below demonstrates that.
>
>> > +   cpus {
>> > +           #address-cells = <1>;
>> > +           #size-cells = <0>;
>> > +
>> > +           cpu@0 {
>> > +                   compatible = "arm,cortex-a9";
>> > +                   reg = <0>;
>> > +                   clocks = <&clk_controller 0>;
>> > +                   clock-names = "cpu";
>> > +                   operating-points-v2 = <&cpu0_opp_table>;
>> > +                   power-domains = <&foo_domain>;
>> > +           };
>> > +   };
>> > +};
>
>> > +                   domain-performance-state = <1>;
>
>> Perhaps instead of a number, this should be a phandle to pstate@1. Then
>> you just get the parent if you need to know the domain.
>
> That's what I did in V2, but then I turned it down considering the parent/child
> relationships we may have.
>
> There are multiple cases we can have:
>
> A.) DeviceX  --->  Parent-domain-1 (Contains Perfomance states)
>
> B.) DeviceX  --->  Parent-domain-1  ---> Parent domain-2 (Contains Perfomance states)
>
>                                     ---> Parent domain-2 (Contains Perfomance states)
>                                     |
>                                     |
> C.) DeviceX  --->  Parent-domain-1  |
>                                     |
>                                     |
>                                     ---> Parent domain-3 (Contains Perfomance states)

I'm a bit confused. How does a domain have 2 parent domains?

You have the same problem either way. If I have performance state 2
for the device, that corresponds to domain 2 or 3?

> The case A.) represents a simple case where the parent domain of the device
> contains the performance states. The phandle can work pretty well in this case.
> But the other cases B.) and C.) are a bit complicated as the direct parent
> domain doesn't allow changing the performance states, but its parents. And so I
> went ahead with numbers instead of phandles. Yes, we will still be able to get
> to the performance state node with the help of phandles, but will that be the
> right thing to do ?
>
> --
> viresh