Re: [PATCH V6 1/9] PM / OPP: Introduce "power-domain-opp" property

[Kevin Hilman <khilman@baylibre.com>]

Rob Herring <robh@kernel.org> writes:

> On Wed, Apr 26, 2017 at 04:27:05PM +0530, Viresh Kumar wrote:
>> Power-domains need to express their active states in DT and the devices
>> within the power-domain need to express their dependency on those active
>> states. The power-domains can use the OPP tables without any
>> modifications to the bindings.
>> 
>> Add a new property "power-domain-opp", which will contain phandle to the
>> OPP node of the parent power domain. This is required for devices which
>> have dependency on the configured active state of the power domain for
>> their working.
>> 
>> For some platforms the actual frequency and voltages of the power
>> domains are managed by the firmware and are so hidden from the high
>> level operating system. The "opp-hz" property is relaxed a bit to
>> contain indexes instead of actual frequency values to support such
>> platforms.
>> 
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>  Documentation/devicetree/bindings/opp/opp.txt | 74 ++++++++++++++++++++++++++-
>>  1 file changed, 73 insertions(+), 1 deletion(-)
>> 
>> diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
>> index 63725498bd20..6e30cae2a936 100644
>> --- a/Documentation/devicetree/bindings/opp/opp.txt
>> +++ b/Documentation/devicetree/bindings/opp/opp.txt
>> @@ -77,7 +77,10 @@ This defines voltage-current-frequency combinations along with other related
>>  properties.
>>  
>>  Required properties:
>> -- opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer.
>> +- opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. In some
>> +  cases the exact frequency in Hz may be hidden from the OS by the firmware and
>> +  this field may contain values that represent the frequency in a firmware
>> +  dependent way, for example an index of an array in the firmware.
>
> Not really sure OPP binding makes sense here.

I think OPP makes perfect sense here, because microcontroller firmware
is managaging OPPs in hardware.  We just may not know the exact voltage
and/or frequency (and the firmware/hardware may even be doing AVS for
micro-adjustments.)

> What about all the other properties. We expose voltage, but not freq?

I had the same question.  Seems the same comment about an abstract
"index" is needed for voltage also.

>>  
>>  Optional properties:
>>  - opp-microvolt: voltage in micro Volts.
>> @@ -154,6 +157,13 @@ properties.
>>  
>>  - status: Marks the node enabled/disabled.
>>  
>> +- power-domain-opp: Phandle to the OPP node of the parent power-domain. The
>> +  parent power-domain should be configured to the OPP whose node is pointed by
>> +  the phandle, in order to configure the device for the OPP node that contains
>> +  this property. The order in which the device and power domain should be
>> +  configured is implementation defined. The OPP table of a device can set this
>> +  property only if the device node contains "power-domains" property.
>> +

I do understand the need to map a device OPP to a parent power-domain
OPP, but I really don't like another phandle.

First, just because a device OPP changes does not mean that a
power-domain OPP has to change.  What really needs to be specified is a
minimum requirement, not an exact OPP.  IOW, if a device changes OPP,
the power-domain OPP has to be *at least* an OPP that can guarantee that
level of performance, but could also be a more performant OPP, right?

Also, the parent power-domain driver will have a list of all its
devices, and be able to get OPPs from those devices.

IMO, we should do the first (few) implementations of this feature from
the power-domain driver itself, and not try to figure out how to define
this for everyone in DT until we have a better handle on it (pun
intended) ;)

> I don't even know what to say on this. The continual evolution of 
> OPP bindings continues. This seems like further abuse of DT 
> power-domains (being a region in a chip that can be powergated) with 
> Linux PM domains.

Agreed.

Kevin