Re: [PATCH v3 1/2] PM / devfreq: Generic CPU frequency to device frequency mapping governor

[skannan@codeaurora.org]

On 2018-08-08 01:47, Sudeep Holla wrote:
> On Tue, Aug 07, 2018 at 12:37:07PM -0700, skannan@codeaurora.org wrote:
>> On 2018-08-07 09:41, Rob Herring wrote:
>> >On Wed, Aug 01, 2018 at 05:57:41PM -0700, Saravana Kannan wrote:
>> >>Many CPU architectures have caches that can scale independent of the
>> >>CPUs.
>> >>Frequency scaling of the caches is necessary to make sure the cache is
>> >>not
>> >>a performance bottleneck that leads to poor performance and power. The
>> >>same
>> >>idea applies for RAM/DDR.
>> >>
>> >>To achieve this, this patch adds a generic devfreq governor that takes
>> >>the
>> >>current frequency of each CPU frequency domain and then adjusts the
>> >>frequency of the cache (or any devfreq device) based on the frequency of
>> >>the CPUs. It listens to CPU frequency transition notifiers to keep
>> >>itself
>> >>up to date on the current CPU frequency.
>> >>
>> >>To decide the frequency of the device, the governor does one of the
>> >>following:
>> >>
>> >>* Uses a CPU frequency to device frequency mapping table
>> >>  - Either one mapping table used for all CPU freq policies (typically
>> >>used
>> >>    for system with homogeneous cores/clusters that have the same OPPs).
>> >>  - One mapping table per CPU freq policy (typically used for ASMP
>> >>systems
>> >>    with heterogeneous CPUs with different OPPs)
>> >>
>> >>OR
>> >>
>> >>* Scales the device frequency in proportion to the CPU frequency. So, if
>> >>  the CPUs are running at their max frequency, the device runs at its
>> >>max
>> >>  frequency.  If the CPUs are running at their min frequency, the device
>> >>  runs at its min frequency. And interpolated for frequencies in
>> >>between.
>> >>
>> >>Signed-off-by: Saravana Kannan <skannan@codeaurora.org>
>> >>---
>> >> .../bindings/devfreq/devfreq-cpufreq-map.txt       |  53 ++
>> >
>> >Bindings should be a separate patch.
>> >
>> >> drivers/devfreq/Kconfig                            |   8 +
>> >> drivers/devfreq/Makefile                           |   1 +
>> >> drivers/devfreq/governor_cpufreq_map.c             | 583
>> >>+++++++++++++++++++++
>> >> 4 files changed, 645 insertions(+)
>> >> create mode 100644
>> >>Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
>> >> create mode 100644 drivers/devfreq/governor_cpufreq_map.c
>> >>
>> >>diff --git
>> >>a/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
>> >>b/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
>> >>new file mode 100644
>> >>index 0000000..982a30b
>> >>--- /dev/null
>> >>+++ b/Documentation/devicetree/bindings/devfreq/devfreq-cpufreq-map.txt
>> >>@@ -0,0 +1,53 @@
>> >>+Devfreq CPUfreq governor
>> >>+
>> >>+devfreq-cpufreq-map is a parent device that contains one or more child
>> >>devices.
>> >>+Each child device provides CPU frequency to device frequency mapping
>> >>for a
>> >>+specific device. Examples of devices that could use this are: DDR,
>> >>cache and
>> >>+CCI.
>> >>+
>> >>+Parent device name shall be "devfreq-cpufreq-map".
>> >>+
>> >>+Required child device properties:
>> >>+- cpu-to-dev-map, or cpu-to-dev-map-<X>:
>> >>+			A list of tuples where each tuple consists of a
>> >>+			CPU frequency (KHz) and the corresponding device
>> >>+			frequency. CPU frequencies not listed in the table
>> >>+			will use the device frequency that corresponds to the
>> >>+			next rounded up CPU frequency.
>> >>+			Use "cpu-to-dev-map" if all CPUs in the system should
>> >>+			share same mapping.
>> >>+			Use cpu-to-dev-map-<cpuid> to describe different
>> >>+			mappings for different CPUs. The property should be
>> >>+			listed only for the first CPU if multiple CPUs are
>> >>+			synchronous.
>> >>+- target-dev:		Phandle to device that this mapping applies to.
>> >>+
>> >>+Example:
>> >>+	devfreq-cpufreq-map {
>> >>+		cpubw-cpufreq {
>> >>+			target-dev = <&cpubw>;
>> >>+			cpu-to-dev-map =
>> >>+				<  300000  1144000 >,
>> >>+				<  422400  2288000 >,
>> >>+				<  652800  3051000 >,
>> >>+				<  883200  5996000 >,
>> >>+				< 1190400  8056000 >,
>> >>+				< 1497600 10101000 >,
>> >>+				< 1728000 12145000 >,
>> >>+				< 2649600 16250000 >;
>> >
>> >Now we have frequencies listed in multiple places, the OPP tables and
>> >here? Perhaps it is grouping OPPs that should be done.
>> 
>> This doesn't list all OPPs (it can if necessary). This is listing the
>> minimum frequency needed to give good performance/power for a
>> device/product.
>> 
> 
> Shouldn't the "status" property be used to disable OPPs you don't need
> on a particular platform ?

But that's not the point here? We aren't trying to disable any OPPs 
here? Not sure what you mean.

> Duplicating values is highly prone to errors and should be avoided.
> 
>> AFAIK, OPP grouping isn't something that's supported in OPP framework 
>> or in
>> DT. Is there something specific you had in mind? Also, I'd like for 
>> this to
>> work even with devices that don't have OPPs listed in DT.
>> 
> Also what's the solution you have for platforms with new *QCom FW 
> Cpufreq* ?
> IIUC the frequency is obtained from the firmware. TBH this should 
> ideally
> be handled in firmware if cpufreq is also handled by the firmware. I 
> guess
> this platform doesn't have that ?

All QC platforms would use this.

As a personal (non-Qcom) opinion, I'd rather the kernel control this 
than have some black magic FW manage this. I've a really bitter taste in 
my mouth for FW hiding this because of a broken ACPI implementation in 
one of my x86 motherboards prevented CPUfreq from working (this was well 
before I worked on CPUfreq). Pushing stuff to FW seems to beat the ideal 
behind an opensource OS. In a few cases it's elegant or more robust, so 
maybe in those cases its okay to use a FW. But I'd rather not for 
simpler stuff like this.

-Saravana