Re: [RFD] DTPM hierarchy description via DT

[Daniel Lezcano]

Hi Rob,

just a heads up about the discussion.

Do you have a suggestion about the bindings as proposed below?

Is it valid to have a phandle to point to an empty node ?




On 04/07/2021 11:58, Daniel Lezcano wrote:
> 
> Hi Rob,
> 
> thank you for your answer.
> 
> On 02/07/2021 18:51, Rob Herring wrote:
>> On Fri, Jul 2, 2021 at 7:51 AM Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> wrote:
>>>
>>>
>>> Hi,
>>>
>>> recently a new framework based on top of the powercap framework was
>>> introduced to limit the power of some devices when they are capable of
>>> that. Based on the approximate power numbers from the energy model, it
>>> allows to have a rough estimation of the power consumption and set the
>>> power limit [1].
>>>
>>> This framework describes via a hierarchy the constraints relationship
>>> between all those devices and it is SoC specific.
>>>
>>> The problem is how to describe this hierarchy.
>>>
>>> The hierarchy could be like:
>>>
>>> soc
>>>    |
>>>    |-- package
>>>    |      |
>>>    |      |-- cluster0
>>>    |      |      |
>>>    |      |      |-- cpu0
>>>    |      |      |
>>>    |      |      |-- cpu1
>>>    |      |      |
>>>    |      |      |-- cpu2
>>>    |      |      |
>>>    |      |      `-- cpu3
>>>    |      |
>>>    |      |-- cluster1
>>>    |      |      |
>>>    |      |      |-- cpu4
>>>    |      |      |
>>>    |      |      `-- cpu5
>>
>> We already have all this with cpu topology binding which feeds cpu
>> topology functionality in the kernel. Is there a case for the
>> powerzone hierarchy to be different? 
> 
> Yes, the performance domain and the topology may differ. For instance
> the hi6220 has two clusters but one performance domain, so the powerzone
> is all the cpus under the same group.
> 
> I realize I should have not put the clusterx name but 'big' and
> 'little'. Also we may want to add only one CPU in the powerzone for
> example, or make a device belonging to multiple powerzones.
> 
> 
>> For CPUs, I don't see why we'd
>> need anything else or perhaps just a new property in cpu nodes for
>> something?
> 
> Yes, a property should be fine.
> 
>> For other devices, is there a need for a hierarchy or just grouping?
> 
> [ ... ]
> 
> We need to create a parent powerzone which is not a device but a node to
> group different devices and/or another powerzone. So I guess the answer
> is both grouping the devices but also a hierarchy.
> 
> 
>>> We are far from this description yet but it is for illustration purpose.
>>>
>>> All the nodes of the tree do not necessarily reflect real devices, for
>>> example, 'package' or 'multimedia' are not described in the DT.
>>>
>>> What we want is to build this hierarchy which is SoC dependent.
>>>
>>> A first proposal was made to create those nodes in configfs but because
>>> it results in creating a node in sysfs also the approach is not valid [2]
>>>
>>> It was suggested to use the devicetree to describe such hierarchy. There
>>> are several possibilities but the nodes not describing real hardware
>>> above is difficult to describe. Also, the hierarchy should not be over
>>> complexified.
>>
>> I'm very leary of yet another CPU PM related binding. We already have
>> topology, idle states, OPP, ...
>>
>> There's less on the device side, but it's more fragmented with each
>> vendor doing their own thing.
> 
> Ok fair enough, I guess that implies the solution 1 below is not adequate.
> 
> The main difference with the 2 other possibilities below is one will
> always need SoC specific code to create the hierarchy while the other
> can work with generic code.
> 
> However, I'm uncomfortable with the idea of "powerzone-parent' property
> with a phandle pointing to an empty node. Is is valid ?
> 




>>> On the other hand most of the devices are already described, so it is a
>>> question of tightening them together.
>>>
>>> There are different possibilities to describe this hierarchy:
>>>
>>> 1. Create the hierarchy in the DT:
>>>
>>>         power-zones {
>>>
>>>                 package {
>>>
>>>                         big {
>>>                                 powerzone = <&cpu_b0 POWERZONE_DVFS>,
>>>                                         <&cpu_b1 POWERZONE_DVFS>;
>>>                         };
>>>
>>>                         little {
>>>                                 powerzone = <&cpu_l0 POWERZONE_DVFS>,
>>>                                         <&cpu_l1 POWERZONE_DVFS>,
>>>                                         <&cpu_l2 POWERZONE_DVFS>,
>>>                                         <&cpu_l3 POWERZONE_DVFS>;
>>>                         };
>>>
>>>                         gpu {
>>>                                 powerzone = <&gpu POWERZONE_DVFS>;
>>>                         };
>>>                 };
>>>         };
>>>
>>> 2. Let the kernel build the hierarchy but add a property in the
>>> different nodes:
>>>
>>> https://git.linaro.org/people/daniel.lezcano/linux.git/commit/?h=powercap/dtpm-dts-1.0&id=05943f5a1cf33df36dbe423fd4b549a9aa244da1
>>>
>>> And from there the kernel does for_each_node_with_property(). The kernel
>>> has to know "cpu-pd0" and "cpu-pd1". That implies a per soc
>>> initialization code:
>>>
>>> https://git.linaro.org/people/daniel.lezcano/linux.git/commit/?h=powercap/dtpm-dts-1.0&id=7525abb234695d07a0094b2f511d5fe8bea0a979
>>>
>>> https://git.linaro.org/people/daniel.lezcano/linux.git/commit/?h=powercap/dtpm-dts-1.0&id=70e1deb642a939d14dd9b0391d8219cf21a03253
>>>
>>>
>>> 3. An intermediate description between 1. and 2.
>>>
>>> The nodes have a property which is a phandle to the parent node. But if
>>> the parent node does not exists, create an empty and point to it.
>>>
>>> package {
>>>         powerzone;
>>>         powerzone-parent = <&soc>;
>>> };
>>>
>>> cluster0 {
>>>         powerzone-parent = <&package>;
>>> };
>>>
>>>         ...
>>>
>>>                 cpu_l0: cpu@0 {
>>>                         device_type = "cpu";
>>>                         compatible = "arm,cortex-a53";
>>>                         reg = <0x0 0x0>;
>>>                         enable-method = "psci";
>>>                         capacity-dmips-mhz = <485>;
>>>                         clocks = <&cru ARMCLKL>;
>>>                         #cooling-cells = <2>; /* min followed by max */
>>>                         dynamic-power-coefficient = <100>;
>>>                         cpu-idle-states = <&CPU_SLEEP &CLUSTER_SLEEP>;
>>>                         powerzone-parent = <&cluster0>;
>>>                 };
>>>
>>>         ...
>>>
>>> I implemented 1. and 2. but before going forward and writing the yaml
>>> bindings which are time consuming, it would be preferable we agree on
>>> something instead of having to drop the code again and again.
>>>
>>> Thanks for your comments
>>>
>>>
>>> [1] https://lwn.net/Articles/839318/
>>> [2] https://www.spinics.net/lists/kernel/msg3891770.html
>>>
>>> --
>>> <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
> 
> 


-- 
<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