* [PATCH] thermal: of-thermal: avoid NULL ops dereference when writing trip temperature
From: Akinobu Mita @ 2019-07-03 14:24 UTC (permalink / raw)
To: linux-pm; +Cc: Akinobu Mita, Zhang Rui, Eduardo Valentin, Daniel Lezcano
While no sensor is registered to a DT thermal zone, __thermal_zone->ops
(struct thermal_zone_of_device_ops) is set to NULL.
This causes a NULL pointer dereference when writing trip point temperature
(i.e. trip_point_[0-*]_temp).
Fix it by checking if ops is NULL under thermal_zone_device's lock that
protects against concurrent access by thermal_zone_of_sensor_register() and
thermal_zone_of_sensor_unregister().
Cc: Zhang Rui <rui.zhang@intel.com>
Cc: Eduardo Valentin <edubezval@gmail.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
drivers/thermal/of-thermal.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/thermal/of-thermal.c b/drivers/thermal/of-thermal.c
index dc5093b..c207075 100644
--- a/drivers/thermal/of-thermal.c
+++ b/drivers/thermal/of-thermal.c
@@ -332,17 +332,20 @@ static int of_thermal_set_trip_temp(struct thermal_zone_device *tz, int trip,
int temp)
{
struct __thermal_zone *data = tz->devdata;
+ int ret = 0;
if (trip >= data->ntrips || trip < 0)
return -EDOM;
- if (data->ops->set_trip_temp) {
- int ret;
+ mutex_lock(&tz->lock);
+ if (data->ops && data->ops->set_trip_temp)
ret = data->ops->set_trip_temp(data->sensor_data, trip, temp);
- if (ret)
- return ret;
- }
+
+ mutex_unlock(&tz->lock);
+
+ if (ret)
+ return ret;
/* thermal framework should take care of data->mask & (1 << trip) */
data->trips[trip].temperature = temp;
--
2.7.4
^ permalink raw reply related
* RE: [PATCH] cpuidle/drivers/mobile: Add new governor for mobile/embedded systems
From: Doug Smythies @ 2019-07-03 14:23 UTC (permalink / raw)
To: 'Daniel Lezcano'
Cc: linux-kernel, 'Rafael J. Wysocki',
'Thomas Gleixner', 'Greg Kroah-Hartman',
'open list:CPU IDLE TIME MANAGEMENT FRAMEWORK', rafael
In-Reply-To: <20190620115826.4897-1-daniel.lezcano@linaro.org>
Hi Daniel,
I tried your "mobile" governor, albeit not on a mobile device.
On 2019.06.20 04:58 Daniel Lezcano wrote:
...
> The mobile governor is a new governor targeting embedded systems
> running on battery where the energy saving has a higher priority than
> servers or desktops. This governor aims to save energy as much as
> possible but with a performance degradation tolerance.
>
> In this way, we can optimize the governor for specific mobile workload
> and more generally embedded systems without impacting other platforms.
I just wanted to observe the lower energy, accepting performance
degradation. My workloads may have been inappropriate.
...
> +
> +#define EMA_ALPHA_VAL 64
> +#define EMA_ALPHA_SHIFT 7
> +#define MAX_RESCHED_INTERVAL_MS 100
> +
> +static DEFINE_PER_CPU(struct mobile_device, mobile_devices);
> +
> +static int mobile_ema_new(s64 value, s64 ema_old)
> +{
> + if (likely(ema_old))
> + return ema_old + (((value - ema_old) * EMA_ALPHA_VAL) >>
> + EMA_ALPHA_SHIFT);
> + return value;
> +}
Do you have any information as to why these numbers?
Without any background, the filter seems overly new value weighted to me.
It is an infinite impulse response type filter, currently at:
output = 0.5 * old + 0.5 * new.
I tried, but didn't get anything conclusive:
output = 0.875 * old + 0.125 * new.
I did it this way:
#define EMA_ALPHA_VAL 7
#define EMA_ALPHA_SHIFT 3
#define MAX_RESCHED_INTERVAL_MS 100
static DEFINE_PER_CPU(struct mobile_device, mobile_devices);
static int mobile_ema_new(s64 value, s64 ema_old)
{
if (likely(ema_old))
return ((ema_old * EMA_ALPHA_VAL) + value) >>
EMA_ALPHA_SHIFT;
return value;
}
...
> + /*
> + * Sum all the residencies in order to compute the total
> + * duration of the idle task.
> + */
> + residency = dev->last_residency - s->exit_latency;
What about when the CPU comes out of the idle state before it
even gets fully into it? Under such conditions it seems to hold
much too hard at idle states that are too deep, to the point
where energy goes up while performance goes down.
Anyway, I did a bunch of tests and such, but have deleted
most from this e-mail, because it's just noise. I'll
include just one set:
For a work load that would normally result in a lot of use
of shallow idle states (single core pipe-test * 2 cores).
I got (all kernel 5.2-rc5 + this patch):
Idle governor, teo; CPU frequency scaling: intel-cpufreq/ondemand;
Processor package power: 40.4 watts; 4.9 uSec/loop
Idle governor, teo; CPU frequency scaling: intel-cpufreq/ondemand;
Processor package power: 34 watts; 5.2 uSec/loop
Idle governor, mobile; CPU frequency scaling: intel-cpufreq/ondemand;
Processor package power: 25.9 watts; 11.1 uSec/loop
Idle governor, menu; CPU frequency scaling: intel-cpufreq/ondemand;
Processor package power: 34.2 watts; 5.23 uSec/loop
Idle governor, teo; CPU frequency scaling: intel-cpufreq/ondemand;
Maximum CPU frequency limited to 73% to match mobile energy.
Processor package power: 25.4 watts; 6.4 uSec/loop
... Doug
^ permalink raw reply
* RE: NO_HZ_IDLE causes consistently low cpu "iowait" time (and higher cpu "idle" time)
From: Doug Smythies @ 2019-07-03 14:06 UTC (permalink / raw)
To: 'Alan Jenkins'; +Cc: linux-pm, linux-kernel
In-Reply-To: <2ff025f1-9a3e-3eae-452b-ef84824009b4@gmail.com>
On 2019.07.01 08:34 Alan Jenkins wrote:
> Hi
Hi,
> I tried running a simple test:
>
> dd if=testfile iflag=direct bs=1M of=/dev/null
>
> With my default settings, `vmstat 10` shows something like 85% idle time
> to 15% iowait time. I have 4 CPUs, so this is much less than one CPU
> worth of iowait time.
>
> If I boot with "nohz=off", I see idle time fall to 75% or below, and
> iowait rise to about 25%, equivalent to one CPU. That is what I had
> originally expected.
>
> (I can also see my expected numbers, if I disable *all* C-states and
> force polling using `pm_qos_resume_latency_us` in sysfs).
>
> The numbers above are from a kernel somewhere around v5.2-rc5. I saw
> the "wrong" results on some previous kernels as well. I just now
> realized the link to NO_HZ_IDLE.[1]
>
> [1]
> https://unix.stackexchange.com/questions/517757/my-basic-assumption-about-system-iowait-does-not-hold/527836#527836
>
> I did not find any information about this high level of inaccuracy. Can
> anyone explain, is this behaviour expected?
I'm not commenting on expected behaviour or not, just that it is
inconsistent.
>
> I found several patches that mentioned "iowait" and NO_HZ_IDLE. But if
> they described this problem, it was not clear to me.
>
> I thought this might also be affecting the "IO pressure" values from the
> new "pressure stall information"... but I am too confused already, so I
> am only asking about iowait at the moment :-).
Using your workload, I confirm inconsistent behaviour for /proc/stat
(which vmstat uses) between kernels 4.15, 4.16, and 4.17:
4.15 does what you expect, no matter idle states enabled or disabled.
4.16 doesn't do what you expect regardless. (although a little erratic.)
>= 4.17 does what you expect with only idle state 0 enabled, and doesn't otherwise.
Actual test data vmstat (/proc/stat) (8 CPUs, 12.5% = 1 CPU)):
Kernel idle/iowait % Idle states >= 1
4.15 88/12 enabled
4.15 88/12 disabled
4.16 99/1 enabled
4.16 99/1 disabled
4.17 98/2 enabled
4.17 88/12 disabled
Note 1: I never booted with "nohz=off" because the tick never turns off for
idle state 0, which is good enough for testing.
Note 2: Myself, I don't use /proc/stat for idle time statistics. I use:
/sys/devices/system/cpu/cpu*/cpuidle/state*/time
And they seem to always be consistent at the higher idle percentage number.
Unless someone has some insight, the next step is kernel bisection,
once for between kernel 4.15 and 4.16, then again between 4.16 and 4.17.
The second bisection might go faster with knowledge gained from the first.
Alan: Can you do kernel bisection? I can only do it starting maybe Friday.
... Doug
^ permalink raw reply
* Re: [RFC PATCH v2 0/5] sched/cpufreq: Make schedutil energy aware
From: Douglas Raillard @ 2019-07-03 13:38 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, mingo, rjw, viresh.kumar, quentin.perret,
patrick.bellasi, dietmar.eggemann
In-Reply-To: <20190702154422.GV3436@hirez.programming.kicks-ass.net>
Hi Peter,
On 7/2/19 4:44 PM, Peter Zijlstra wrote:
> On Thu, Jun 27, 2019 at 06:15:58PM +0100, Douglas RAILLARD wrote:
>> Make schedutil cpufreq governor energy-aware.
>>
>> - patch 1 introduces a function to retrieve a frequency given a base
>> frequency and an energy cost margin.
>> - patch 2 links Energy Model perf_domain to sugov_policy.
>> - patch 3 updates get_next_freq() to make use of the Energy Model.
>
>>
>> 1) Selecting the highest possible frequency for a given cost. Some
>> platforms can have lower frequencies that are less efficient than
>> higher ones, in which case they should be skipped for most purposes.
>> They can still be useful to give more freedom to thermal throttling
>> mechanisms, but not under normal circumstances.
>> note: the EM framework will warn about such OPPs "hertz/watts ratio
>> non-monotonically decreasing"
>
> Humm, for some reason I was thinking we explicitly skipped those OPPs
> and they already weren't used.
>
> This isn't in fact so, and these first few patches make it so?
That's correct, the cost information about each OPP has been introduced recently in mainline
by the energy model series. Without that info, the only way to skip them that comes to my
mind is to set a policy min frequency, since these inefficient OPPs are usually located
at the lower end.
Thanks,
Douglas
^ permalink raw reply
* [PATCH] opp: Don't decrement uninitialized list_kref
From: Viresh Kumar @ 2019-07-03 10:48 UTC (permalink / raw)
To: Viresh Kumar, Nishanth Menon, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Rafael Wysocki,
Rajendra Nayak, linux-kernel
The list_kref was added for static OPPs and to track their users. The
kref is initialized while the static OPPs are added, but removed
unconditionally even if the static OPPs were never added. This causes
refcount mismatch warnings currently.
Fix that by always initializing the kref when the OPP table is first
initialized. The refcount is later incremented only for the second user
onwards.
Fixes: d0e8ae6c26da ("OPP: Create separate kref for static OPPs list")
Reported-and-tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 1 +
drivers/opp/of.c | 21 ++++-----------------
2 files changed, 5 insertions(+), 17 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 89ec6aa220cf..2958cc7bbb58 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -943,6 +943,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
INIT_LIST_HEAD(&opp_table->opp_list);
kref_init(&opp_table->kref);
+ kref_init(&opp_table->list_kref);
/* Secure the device table modification */
list_add(&opp_table->node, &opp_tables);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index a637f30552a3..bf62b357437c 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -665,8 +665,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
return 0;
}
- kref_init(&opp_table->list_kref);
-
/* We have opp-table node now, iterate over it and add OPPs */
for_each_available_child_of_node(opp_table->np, np) {
opp = _opp_add_static_v2(opp_table, dev, np);
@@ -675,17 +673,15 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
ret);
of_node_put(np);
- goto put_list_kref;
+ return ret;
} else if (opp) {
count++;
}
}
/* There should be one of more OPP defined */
- if (WARN_ON(!count)) {
- ret = -ENOENT;
- goto put_list_kref;
- }
+ if (WARN_ON(!count))
+ return -ENOENT;
list_for_each_entry(opp, &opp_table->opp_list, node)
pstate_count += !!opp->pstate;
@@ -694,8 +690,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
if (pstate_count && pstate_count != count) {
dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
count, pstate_count);
- ret = -ENOENT;
- goto put_list_kref;
+ return -ENOENT;
}
if (pstate_count)
@@ -704,11 +699,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
opp_table->parsed_static_opps = true;
return 0;
-
-put_list_kref:
- _put_opp_list_kref(opp_table);
-
- return ret;
}
/* Initializes OPP tables based on old-deprecated bindings */
@@ -734,8 +724,6 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
return -EINVAL;
}
- kref_init(&opp_table->list_kref);
-
val = prop->value;
while (nr) {
unsigned long freq = be32_to_cpup(val++) * 1000;
@@ -745,7 +733,6 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
if (ret) {
dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
__func__, freq, ret);
- _put_opp_list_kref(opp_table);
return ret;
}
nr -= 2;
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* Re: [PATCH 2/2] opp: Manage empty OPP tables with clk handle
From: Rajendra Nayak @ 2019-07-03 10:47 UTC (permalink / raw)
To: Viresh Kumar; +Cc: vireshk, sboyd, linux-pm, linux-kernel, linux-arm-msm
In-Reply-To: <20190703094746.l354nwp2gwuwhiu4@vireshk-i7>
On 7/3/2019 3:17 PM, Viresh Kumar wrote:
> On 03-07-19, 14:41, Rajendra Nayak wrote:
>> []..
>>>
>>> Explain the rationale behind this code here in a comment.
>>>
>>>> + if (!_get_opp_count(opp_table)) {
>>>> + ret = _generic_set_opp_clk_only(dev, clk, freq);
>>>> + goto put_opp_table;
>>>> + }
>>>> +
>>>> temp_freq = old_freq;
>>>> old_opp = _find_freq_ceil(opp_table, &temp_freq);
>>>> if (IS_ERR(old_opp)) {
>>>
>>> Also, rebase over the OPP branch please:
>>
>> thanks, I will fix/rebase and repost,
>> in the meantime while I was testing this a little more I realized I also need
>> something like the change below to avoid a refcount mismatch WARN when empty OPP
>> table is removed using dev_pm_opp_of_remove_table()
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index fa7d4d6d37b3..20128a88baf2 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -2118,7 +2118,8 @@ void _dev_pm_opp_find_and_remove_table(struct device *dev)
>> return;
>> }
>> - _put_opp_list_kref(opp_table);
>> + if (_get_opp_count(opp_table))
>> + _put_opp_list_kref(opp_table);
>> /* Drop reference taken by _find_opp_table() */
>> dev_pm_opp_put_opp_table(opp_table);
>>
>> Does this look like a good way to fix it?
>
> No. If an OPP table only has dynamic OPPs, this will still generate
> warning. Below is the fix I would suggest. Please test it, I haven't
> tested it at all :)
thanks, yes, this seems to work.
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply
* Re: [patch 3/5] cpuidle: add haltpoll governor
From: Rafael J. Wysocki @ 2019-07-03 10:04 UTC (permalink / raw)
To: Marcelo Tosatti
Cc: kvm-devel, Linux PM, Paolo Bonzini, Radim Krcmar,
Andrea Arcangeli, Rafael J. Wysocki, Peter Zijlstra, Wanpeng Li,
Konrad Rzeszutek Wilk, Raslan KarimAllah, Boris Ostrovsky,
Ankur Arora, Christian Borntraeger
In-Reply-To: <20190701185528.153537911@asus.localdomain>
On Mon, Jul 1, 2019 at 8:57 PM Marcelo Tosatti <mtosatti@redhat.com> wrote:
>
> The cpuidle_haltpoll governor, in conjunction with the haltpoll cpuidle
> driver, allows guest vcpus to poll for a specified amount of time before
> halting.
> This provides the following benefits to host side polling:
>
> 1) The POLL flag is set while polling is performed, which allows
> a remote vCPU to avoid sending an IPI (and the associated
> cost of handling the IPI) when performing a wakeup.
>
> 2) The VM-exit cost can be avoided.
>
> The downside of guest side polling is that polling is performed
> even with other runnable tasks in the host.
>
> Results comparing halt_poll_ns and server/client application
> where a small packet is ping-ponged:
>
> host --> 31.33
> halt_poll_ns=300000 / no guest busy spin --> 33.40 (93.8%)
> halt_poll_ns=0 / guest_halt_poll_ns=300000 --> 32.73 (95.7%)
>
> For the SAP HANA benchmarks (where idle_spin is a parameter
> of the previous version of the patch, results should be the
> same):
>
> hpns == halt_poll_ns
>
> idle_spin=0/ idle_spin=800/ idle_spin=0/
> hpns=200000 hpns=0 hpns=800000
> DeleteC06T03 (100 thread) 1.76 1.71 (-3%) 1.78 (+1%)
> InsertC16T02 (100 thread) 2.14 2.07 (-3%) 2.18 (+1.8%)
> DeleteC00T01 (1 thread) 1.34 1.28 (-4.5%) 1.29 (-3.7%)
> UpdateC00T03 (1 thread) 4.72 4.18 (-12%) 4.53 (-5%)
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
>
> ---
> Documentation/virtual/guest-halt-polling.txt | 79 ++++++++++++
> drivers/cpuidle/Kconfig | 11 +
> drivers/cpuidle/governors/Makefile | 1
> drivers/cpuidle/governors/haltpoll.c | 175 +++++++++++++++++++++++++++
> 4 files changed, 266 insertions(+)
>
> Index: linux-2.6-newcpuidle.git/drivers/cpuidle/Kconfig
> ===================================================================
> --- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/Kconfig
> +++ linux-2.6-newcpuidle.git/drivers/cpuidle/Kconfig
> @@ -33,6 +33,17 @@ config CPU_IDLE_GOV_TEO
> Some workloads benefit from using it and it generally should be safe
> to use. Say Y here if you are not happy with the alternatives.
>
> +config CPU_IDLE_GOV_HALTPOLL
> + bool "Haltpoll governor (for virtualized systems)"
> + depends on KVM_GUEST
> + help
> + This governor implements haltpoll idle state selection, to be
> + used in conjunction with the haltpoll cpuidle driver, allowing
> + for polling for a certain amount of time before entering idle
> + state.
> +
> + Some virtualized workloads benefit from using it.
> +
> config DT_IDLE_STATES
> bool
>
> Index: linux-2.6-newcpuidle.git/drivers/cpuidle/governors/Makefile
> ===================================================================
> --- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/governors/Makefile
> +++ linux-2.6-newcpuidle.git/drivers/cpuidle/governors/Makefile
> @@ -6,3 +6,4 @@
> obj-$(CONFIG_CPU_IDLE_GOV_LADDER) += ladder.o
> obj-$(CONFIG_CPU_IDLE_GOV_MENU) += menu.o
> obj-$(CONFIG_CPU_IDLE_GOV_TEO) += teo.o
> +obj-$(CONFIG_CPU_IDLE_GOV_HALTPOLL) += haltpoll.o
> Index: linux-2.6-newcpuidle.git/drivers/cpuidle/governors/haltpoll.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6-newcpuidle.git/drivers/cpuidle/governors/haltpoll.c
> @@ -0,0 +1,176 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * haltpoll.c - haltpoll idle governor
> + *
> + * Copyright 2019 Red Hat, Inc. and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + *
> + * Authors: Marcelo Tosatti <mtosatti@redhat.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/cpuidle.h>
> +#include <linux/time.h>
> +#include <linux/ktime.h>
> +#include <linux/hrtimer.h>
> +#include <linux/tick.h>
> +#include <linux/sched.h>
> +#include <linux/module.h>
> +#include <linux/kvm_para.h>
> +
> +static unsigned int guest_halt_poll_us __read_mostly = 200;
> +module_param(guest_halt_poll_us, uint, 0644);
> +
> +/* division factor to shrink halt_poll_us */
> +static unsigned int guest_halt_poll_shrink __read_mostly = 2;
> +module_param(guest_halt_poll_shrink, uint, 0644);
> +
> +/* multiplication factor to grow per-cpu halt_poll_us */
> +static unsigned int guest_halt_poll_grow __read_mostly = 2;
> +module_param(guest_halt_poll_grow, uint, 0644);
> +
> +/* value in us to start growing per-cpu halt_poll_us */
> +static unsigned int guest_halt_poll_grow_start __read_mostly = 50;
> +module_param(guest_halt_poll_grow_start, uint, 0644);
> +
> +/* allow shrinking guest halt poll */
> +static bool guest_halt_poll_allow_shrink __read_mostly = true;
> +module_param(guest_halt_poll_allow_shrink, bool, 0644);
> +
> +struct haltpoll_device {
> + int last_state_idx;
> + unsigned int halt_poll_us;
> +};
Say you have poll_limit_ns in struct cpuidle_device as mentioned in
the other reply.
Since all of the existing governors use last_state_idx (or
equivalent), that could be moved to struct cpuidle_device too, in
principle.
Would you still need the new structure here then?
> +
> +static DEFINE_PER_CPU_ALIGNED(struct haltpoll_device, hpoll_devices);
> +
> +/**
> + * haltpoll_select - selects the next idle state to enter
> + * @drv: cpuidle driver containing state data
> + * @dev: the CPU
> + * @stop_tick: indication on whether or not to stop the tick
> + */
> +static int haltpoll_select(struct cpuidle_driver *drv,
> + struct cpuidle_device *dev,
> + bool *stop_tick)
> +{
> + struct haltpoll_device *hdev = this_cpu_ptr(&hpoll_devices);
> + int latency_req = cpuidle_governor_latency_req(dev->cpu);
> +
> + if (!drv->state_count || latency_req == 0) {
> + *stop_tick = false;
> + return 0;
> + }
> +
> + if (hdev->halt_poll_us == 0)
> + return 1;
> +
> + /* Last state was poll? */
> + if (hdev->last_state_idx == 0) {
> + /* Halt if no event occurred on poll window */
> + if (dev->poll_time_limit == true)
> + return 1;
> +
> + *stop_tick = false;
> + /* Otherwise, poll again */
> + return 0;
> + }
> +
> + *stop_tick = false;
> + /* Last state was halt: poll */
> + return 0;
> +}
> +
> +static void adjust_haltpoll_us(unsigned int block_us,
> + struct haltpoll_device *dev)
> +{
> + unsigned int val;
> +
> + /* Grow cpu_halt_poll_us if
> + * cpu_halt_poll_us < block_ns < guest_halt_poll_us
> + */
> + if (block_us > dev->halt_poll_us && block_us <= guest_halt_poll_us) {
> + val = dev->halt_poll_us * guest_halt_poll_grow;
> +
> + if (val < guest_halt_poll_grow_start)
> + val = guest_halt_poll_grow_start;
> + if (val > guest_halt_poll_us)
> + val = guest_halt_poll_us;
> +
> + dev->halt_poll_us = val;
> + } else if (block_us > guest_halt_poll_us &&
> + guest_halt_poll_allow_shrink) {
> + unsigned int shrink = guest_halt_poll_shrink;
> +
> + val = dev->halt_poll_us;
> + if (shrink == 0)
> + val = 0;
> + else
> + val /= shrink;
> + dev->halt_poll_us = val;
> + }
> +}
> +
> +/**
> + * haltpoll_reflect - update variables and update poll time
> + * @dev: the CPU
> + * @index: the index of actual entered state
> + */
> +static void haltpoll_reflect(struct cpuidle_device *dev, int index)
> +{
> + struct haltpoll_device *hdev = this_cpu_ptr(&hpoll_devices);
> +
> + hdev->last_state_idx = index;
> +
> + if (index != 0)
> + adjust_haltpoll_us(dev->last_residency, hdev);
> +}
> +
> +/**
> + * haltpoll_enable_device - scans a CPU's states and does setup
> + * @drv: cpuidle driver
> + * @dev: the CPU
> + */
> +static int haltpoll_enable_device(struct cpuidle_driver *drv,
> + struct cpuidle_device *dev)
> +{
> + struct haltpoll_device *hdev = &per_cpu(hpoll_devices, dev->cpu);
> +
> + memset(hdev, 0, sizeof(struct haltpoll_device));
> +
> + return 0;
> +}
> +
> +/**
> + * haltpoll_get_poll_time - return amount of poll time
> + * @drv: cpuidle driver
> + * @dev: the CPU
> + */
> +static u64 haltpoll_get_poll_time(struct cpuidle_driver *drv,
> + struct cpuidle_device *dev)
> +{
> + struct haltpoll_device *hdev = &per_cpu(hpoll_devices, dev->cpu);
> +
> + return hdev->halt_poll_us * NSEC_PER_USEC;
> +}
> +
> +static struct cpuidle_governor haltpoll_governor = {
> + .name = "haltpoll",
> + .rating = 21,
> + .enable = haltpoll_enable_device,
> + .select = haltpoll_select,
> + .reflect = haltpoll_reflect,
> + .get_poll_time = haltpoll_get_poll_time,
> +};
> +
> +static int __init init_haltpoll(void)
> +{
> + if (kvm_para_available())
> + return cpuidle_register_governor(&haltpoll_governor);
> +
> + return 0;
> +}
> +
> +postcore_initcall(init_haltpoll);
> Index: linux-2.6-newcpuidle.git/Documentation/virtual/guest-halt-polling.txt
> ===================================================================
> --- /dev/null
> +++ linux-2.6-newcpuidle.git/Documentation/virtual/guest-halt-polling.txt
> @@ -0,0 +1,79 @@
> +Guest halt polling
> +==================
> +
> +The cpuidle_haltpoll driver, with the haltpoll governor, allows
> +the guest vcpus to poll for a specified amount of time before
> +halting.
> +This provides the following benefits to host side polling:
> +
> + 1) The POLL flag is set while polling is performed, which allows
> + a remote vCPU to avoid sending an IPI (and the associated
> + cost of handling the IPI) when performing a wakeup.
> +
> + 2) The VM-exit cost can be avoided.
> +
> +The downside of guest side polling is that polling is performed
> +even with other runnable tasks in the host.
> +
> +The basic logic as follows: A global value, guest_halt_poll_us,
> +is configured by the user, indicating the maximum amount of
> +time polling is allowed. This value is fixed.
> +
> +Each vcpu has an adjustable guest_halt_poll_us
> +("per-cpu guest_halt_poll_us"), which is adjusted by the algorithm
> +in response to events (explained below).
> +
> +Module Parameters
> +=================
> +
> +The haltpoll governor has 5 tunable module parameters:
> +
> +1) guest_halt_poll_us:
> +Maximum amount of time, in microseconds, that polling is
> +performed before halting.
> +
> +Default: 200
> +
> +2) guest_halt_poll_shrink:
> +Division factor used to shrink per-cpu guest_halt_poll_us when
> +wakeup event occurs after the global guest_halt_poll_us.
> +
> +Default: 2
> +
> +3) guest_halt_poll_grow:
> +Multiplication factor used to grow per-cpu guest_halt_poll_us
> +when event occurs after per-cpu guest_halt_poll_us
> +but before global guest_halt_poll_us.
> +
> +Default: 2
> +
> +4) guest_halt_poll_grow_start:
> +The per-cpu guest_halt_poll_us eventually reaches zero
> +in case of an idle system. This value sets the initial
> +per-cpu guest_halt_poll_us when growing. This can
> +be increased from 10, to avoid misses during the initial
> +growth stage:
> +
> +10, 20, 40, ... (example assumes guest_halt_poll_grow=2).
> +
> +Default: 50
> +
> +5) guest_halt_poll_allow_shrink:
> +
> +Bool parameter which allows shrinking. Set to N
> +to avoid it (per-cpu guest_halt_poll_us will remain
> +high once achieves global guest_halt_poll_us value).
> +
> +Default: Y
> +
> +The module parameters can be set from the debugfs files in:
> +
> + /sys/module/haltpoll/parameters/
> +
> +Further Notes
> +=============
> +
> +- Care should be taken when setting the guest_halt_poll_us parameter as a
> +large value has the potential to drive the cpu usage to 100% on a machine which
> +would be almost entirely idle otherwise.
> +
>
>
^ permalink raw reply
* Re: [patch 1/5] add cpuidle-haltpoll driver
From: Rafael J. Wysocki @ 2019-07-03 9:54 UTC (permalink / raw)
To: Marcelo Tosatti
Cc: kvm-devel, Linux PM, Paolo Bonzini, Radim Krcmar,
Andrea Arcangeli, Rafael J. Wysocki, Peter Zijlstra, Wanpeng Li,
Konrad Rzeszutek Wilk, Raslan KarimAllah, Boris Ostrovsky,
Ankur Arora, Christian Borntraeger
In-Reply-To: <20190701185528.078114514@asus.localdomain>
On Mon, Jul 1, 2019 at 8:57 PM Marcelo Tosatti <mtosatti@redhat.com> wrote:
>
> Add a cpuidle driver that calls the architecture default_idle routine.
>
> To be used in conjunction with the haltpoll governor.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> ---
> arch/x86/kernel/process.c | 2 -
> drivers/cpuidle/Kconfig | 9 +++++
> drivers/cpuidle/Makefile | 1
> drivers/cpuidle/cpuidle-haltpoll.c | 65 +++++++++++++++++++++++++++++++++++++
> 4 files changed, 76 insertions(+), 1 deletion(-)
>
> Index: linux-2.6-newcpuidle.git/arch/x86/kernel/process.c
> ===================================================================
> --- linux-2.6-newcpuidle.git.orig/arch/x86/kernel/process.c
> +++ linux-2.6-newcpuidle.git/arch/x86/kernel/process.c
> @@ -580,7 +580,7 @@ void __cpuidle default_idle(void)
> safe_halt();
> trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
> }
> -#ifdef CONFIG_APM_MODULE
> +#if defined(CONFIG_APM_MODULE) || defined(CONFIG_HALTPOLL_CPUIDLE_MODULE)
> EXPORT_SYMBOL(default_idle);
> #endif
>
> Index: linux-2.6-newcpuidle.git/drivers/cpuidle/Kconfig
> ===================================================================
> --- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/Kconfig
> +++ linux-2.6-newcpuidle.git/drivers/cpuidle/Kconfig
> @@ -51,6 +51,15 @@ depends on PPC
> source "drivers/cpuidle/Kconfig.powerpc"
> endmenu
>
> +config HALTPOLL_CPUIDLE
> + tristate "Halt poll cpuidle driver"
> + depends on X86 && KVM_GUEST
> + default y
> + help
> + This option enables halt poll cpuidle driver, which allows to poll
> + before halting in the guest (more efficient than polling in the
> + host via halt_poll_ns for some scenarios).
> +
> endif
>
> config ARCH_NEEDS_CPU_IDLE_COUPLED
> Index: linux-2.6-newcpuidle.git/drivers/cpuidle/Makefile
> ===================================================================
> --- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/Makefile
> +++ linux-2.6-newcpuidle.git/drivers/cpuidle/Makefile
> @@ -7,6 +7,7 @@ obj-y += cpuidle.o driver.o governor.o s
> obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
> obj-$(CONFIG_DT_IDLE_STATES) += dt_idle_states.o
> obj-$(CONFIG_ARCH_HAS_CPU_RELAX) += poll_state.o
> +obj-$(CONFIG_HALTPOLL_CPUIDLE) += cpuidle-haltpoll.o
>
> ##################################################################################
> # ARM SoC drivers
> Index: linux-2.6-newcpuidle.git/drivers/cpuidle/cpuidle-haltpoll.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6-newcpuidle.git/drivers/cpuidle/cpuidle-haltpoll.c
> @@ -0,0 +1,69 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * cpuidle driver for haltpoll governor.
> + *
> + * Copyright 2019 Red Hat, Inc. and/or its affiliates.
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2. See
> + * the COPYING file in the top-level directory.
> + *
> + * Authors: Marcelo Tosatti <mtosatti@redhat.com>
> + */
> +
> +#include <linux/init.h>
> +#include <linux/cpuidle.h>
> +#include <linux/module.h>
> +#include <linux/sched/idle.h>
> +#include <linux/kvm_para.h>
> +
> +static int default_enter_idle(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int index)
> +{
> + if (current_clr_polling_and_test()) {
> + local_irq_enable();
> + return index;
> + }
> + default_idle();
> + return index;
> +}
> +
> +static struct cpuidle_driver haltpoll_driver = {
> + .name = "haltpoll",
> + .owner = THIS_MODULE,
> + .states = {
> + { /* entry 0 is for polling */ },
> + {
> + .enter = default_enter_idle,
> + .exit_latency = 1,
> + .target_residency = 1,
> + .power_usage = -1,
> + .name = "haltpoll idle",
> + .desc = "default architecture idle",
> + },
> + },
> + .safe_state_index = 0,
> + .state_count = 2,
> +};
> +
> +static int __init haltpoll_init(void)
> +{
> + struct cpuidle_driver *drv = &haltpoll_driver;
> +
> + cpuidle_poll_state_init(drv);
> +
> + if (!kvm_para_available())
> + return 0;
> +
> + return cpuidle_register(&haltpoll_driver, NULL);
> +}
> +
> +static void __exit haltpoll_exit(void)
> +{
> + cpuidle_unregister(&haltpoll_driver);
> +}
> +
> +module_init(haltpoll_init);
> +module_exit(haltpoll_exit);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Marcelo Tosatti <mtosatti@redhat.com>");
> +
>
>
^ permalink raw reply
* Re: [patch 2/5] cpuidle: add get_poll_time callback
From: Rafael J. Wysocki @ 2019-07-03 9:50 UTC (permalink / raw)
To: Marcelo Tosatti
Cc: kvm-devel, Linux PM, Paolo Bonzini, Radim Krcmar,
Andrea Arcangeli, Rafael J. Wysocki, Peter Zijlstra, Wanpeng Li,
Konrad Rzeszutek Wilk, Raslan KarimAllah, Boris Ostrovsky,
Ankur Arora, Christian Borntraeger
In-Reply-To: <20190701185528.115106953@asus.localdomain>
On Mon, Jul 1, 2019 at 8:57 PM Marcelo Tosatti <mtosatti@redhat.com> wrote:
>
> Add a "get_poll_time" callback to the cpuidle_governor structure,
> and change poll state to poll for that amount of time.
>
> Provide a default method for it, while allowing individual governors
> to override it.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
I had ACKed this before, but then it occurred to me that it would be
less intrusive to use a new field, say poll_limit_ns (equal to 0 by
default), in struct cpuidle_device.
>
> ---
> drivers/cpuidle/cpuidle.c | 40 ++++++++++++++++++++++++++++++++++++++++
> drivers/cpuidle/poll_state.c | 11 ++---------
> include/linux/cpuidle.h | 8 ++++++++
> 3 files changed, 50 insertions(+), 9 deletions(-)
>
> Index: linux-2.6-newcpuidle.git/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/cpuidle.c
> +++ linux-2.6-newcpuidle.git/drivers/cpuidle/cpuidle.c
> @@ -362,6 +362,46 @@ void cpuidle_reflect(struct cpuidle_devi
> }
>
> /**
> + * cpuidle_default_poll_time - default routine used to return poll time
> + * governors can override it if necessary
> + *
> + * @drv: the cpuidle driver tied with the cpu
> + * @dev: the cpuidle device
> + *
> + */
> +static u64 cpuidle_default_poll_time(struct cpuidle_driver *drv,
> + struct cpuidle_device *dev)
With this new field in place this could be called cpuidle_poll_time() and ->
> +{
> + int i;
-> do something like this here:
if (dev->poll_limit_ns)
return dev->poll_limit_ns;
and the governor changes below wouldn't be necessary any more.
Then, the governor could update poll_limit_ns if it wanted to override
the default.
It also would be possible to use poll_limit_ns as a sort of poll limit
cache to store the last value in it and clear it on state
disable/enable to avoid the search through the states every time even
without haltpoll.
> +
> + for (i = 1; i < drv->state_count; i++) {
> + if (drv->states[i].disabled || dev->states_usage[i].disable)
> + continue;
> +
> + return (u64)drv->states[i].target_residency * NSEC_PER_USEC;
> + }
> +
> + return TICK_NSEC;
> +}
> +
> +/**
> + * cpuidle_get_poll_time - tell the polling driver how much time to poll,
> + * in nanoseconds.
> + *
> + * @drv: the cpuidle driver tied with the cpu
> + * @dev: the cpuidle device
> + *
> + */
> +u64 cpuidle_get_poll_time(struct cpuidle_driver *drv,
> + struct cpuidle_device *dev)
> +{
> + if (cpuidle_curr_governor->get_poll_time)
> + return cpuidle_curr_governor->get_poll_time(drv, dev);
> +
> + return cpuidle_default_poll_time(drv, dev);
> +}
> +
> +/**
> * cpuidle_install_idle_handler - installs the cpuidle idle loop handler
> */
> void cpuidle_install_idle_handler(void)
> Index: linux-2.6-newcpuidle.git/drivers/cpuidle/poll_state.c
> ===================================================================
> --- linux-2.6-newcpuidle.git.orig/drivers/cpuidle/poll_state.c
> +++ linux-2.6-newcpuidle.git/drivers/cpuidle/poll_state.c
> @@ -20,16 +20,9 @@ static int __cpuidle poll_idle(struct cp
> local_irq_enable();
> if (!current_set_polling_and_test()) {
> unsigned int loop_count = 0;
> - u64 limit = TICK_NSEC;
> - int i;
> + u64 limit;
>
> - for (i = 1; i < drv->state_count; i++) {
> - if (drv->states[i].disabled || dev->states_usage[i].disable)
> - continue;
> -
> - limit = (u64)drv->states[i].target_residency * NSEC_PER_USEC;
> - break;
> - }
> + limit = cpuidle_get_poll_time(drv, dev);
>
> while (!need_resched()) {
> cpu_relax();
> Index: linux-2.6-newcpuidle.git/include/linux/cpuidle.h
> ===================================================================
> --- linux-2.6-newcpuidle.git.orig/include/linux/cpuidle.h
> +++ linux-2.6-newcpuidle.git/include/linux/cpuidle.h
> @@ -132,6 +132,8 @@ extern int cpuidle_select(struct cpuidle
> extern int cpuidle_enter(struct cpuidle_driver *drv,
> struct cpuidle_device *dev, int index);
> extern void cpuidle_reflect(struct cpuidle_device *dev, int index);
> +extern u64 cpuidle_get_poll_time(struct cpuidle_driver *drv,
> + struct cpuidle_device *dev);
>
> extern int cpuidle_register_driver(struct cpuidle_driver *drv);
> extern struct cpuidle_driver *cpuidle_get_driver(void);
> @@ -166,6 +168,9 @@ static inline int cpuidle_enter(struct c
> struct cpuidle_device *dev, int index)
> {return -ENODEV; }
> static inline void cpuidle_reflect(struct cpuidle_device *dev, int index) { }
> +extern u64 cpuidle_get_poll_time(struct cpuidle_driver *drv,
> + struct cpuidle_device *dev)
> +{return 0; }
> static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
> {return -ENODEV; }
> static inline struct cpuidle_driver *cpuidle_get_driver(void) {return NULL; }
> @@ -246,6 +251,9 @@ struct cpuidle_governor {
> struct cpuidle_device *dev,
> bool *stop_tick);
> void (*reflect) (struct cpuidle_device *dev, int index);
> +
> + u64 (*get_poll_time) (struct cpuidle_driver *drv,
> + struct cpuidle_device *dev);
> };
>
> #ifdef CONFIG_CPU_IDLE
>
>
^ permalink raw reply
* Re: [PATCH 2/2] opp: Manage empty OPP tables with clk handle
From: Viresh Kumar @ 2019-07-03 9:47 UTC (permalink / raw)
To: Rajendra Nayak; +Cc: vireshk, sboyd, linux-pm, linux-kernel, linux-arm-msm
In-Reply-To: <95cf4e44-d57a-9aa4-40ce-3b7013e10813@codeaurora.org>
On 03-07-19, 14:41, Rajendra Nayak wrote:
> []..
> >
> > Explain the rationale behind this code here in a comment.
> >
> > > + if (!_get_opp_count(opp_table)) {
> > > + ret = _generic_set_opp_clk_only(dev, clk, freq);
> > > + goto put_opp_table;
> > > + }
> > > +
> > > temp_freq = old_freq;
> > > old_opp = _find_freq_ceil(opp_table, &temp_freq);
> > > if (IS_ERR(old_opp)) {
> >
> > Also, rebase over the OPP branch please:
>
> thanks, I will fix/rebase and repost,
> in the meantime while I was testing this a little more I realized I also need
> something like the change below to avoid a refcount mismatch WARN when empty OPP
> table is removed using dev_pm_opp_of_remove_table()
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index fa7d4d6d37b3..20128a88baf2 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -2118,7 +2118,8 @@ void _dev_pm_opp_find_and_remove_table(struct device *dev)
> return;
> }
> - _put_opp_list_kref(opp_table);
> + if (_get_opp_count(opp_table))
> + _put_opp_list_kref(opp_table);
> /* Drop reference taken by _find_opp_table() */
> dev_pm_opp_put_opp_table(opp_table);
>
> Does this look like a good way to fix it?
No. If an OPP table only has dynamic OPPs, this will still generate
warning. Below is the fix I would suggest. Please test it, I haven't
tested it at all :)
--
viresh
-------------------------8<-------------------------
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 3 Jul 2019 15:03:14 +0530
Subject: [PATCH] opp: Don't decrement uninitialized list_kref
The list_kref was added for static OPPs and to track their users. The
kref is initialized while the static OPPs are added, but removed
unconditionally even if the static OPPs were never added. This causes
refcount mismatch warnings currently.
Fix that by always initializing the kref when the OPP table is first
initialized. The refcount is later incremented only for the second user
onwards.
Fixes: d0e8ae6c26da ("OPP: Create separate kref for static OPPs list")
Reported-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 1 +
drivers/opp/of.c | 21 ++++-----------------
2 files changed, 5 insertions(+), 17 deletions(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 89ec6aa220cf..2958cc7bbb58 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -943,6 +943,7 @@ static struct opp_table *_allocate_opp_table(struct device *dev, int index)
BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
INIT_LIST_HEAD(&opp_table->opp_list);
kref_init(&opp_table->kref);
+ kref_init(&opp_table->list_kref);
/* Secure the device table modification */
list_add(&opp_table->node, &opp_tables);
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index a637f30552a3..bf62b357437c 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -665,8 +665,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
return 0;
}
- kref_init(&opp_table->list_kref);
-
/* We have opp-table node now, iterate over it and add OPPs */
for_each_available_child_of_node(opp_table->np, np) {
opp = _opp_add_static_v2(opp_table, dev, np);
@@ -675,17 +673,15 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
dev_err(dev, "%s: Failed to add OPP, %d\n", __func__,
ret);
of_node_put(np);
- goto put_list_kref;
+ return ret;
} else if (opp) {
count++;
}
}
/* There should be one of more OPP defined */
- if (WARN_ON(!count)) {
- ret = -ENOENT;
- goto put_list_kref;
- }
+ if (WARN_ON(!count))
+ return -ENOENT;
list_for_each_entry(opp, &opp_table->opp_list, node)
pstate_count += !!opp->pstate;
@@ -694,8 +690,7 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
if (pstate_count && pstate_count != count) {
dev_err(dev, "Not all nodes have performance state set (%d: %d)\n",
count, pstate_count);
- ret = -ENOENT;
- goto put_list_kref;
+ return -ENOENT;
}
if (pstate_count)
@@ -704,11 +699,6 @@ static int _of_add_opp_table_v2(struct device *dev, struct opp_table *opp_table)
opp_table->parsed_static_opps = true;
return 0;
-
-put_list_kref:
- _put_opp_list_kref(opp_table);
-
- return ret;
}
/* Initializes OPP tables based on old-deprecated bindings */
@@ -734,8 +724,6 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
return -EINVAL;
}
- kref_init(&opp_table->list_kref);
-
val = prop->value;
while (nr) {
unsigned long freq = be32_to_cpup(val++) * 1000;
@@ -745,7 +733,6 @@ static int _of_add_opp_table_v1(struct device *dev, struct opp_table *opp_table)
if (ret) {
dev_err(dev, "%s: Failed to add OPP %ld (%d)\n",
__func__, freq, ret);
- _put_opp_list_kref(opp_table);
return ret;
}
nr -= 2;
^ permalink raw reply related
* Re: [PATCH 2/2] opp: Manage empty OPP tables with clk handle
From: Rajendra Nayak @ 2019-07-03 9:11 UTC (permalink / raw)
To: Viresh Kumar; +Cc: vireshk, sboyd, linux-pm, linux-kernel, linux-arm-msm
In-Reply-To: <20190703085026.xe3hwxqah76b7np3@vireshk-i7>
[]..
>>
>
> Explain the rationale behind this code here in a comment.
>
>> + if (!_get_opp_count(opp_table)) {
>> + ret = _generic_set_opp_clk_only(dev, clk, freq);
>> + goto put_opp_table;
>> + }
>> +
>> temp_freq = old_freq;
>> old_opp = _find_freq_ceil(opp_table, &temp_freq);
>> if (IS_ERR(old_opp)) {
>
> Also, rebase over the OPP branch please:
thanks, I will fix/rebase and repost,
in the meantime while I was testing this a little more I realized I also need
something like the change below to avoid a refcount mismatch WARN when empty OPP
table is removed using dev_pm_opp_of_remove_table()
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index fa7d4d6d37b3..20128a88baf2 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2118,7 +2118,8 @@ void _dev_pm_opp_find_and_remove_table(struct device *dev)
return;
}
- _put_opp_list_kref(opp_table);
+ if (_get_opp_count(opp_table))
+ _put_opp_list_kref(opp_table);
/* Drop reference taken by _find_opp_table() */
dev_pm_opp_put_opp_table(opp_table);
Does this look like a good way to fix it?
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply related
* Re: [PATCH v2] PM: Move disabling/enabling runtime PM to suspend/resume noirq
From: Rafael J. Wysocki @ 2019-07-03 9:07 UTC (permalink / raw)
To: Muchun Song
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Len Brown, Pavel Machek,
Greg Kroah-Hartman, Linux PM, Linux Kernel Mailing List
In-Reply-To: <CAPSr9jE6Qfu4NpmDfUrS+hVad2OP+xr00yoJ3k5=e=f7ZKwY+g@mail.gmail.com>
On Wed, Jul 3, 2019 at 2:56 AM Muchun Song <smuchun@gmail.com> wrote:
>
> Rafael J. Wysocki <rafael@kernel.org> 于2019年7月3日周三 上午1:54写道：
> >
> > On Tue, Jul 2, 2019 at 6:37 PM Muchun Song <smuchun@gmail.com> wrote:
> > >
> > > Currently, the PM core disables runtime PM for all devices right after
> > > executing subsystem/driver .suspend_late() callbacks for them and
> > > re-enables it right before executing subsystem/driver .resume_early()
> > > callbacks for them. This may lead to problems when there are two devices
> > > such that the irq handler thread function executed for one of them
> > > depends on runtime PM working for the other. E.g. There are two devices,
> > > one is i2c slave device depends on another device which can be the i2c
> > > adapter device. The slave device can generate system wakeup signals and
> > > is enabled to wake up the system(via call enable_irq_wake()). So, the irq
> > > of slave device is enabled. If a wakeup signal generate after executing
> > > subsystem/driver .suspend_late() callbacks. Then, the irq handler thread
> > > function will be called(The irq is requested via request_threaded_irq())
> > > and the slave device reads data via i2c adapter device(via i2c_transfer()).
> > > In that case, it may be failed to read data because of the runtime PM
> > > disabled.
> > >
> > > It is also analogously for resume. If a wakeup signal generate when the
> > > system is in the sleep state. The irq handler thread function may be
> > > called before executing subsystem/driver .resume_early(). In that case,
> > > it also may be failed to read data because of the runtime PM disabled.
> > >
> >
> > This has been discussed for a number of times, documented and no, I'm
> > not going to apply this patch.
>
> Thanks for your reply. I want to know why we can't do that, so where
> can I find the discussion?
The discussions rather.
You need to search email archives, especially for the reason why the
"late" and "early" callbacks have been added. There was a conference
presentation about that even.
The bottom line is that the change you are proposing cannot be made.
> > PM-runtime cannot be relied on during the "noirq" stages of suspend
> > and resume, which is why it is disabled by the core in the "late" and
> > "early" stages, respectively.
> >
>
> What better solution do we have for the example I am talking about
> which is described in the commit message?
It's not "what better solution is there", because what you are
proposing would simply not work.
Basically, if there are devices that may need the given one in the
"noirq" resume phase, this device needs to be resumed upfront. It may
be runtime-suspended again in the "early" resume phase.
^ permalink raw reply
* Re: [PATCH 09/13] intel_rapl: abstract RAPL common code
From: Rafael J. Wysocki @ 2019-07-03 9:02 UTC (permalink / raw)
To: Zhang Rui
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux PM,
Pandruvada, Srinivas
In-Reply-To: <1562142196.3256.15.camel@intel.com>
On Wed, Jul 3, 2019 at 10:23 AM Zhang Rui <rui.zhang@intel.com> wrote:
>
> On 二, 2019-07-02 at 23:59 +0200, Rafael J. Wysocki wrote:
> > On Fri, Jun 28, 2019 at 7:50 AM Zhang Rui <rui.zhang@intel.com>
> > wrote:
> > >
> > >
> > > Split intel_rapl.c to intel_rapl_common.c and intel_rapl.c, where
> > > intel_rapl_common.c contains the common code that can be used by
> > > both MSR
> > > and MMIO interface.
> > > intel_rapl.c still contains the implementation of RAPL MSR
> > > interface.
> > >
> > > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > > ---
> > > drivers/powercap/Kconfig | 11 +-
> > > drivers/powercap/Makefile | 1 +
> > > drivers/powercap/intel_rapl.c | 1427 +---------------------
> > > -----------
> > > drivers/powercap/intel_rapl_common.c | 1468
> > > ++++++++++++++++++++++++++++++++++
> > intel_rapl_common.c and intel_rapl_msr.c, please.
> >
> I kept the MSR RAPL interface in intel_rapl.c for compatibility reason,
> and that was made when the module autoloading issue is not fixed, so
> people can still use "modprobe intel_rapl" manually to get the MSR RAPL
> interface.
The module still can be called intel_rapl.o.
^ permalink raw reply
* Re: [PATCH 2/2] opp: Manage empty OPP tables with clk handle
From: Viresh Kumar @ 2019-07-03 8:50 UTC (permalink / raw)
To: Rajendra Nayak; +Cc: vireshk, sboyd, linux-pm, linux-kernel, linux-arm-msm
In-Reply-To: <20190702043643.1746-2-rnayak@codeaurora.org>
On 02-07-19, 10:06, Rajendra Nayak wrote:
> With OPP core now supporting DVFS for IO devices, we have instances of
> IO devices (same IP block) with require an OPP on some platforms/SoCs
which
> while just needing to scale the clock on some others.
Blank line here.
> In order to avoid conditional code in every driver, (to check for
remove ,
> availability of OPPs and then deciding to do either dev_pm_opp_set_rate()
> or clk_set_rate()) add support to manage empty OPP tables with a clk handle.
Blank line here.
> This makes dev_pm_opp_set_rate() equivalent of a clk_set_rate() for devices
> with just a clk and no OPPs specified.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
> drivers/opp/core.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index ae033bb1e5b7..fa7d4d6d37b3 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -801,6 +801,11 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
> goto put_opp_table;
> }
>
Explain the rationale behind this code here in a comment.
> + if (!_get_opp_count(opp_table)) {
> + ret = _generic_set_opp_clk_only(dev, clk, freq);
> + goto put_opp_table;
> + }
> +
> temp_freq = old_freq;
> old_opp = _find_freq_ceil(opp_table, &temp_freq);
> if (IS_ERR(old_opp)) {
Also, rebase over the OPP branch please:
git://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git opp/linux-next
or pm/linux-next
--
viresh
^ permalink raw reply
* [PATCH] power: supply: max77650: add MODULE_ALIAS()
From: Bartosz Golaszewski @ 2019-07-03 8:48 UTC (permalink / raw)
To: Sebastian Reichel; +Cc: linux-kernel, linux-pm, Bartosz Golaszewski
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Define a MODULE_ALIAS() in the charger sub-driver for max77650 so that
the appropriate module gets loaded together with the core mfd driver.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
drivers/power/supply/max77650-charger.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/power/supply/max77650-charger.c b/drivers/power/supply/max77650-charger.c
index e34714cb05ec..5f9477c5cf5a 100644
--- a/drivers/power/supply/max77650-charger.c
+++ b/drivers/power/supply/max77650-charger.c
@@ -366,3 +366,4 @@ module_platform_driver(max77650_charger_driver);
MODULE_DESCRIPTION("MAXIM 77650/77651 charger driver");
MODULE_AUTHOR("Bartosz Golaszewski <bgolaszewski@baylibre.com>");
MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:max77650-charger");
--
2.21.0
^ permalink raw reply related
* Re: [PATCH 09/13] intel_rapl: abstract RAPL common code
From: Zhang Rui @ 2019-07-03 8:23 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Rafael J. Wysocki, Linux PM, Pandruvada, Srinivas
In-Reply-To: <CAJZ5v0gEW_9UaZDnJdz60e6baEYQOuG-SdZu54tswho6J+zYkQ@mail.gmail.com>
On 二, 2019-07-02 at 23:59 +0200, Rafael J. Wysocki wrote:
> On Fri, Jun 28, 2019 at 7:50 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> >
> >
> > Split intel_rapl.c to intel_rapl_common.c and intel_rapl.c, where
> > intel_rapl_common.c contains the common code that can be used by
> > both MSR
> > and MMIO interface.
> > intel_rapl.c still contains the implementation of RAPL MSR
> > interface.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> > drivers/powercap/Kconfig | 11 +-
> > drivers/powercap/Makefile | 1 +
> > drivers/powercap/intel_rapl.c | 1427 +---------------------
> > -----------
> > drivers/powercap/intel_rapl_common.c | 1468
> > ++++++++++++++++++++++++++++++++++
> intel_rapl_common.c and intel_rapl_msr.c, please.
>
I kept the MSR RAPL interface in intel_rapl.c for compatibility reason,
and that was made when the module autoloading issue is not fixed, so
people can still use "modprobe intel_rapl" manually to get the MSR RAPL
interface.
Now given that the MSR RAPL interface driver can be autoloaded, I think
it is okay to use intel_rapl_msr.c and intel_rapl_common.c.
Srinivas, is there anything else prevents us from using a new module
name for MSR RAPL interface?
thanks,
rui
^ permalink raw reply
* Re: [PATCH 04/13] intel_rapl: introduce struct rapl_private
From: Zhang Rui @ 2019-07-03 8:14 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Rafael J. Wysocki, Linux PM, Pandruvada, Srinivas
In-Reply-To: <CAJZ5v0he3Qs_nZmTN9fcgVAnurwXOb+Dj+NnTvUqT8TCH3oNHQ@mail.gmail.com>
On 二, 2019-07-02 at 23:44 +0200, Rafael J. Wysocki wrote:
> On Fri, Jun 28, 2019 at 7:50 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> >
> >
> > Introduce a new structure, rapl_private, to save the private data
> > for different RAPL Interface.
> The new structure is called rapl_priv in the patch.
>
> Besides, I would call it rapl_if_priv to indicate that this related
> to
> an interface.
>
> Also, IMO there should be a kerneldoc comment for it.
>
okay.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> > drivers/powercap/intel_rapl.c | 58 +++++++++++++++++++++--------
> > --------------
> > include/linux/intel_rapl.h | 7 ++++++
> > 2 files changed, 35 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/powercap/intel_rapl.c
> > b/drivers/powercap/intel_rapl.c
> > index adb35ec..a804370 100644
> > --- a/drivers/powercap/intel_rapl.c
> > +++ b/drivers/powercap/intel_rapl.c
> > @@ -75,6 +75,8 @@ enum unit_type {
> > TIME_UNIT,
> > };
> >
> > +static struct rapl_priv rapl_msr_priv;
> Please add a comment describing this variable and its role.
>
> BTW, I'd rather call it rapl_if_msr.
>
> >
> > +
> > /* per domain data, some are optional */
> > #define NR_RAW_PRIMITIVES (NR_RAPL_PRIMITIVES - 2)
> >
> > @@ -155,17 +157,14 @@ static const char * const rapl_domain_names[]
> > = {
> > "psys",
> > };
> >
> > -static struct powercap_control_type *control_type; /* PowerCap
> > Controller */
> > -static struct rapl_domain *platform_rapl_domain; /* Platform
> > (PSys) domain */
> Why do these things need to go into rapl_priv?
>
different RAPL interface may have its own control_type,
"intel_rapl_msr" vs. "intel_rapl_mmio".
MMIO RAPL interface does not have platform rapl domain so far, but
given that the current platform rapl domain uses
MSR_PLATFORM_POWER_LIMIT and MSR_PLATFORM_ENERGY_STATUS.
I made it part of MSR RAPL interface driver.
thanks,
rui
^ permalink raw reply
* Re: [PATCH 06/13] intel_rapl: abstract register access operations
From: Zhang Rui @ 2019-07-03 8:14 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Rafael J. Wysocki, Linux PM, Pandruvada, Srinivas
In-Reply-To: <CAJZ5v0junxx01o8fn-UsoGuuEr18zCqPZ5hWMAD6c=Z-1JNvVA@mail.gmail.com>
On 二, 2019-07-02 at 23:56 +0200, Rafael J. Wysocki wrote:
> On Fri, Jun 28, 2019 at 7:50 AM Zhang Rui <rui.zhang@intel.com>
> wrote:
> >
> >
> > MSR and MMIO RAPL interfaces have different ways to access the
> > registers,
> > thus in order to abstract the register access operations, two
> > callbacks,
> > .read_raw()/.write_raw() are introduced, and they should be
> > implemented by
> > MSR RAPL and MMIO RAPL interface driver respectly.
> However, this patch implements them for the MSR I/F only.
>
Right, will add this in the changelog.
> >
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> > drivers/powercap/intel_rapl.c | 111 ++++++++++++++++++++++------
> > --------------
> > include/linux/intel_rapl.h | 9 ++++
> > 2 files changed, 67 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/powercap/intel_rapl.c
> > b/drivers/powercap/intel_rapl.c
> > index 70990ff..7dc9965 100644
> > --- a/drivers/powercap/intel_rapl.c
> > +++ b/drivers/powercap/intel_rapl.c
> > @@ -92,13 +92,6 @@ static struct rapl_priv rapl_msr_priv = {
> > /* per domain data, some are optional */
> > #define NR_RAW_PRIMITIVES (NR_RAPL_PRIMITIVES - 2)
> >
> > -struct msrl_action {
> > - u32 msr_no;
> > - u64 clear_mask;
> > - u64 set_mask;
> > - int err;
> > -};
> > -
> > #define DOMAIN_STATE_INACTIVE BIT(0)
> > #define DOMAIN_STATE_POWER_LIMIT_SET BIT(1)
> > #define DOMAIN_STATE_BIOS_LOCKED BIT(2)
> > @@ -691,16 +684,16 @@ static int rapl_read_data_raw(struct
> > rapl_domain *rd,
> > enum rapl_primitives prim,
> > bool xlate, u64 *data)
> > {
> > - u64 value, final;
> > - u32 msr;
> > + u64 value;
> > struct rapl_primitive_info *rp = &rpi[prim];
> > + struct reg_action ra;
> > int cpu;
> >
> > if (!rp->name || rp->flag & RAPL_PRIMITIVE_DUMMY)
> > return -EINVAL;
> >
> > - msr = rd->regs[rp->id];
> > - if (!msr)
> > + ra.reg = rd->regs[rp->id];
> > + if (!ra.reg)
> > return -EINVAL;
> >
> > cpu = rd->rp->lead_cpu;
> > @@ -716,47 +709,23 @@ static int rapl_read_data_raw(struct
> > rapl_domain *rd,
> > return 0;
> > }
> >
> > - if (rdmsrl_safe_on_cpu(cpu, msr, &value)) {
> > - pr_debug("failed to read msr 0x%x on cpu %d\n",
> > msr, cpu);
> > + ra.mask = rp->mask;
> > +
> > + if (rd->rp->priv->read_raw(cpu, &ra)) {
> > + pr_debug("failed to read reg 0x%x on cpu %d\n",
> > ra.reg, cpu);
> > return -EIO;
> > }
> >
> > - final = value & rp->mask;
> > - final = final >> rp->shift;
> > + value = ra.value >> rp->shift;
> > +
> > if (xlate)
> > - *data = rapl_unit_xlate(rd, rp->unit, final, 0);
> > + *data = rapl_unit_xlate(rd, rp->unit, value, 0);
> > else
> > - *data = final;
> > + *data = value;
> >
> > return 0;
> > }
> >
> > -
> > -static int msrl_update_safe(u32 msr_no, u64 clear_mask, u64
> > set_mask)
> > -{
> > - int err;
> > - u64 val;
> > -
> > - err = rdmsrl_safe(msr_no, &val);
> > - if (err)
> > - goto out;
> > -
> > - val &= ~clear_mask;
> > - val |= set_mask;
> > -
> > - err = wrmsrl_safe(msr_no, val);
> > -
> > -out:
> > - return err;
> > -}
> > -
> > -static void msrl_update_func(void *info)
> > -{
> > - struct msrl_action *ma = info;
> > -
> > - ma->err = msrl_update_safe(ma->msr_no, ma->clear_mask, ma-
> > >set_mask);
> > -}
> > -
> > /* Similar use of primitive info in the read counterpart */
> > static int rapl_write_data_raw(struct rapl_domain *rd,
> > enum rapl_primitives prim,
> > @@ -765,7 +734,7 @@ static int rapl_write_data_raw(struct
> > rapl_domain *rd,
> > struct rapl_primitive_info *rp = &rpi[prim];
> > int cpu;
> > u64 bits;
> > - struct msrl_action ma;
> > + struct reg_action ra;
> > int ret;
> >
> > cpu = rd->rp->lead_cpu;
> > @@ -773,17 +742,13 @@ static int rapl_write_data_raw(struct
> > rapl_domain *rd,
> > bits <<= rp->shift;
> > bits &= rp->mask;
> >
> > - memset(&ma, 0, sizeof(ma));
> > + memset(&ra, 0, sizeof(ra));
> >
> > - ma.msr_no = rd->regs[rp->id];
> > - ma.clear_mask = rp->mask;
> > - ma.set_mask = bits;
> > + ra.reg = rd->regs[rp->id];
> > + ra.mask = rp->mask;
> > + ra.value = bits;
> >
> > - ret = smp_call_function_single(cpu, msrl_update_func, &ma,
> > 1);
> > - if (ret)
> > - WARN_ON_ONCE(ret);
> > - else
> > - ret = ma.err;
> > + ret = rd->rp->priv->write_raw(cpu, &ra);
> >
> > return ret;
> > }
> > @@ -1506,6 +1471,44 @@ static struct notifier_block
> > rapl_pm_notifier = {
> > .notifier_call = rapl_pm_callback,
> > };
> >
> > +static int rapl_msr_read_raw(int cpu, struct reg_action *ra)
> > +{
> > + if (rdmsrl_safe_on_cpu(cpu, ra->reg, &ra->value)) {
> > + pr_debug("failed to read msr 0x%x on cpu %d\n", ra-
> > >reg, cpu);
> > + return -EIO;
> > + }
> > + ra->value &= ra->mask;
> > + return 0;
> > +}
> > +
> > +static void rapl_msr_update_func(void *info)
> > +{
> > + struct reg_action *ra = info;
> > + u64 val;
> > +
> > + ra->err = rdmsrl_safe(ra->reg, &val);
> > + if (ra->err)
> > + return;
> > +
> > + val &= ~ra->mask;
> > + val |= ra->value;
> > +
> > + ra->err = wrmsrl_safe(ra->reg, val);
> > +}
> > +
> > +
> > +static int rapl_msr_write_raw(int cpu, struct reg_action *ra)
> > +{
> > + int ret;
> > +
> > + ret = smp_call_function_single(cpu, rapl_msr_update_func,
> > ra, 1);
> > + if (ret)
> > + WARN_ON_ONCE(ret);
> > + else
> > + ret = ra->err;
> > + return ret;
> I would prefer the above to be written as:
>
> ret = smp_call_function_single(cpu, rapl_msr_update_func, ra, 1);
> if (WARN_ON_ONCE(ret))
> return ret;
>
> return ra->err;
okay, will update it in next version.
thanks,
rui
^ permalink raw reply
* Re: [PATCH v3 6/6] interconnect: Add OPP table support for interconnects
From: Vincent Guittot @ 2019-07-03 6:45 UTC (permalink / raw)
To: Saravana Kannan
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, Sweeney, Sean,
daidavid1, Rajendra Nayak, sibis, Bjorn Andersson, Evan Green,
kernel-team, open list:THERMAL,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
In-Reply-To: <20190703011020.151615-7-saravanak@google.com>
On Wed, 3 Jul 2019 at 03:10, Saravana Kannan <saravanak@google.com> wrote:
>
> Interconnect paths can have different performance points. Now that OPP
> framework supports bandwidth OPP tables, add OPP table support for
> interconnects.
>
> Devices can use the interconnect-opp-table DT property to specify OPP
> tables for interconnect paths. And the driver can obtain the OPP table for
> an interconnect path by calling icc_get_opp_table().
The opp table of a path must come from the aggregation of OPP tables
of the interconnect providers. So such kind of OPP table should be at
provider level but not at path level.
>
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> drivers/interconnect/core.c | 27 ++++++++++++++++++++++++++-
> include/linux/interconnect.h | 7 +++++++
> 2 files changed, 33 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
> index 871eb4bc4efc..881bac80bc1e 100644
> --- a/drivers/interconnect/core.c
> +++ b/drivers/interconnect/core.c
> @@ -47,6 +47,7 @@ struct icc_req {
> */
> struct icc_path {
> size_t num_nodes;
> + struct opp_table *opp_table;
> struct icc_req reqs[];
> };
>
> @@ -313,7 +314,7 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> {
> struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
> struct icc_node *src_node, *dst_node;
> - struct device_node *np = NULL;
> + struct device_node *np = NULL, *opp_node;
> struct of_phandle_args src_args, dst_args;
> int idx = 0;
> int ret;
> @@ -381,10 +382,34 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
> dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
> mutex_unlock(&icc_lock);
>
> + opp_node = of_parse_phandle(np, "interconnect-opp-table", idx);
> + if (opp_node) {
> + path->opp_table = dev_pm_opp_of_find_table_from_node(opp_node);
> + of_node_put(opp_node);
> + }
> +
> +
> return path;
> }
> EXPORT_SYMBOL_GPL(of_icc_get);
>
> +/**
> + * icc_get_opp_table() - Get the OPP table that corresponds to a path
> + * @path: reference to the path returned by icc_get()
> + *
> + * This function will return the OPP table that corresponds to a path handle.
> + * If the interconnect API is disabled, NULL is returned and the consumer
> + * drivers will still build. Drivers are free to handle this specifically, but
> + * they don't have to.
> + *
> + * Return: opp_table pointer on success. NULL is returned when the API is
> + * disabled or the OPP table is missing.
> + */
> +struct opp_table *icc_get_opp_table(struct icc_path *path)
> +{
> + return path->opp_table;
> +}
> +
> /**
> * icc_set_bw() - set bandwidth constraints on an interconnect path
> * @path: reference to the path returned by icc_get()
> diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
> index dc25864755ba..0c0bc55f0e89 100644
> --- a/include/linux/interconnect.h
> +++ b/include/linux/interconnect.h
> @@ -9,6 +9,7 @@
>
> #include <linux/mutex.h>
> #include <linux/types.h>
> +#include <linux/pm_opp.h>
>
> /* macros for converting to icc units */
> #define Bps_to_icc(x) ((x) / 1000)
> @@ -28,6 +29,7 @@ struct device;
> struct icc_path *icc_get(struct device *dev, const int src_id,
> const int dst_id);
> struct icc_path *of_icc_get(struct device *dev, const char *name);
> +struct opp_table *icc_get_opp_table(struct icc_path *path);
> void icc_put(struct icc_path *path);
> int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
>
> @@ -49,6 +51,11 @@ static inline void icc_put(struct icc_path *path)
> {
> }
>
> +static inline struct opp_table *icc_get_opp_table(struct icc_path *path)
> +{
> + return NULL;
> +}
> +
> static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
> {
> return 0;
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
^ permalink raw reply
* Re: [PATCH v3 0/6] Introduce Bandwidth OPPs for interconnect paths
From: Viresh Kumar @ 2019-07-03 6:36 UTC (permalink / raw)
To: Saravana Kannan
Cc: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki, vincent.guittot,
seansw, daidavid1, Rajendra Nayak, sibis, bjorn.andersson,
evgreen, kernel-team, linux-pm, devicetree, linux-kernel
In-Reply-To: <20190703011020.151615-1-saravanak@google.com>
On 02-07-19, 18:10, Saravana Kannan wrote:
> Interconnects and interconnect paths quantify their performance levels in
> terms of bandwidth and not in terms of frequency. So similar to how we have
> frequency based OPP tables in DT and in the OPP framework, we need
> bandwidth OPP table support in the OPP framework and in DT. Since there can
> be more than one interconnect path used by a device, we also need a way to
> assign a bandwidth OPP table to an interconnect path.
>
> This patch series:
> - Adds opp-peak-KBps and opp-avg-KBps properties to OPP DT bindings
> - Adds interconnect-opp-table property to interconnect DT bindings
> - Adds OPP helper functions for bandwidth OPP tables
> - Adds icc_get_opp_table() to get the OPP table for an interconnect path
>
> So with the DT bindings added in this patch series, the DT for a GPU
> that does bandwidth voting from GPU to Cache and GPU to DDR would look
> something like this:
And what changed since V2 ?
--
viresh
^ permalink raw reply
* Re: [RFCv2 6/8] devfreq: Add imx-devfreq driver
From: Chanwoo Choi @ 2019-07-03 1:31 UTC (permalink / raw)
To: Leonard Crestez, Alexandre Bailon, Georgi Djakov, Stephen Boyd,
Michael Turquette, Viresh Kumar
Cc: MyungJoo Ham, Kyungmin Park, Shawn Guo, Dong Aisheng,
Fabio Estevam, Rafael J. Wysocki, Jacky Bai, Anson Huang,
Abel Vesa, Krzysztof Kozlowski, Ulf Hansson, Saravana Kannan,
kernel, linux-imx, linux-pm, linux-clk, linux-arm-kernel,
cpgs (cpgs@samsung.com)
In-Reply-To: <5531b22cdcb056c66fc0a51b4c839df5d86049d0.1561707104.git.leonard.crestez@nxp.com>
Hi Leonard,
On 19. 6. 28. 오후 4:39, Leonard Crestez wrote:
> Add initial support for frequency switches on pieces of the imx
> interconnect fabric.
>
> Uses clk_set_min_rate so that other subsytems can also impose minimum
> rate requests.
>
> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> ---
> drivers/devfreq/Kconfig | 10 +++
> drivers/devfreq/Makefile | 1 +
> drivers/devfreq/imx-devfreq.c | 142 ++++++++++++++++++++++++++++++++++
> 3 files changed, 153 insertions(+)
> create mode 100644 drivers/devfreq/imx-devfreq.c
>
> diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
> index f3b242987fd9..3a7c6265ca2f 100644
> --- a/drivers/devfreq/Kconfig
> +++ b/drivers/devfreq/Kconfig
> @@ -90,10 +90,20 @@ config ARM_EXYNOS_BUS_DEVFREQ
> Each memory bus group could contain many memoby bus block. It reads
> PPMU counters of memory controllers by using DEVFREQ-event device
> and adjusts the operating frequencies and voltages with OPP support.
> This does not yet operate with optimal voltages.
>
> +config ARM_IMX_DEVFREQ
> + tristate "i.MX DEVFREQ Driver"
> + depends on ARCH_MXC
You don't need to add 'COMPILE_TEST' as following?
depends on ARCH_MXC || COMPILE_TEST
> + select DEVFREQ_GOV_USERSPACE
> + select PM_OPP
> + help
> + This adds the DEVFREQ driver for the Tegra family of SoCs.
> + It reads ACTMON counters of memory controllers and adjusts the
> + operating frequencies and voltages with OPP support.
Maybe, you referred to the description of ARM_TEGRA_DEVFREQ.
You have to modify it according to the role of IMX_DEVFREQ.
> +
> config ARM_TEGRA_DEVFREQ
> tristate "NVIDIA Tegra30/114/124/210 DEVFREQ Driver"
> depends on ARCH_TEGRA_3x_SOC || ARCH_TEGRA_114_SOC || \
> ARCH_TEGRA_132_SOC || ARCH_TEGRA_124_SOC || \
> ARCH_TEGRA_210_SOC || \
> diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
> index 338ae8440db6..c2463ed4c934 100644
> --- a/drivers/devfreq/Makefile
> +++ b/drivers/devfreq/Makefile
> @@ -7,10 +7,11 @@ obj-$(CONFIG_DEVFREQ_GOV_POWERSAVE) += governor_powersave.o
> obj-$(CONFIG_DEVFREQ_GOV_USERSPACE) += governor_userspace.o
> obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
>
> # DEVFREQ Drivers
> obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
> +obj-$(CONFIG_ARM_IMX_DEVFREQ) += imx-devfreq.o
> obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
> obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra30-devfreq.o
> obj-$(CONFIG_ARM_TEGRA20_DEVFREQ) += tegra20-devfreq.o
>
> # DEVFREQ Event Drivers
> diff --git a/drivers/devfreq/imx-devfreq.c b/drivers/devfreq/imx-devfreq.c
> new file mode 100644
> index 000000000000..aa8868d32f22
> --- /dev/null
> +++ b/drivers/devfreq/imx-devfreq.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2019 NXP
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/devfreq.h>
> +#include <linux/devfreq-event.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +struct imx_devfreq {
> + struct devfreq_dev_profile profile;
> + struct devfreq *devfreq;
> + struct clk *clk;
> +};
> +
> +static int imx_devfreq_target(struct device *dev, unsigned long *freq, u32 flags)
> +{
> + struct imx_devfreq *priv = dev_get_drvdata(dev);
> + struct dev_pm_opp *new_opp;
> + unsigned long new_freq;
> + int ret;
> +
> + new_opp = devfreq_recommended_opp(dev, freq, flags);
> + if (IS_ERR(new_opp)) {
> + ret = PTR_ERR(new_opp);
> + dev_err(dev, "failed to get recommended opp: %d\n", ret);
> + return ret;
> + }
> + new_freq = dev_pm_opp_get_freq(new_opp);
> + dev_pm_opp_put(new_opp);
> +
> + ret = clk_set_min_rate(priv->clk, new_freq);
> + if (ret)
Fix the indentation.
> + return ret;
> +
> + ret = clk_set_rate(priv->clk, 0);
> + if (ret) {
> + clk_set_min_rate(priv->clk, priv->devfreq->previous_freq);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int imx_devfreq_get_cur_freq(struct device *dev, unsigned long *freq)
> +{
> + struct imx_devfreq *priv = dev_get_drvdata(dev);
> +
> + *freq = clk_get_rate(priv->clk);
> +
> + return 0;
> +}
> +
> +static int imx_devfreq_get_dev_status(struct device *dev,
> + struct devfreq_dev_status *stat)
> +{
> + struct imx_devfreq *priv = dev_get_drvdata(dev);
> +
> + stat->busy_time = 0;
> + stat->total_time = 0;
> + stat->current_frequency = clk_get_rate(priv->clk);
> +
> + return 0;
> +}
> +
> +static void imx_devfreq_exit(struct device *dev)
> +{
> + return dev_pm_opp_of_remove_table(dev);
> +}
> +
> +static int imx_devfreq_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct imx_devfreq *priv;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(priv->devfreq)) {
> + ret = PTR_ERR(priv->devfreq);
> + dev_err(dev, "failed to fetch clk: %d\n", ret);
> + return ret;
> + }
> + platform_set_drvdata(pdev, priv);
> +
> + ret = dev_pm_opp_of_add_table(dev);
> + if (ret < 0) {
> + dev_err(dev, "failed to get OPP table\n");
> + return ret;
> + }
> +
> + priv->profile.polling_ms = 1000;
> + priv->profile.target = imx_devfreq_target;
> + priv->profile.get_dev_status = imx_devfreq_get_dev_status;
> + priv->profile.exit = imx_devfreq_exit;
> + priv->profile.get_cur_freq = imx_devfreq_get_cur_freq;
> + priv->profile.initial_freq = clk_get_rate(priv->clk);
> +
> + priv->devfreq = devm_devfreq_add_device(dev, &priv->profile,
> + DEVFREQ_GOV_USERSPACE,
> + NULL);
> + if (IS_ERR(priv->devfreq)) {
> + ret = PTR_ERR(priv->devfreq);
> + dev_err(dev, "failed to add devfreq device: %d\n", ret);
> + goto err_remove_table;
> + }
> +
> + return 0;
> +
> +err_remove_table:
> + dev_pm_opp_of_remove_table(dev);
> + return ret;
> +}
> +
> +static const struct of_device_id imx_devfreq_of_match[] = {
> + { .compatible = "fsl,imx8mm-ddrc", },
> + { .compatible = "fsl,imx8mm-noc", },
> + { .compatible = "fsl,imx8mm-nic", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, imx_devfreq_of_match);
> +
> +static struct platform_driver imx_devfreq_platdrv = {
> + .probe = imx_devfreq_probe,
> + .driver = {
> + .name = "devfreq-imx",
> + .of_match_table = of_match_ptr(imx_devfreq_of_match),
> + },
> +};
> +module_platform_driver(imx_devfreq_platdrv);
> +
> +MODULE_DESCRIPTION("Generic i.MX bus frequency driver");
> +MODULE_LICENSE("GPL v2");
>
Why don't you add the author information?
And when you are sending the patch, you better
to use get_maintainer.pl script to contain
the related all maintainers/reviewers.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* [PATCH v3 1/6] dt-bindings: opp: Introduce opp-peak-KBps and opp-avg-KBps bindings
From: Saravana Kannan @ 2019-07-03 1:10 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: Saravana Kannan, vincent.guittot, seansw, daidavid1,
Rajendra Nayak, sibis, bjorn.andersson, evgreen, kernel-team,
linux-pm, devicetree, linux-kernel
In-Reply-To: <20190703011020.151615-1-saravanak@google.com>
Interconnects often quantify their performance points in terms of
bandwidth. So, add opp-peak-KBps (required) and opp-avg-KBps (optional) to
allow specifying Bandwidth OPP tables in DT.
opp-peak-KBps is a required property that replace opp-hz for Bandwidth OPP
tables.
opp-avg-KBps is an optional property that can be used in Bandwidth OPP
tables.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
Documentation/devicetree/bindings/opp/opp.txt | 15 ++++++++++++---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/opp/opp.txt b/Documentation/devicetree/bindings/opp/opp.txt
index 76b6c79604a5..c869e87caa2a 100644
--- a/Documentation/devicetree/bindings/opp/opp.txt
+++ b/Documentation/devicetree/bindings/opp/opp.txt
@@ -83,9 +83,14 @@ properties.
Required properties:
- opp-hz: Frequency in Hz, expressed as a 64-bit big-endian integer. This is a
- required property for all device nodes but devices like power domains. The
- power domain nodes must have another (implementation dependent) property which
- uniquely identifies the OPP nodes.
+ required property for all device nodes but for devices like power domains or
+ bandwidth opp tables. The power domain nodes must have another (implementation
+ dependent) property which uniquely identifies the OPP nodes. The interconnect
+ opps are required to have the opp-peak-bw property.
+
+- opp-peak-KBps: Peak bandwidth in kilobytes per second, expressed as a 32-bit
+ big-endian integer. This is a required property for all devices that don't
+ have opp-hz. For example, bandwidth OPP tables for interconnect paths.
Optional properties:
- opp-microvolt: voltage in micro Volts.
@@ -132,6 +137,10 @@ Optional properties:
- opp-level: A value representing the performance level of the device,
expressed as a 32-bit integer.
+- opp-avg-KBps: Average bandwidth in kilobytes per second, expressed as a
+ 32-bit big-endian integer. This property is only meaningful in OPP tables
+ where opp-peak-KBps is present.
+
- clock-latency-ns: Specifies the maximum possible transition latency (in
nanoseconds) for switching to this OPP from any other OPP.
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v3 2/6] OPP: Add support for bandwidth OPP tables
From: Saravana Kannan @ 2019-07-03 1:10 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: Saravana Kannan, vincent.guittot, seansw, daidavid1,
Rajendra Nayak, sibis, bjorn.andersson, evgreen, kernel-team,
linux-pm, devicetree, linux-kernel
In-Reply-To: <20190703011020.151615-1-saravanak@google.com>
Not all devices quantify their performance points in terms of frequency.
Devices like interconnects quantify their performance points in terms of
bandwidth. We need a way to represent these bandwidth levels in OPP. So,
add support for parsing bandwidth OPPs from DT.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/opp/of.c | 34 ++++++++++++++++++++++++++++++++--
drivers/opp/opp.h | 4 +++-
2 files changed, 35 insertions(+), 3 deletions(-)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index c10c782d15aa..54fa70ed2adc 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -552,6 +552,35 @@ void dev_pm_opp_of_remove_table(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
+static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np)
+{
+ int ret;
+ u64 rate;
+ u32 bw;
+
+ ret = of_property_read_u64(np, "opp-hz", &rate);
+ if (!ret) {
+ /*
+ * Rate is defined as an unsigned long in clk API, and so
+ * casting explicitly to its type. Must be fixed once rate is 64
+ * bit guaranteed in clk API.
+ */
+ new_opp->rate = (unsigned long)rate;
+ return 0;
+ }
+
+ ret = of_property_read_u32(np, "opp-peak-KBps", &bw);
+ if (ret)
+ return ret;
+ new_opp->rate = (unsigned long) &bw;
+
+ ret = of_property_read_u32(np, "opp-avg-KBps", &bw);
+ if (!ret)
+ new_opp->avg_bw = (unsigned long) &bw;
+
+ return 0;
+}
+
/**
* _opp_add_static_v2() - Allocate static OPPs (As per 'v2' DT bindings)
* @opp_table: OPP table
@@ -589,11 +618,12 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
if (!new_opp)
return ERR_PTR(-ENOMEM);
- ret = of_property_read_u64(np, "opp-hz", &rate);
+ ret = _read_opp_key(new_opp, np);
if (ret < 0) {
/* "opp-hz" is optional for devices like power domains. */
if (!opp_table->is_genpd) {
- dev_err(dev, "%s: opp-hz not found\n", __func__);
+ dev_err(dev, "%s: opp-hz or opp-peak-bw not found\n",
+ __func__);
goto free_opp;
}
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 569b3525aa67..ead2cdafe957 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -59,7 +59,8 @@ extern struct list_head opp_tables;
* @turbo: true if turbo (boost) OPP
* @suspend: true if suspend OPP
* @pstate: Device's power domain's performance state.
- * @rate: Frequency in hertz
+ * @rate: Frequency in hertz OR Peak bandwidth in kilobytes per second
+ * @avg_bw: Average bandwidth in kilobytes per second
* @level: Performance level
* @supplies: Power supplies voltage/current values
* @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
@@ -81,6 +82,7 @@ struct dev_pm_opp {
bool suspend;
unsigned int pstate;
unsigned long rate;
+ unsigned long avg_bw;
unsigned int level;
struct dev_pm_opp_supply *supplies;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v3 4/6] OPP: Add API to find an OPP table from its DT node
From: Saravana Kannan @ 2019-07-03 1:10 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: Saravana Kannan, vincent.guittot, seansw, daidavid1,
Rajendra Nayak, sibis, bjorn.andersson, evgreen, kernel-team,
linux-pm, devicetree, linux-kernel
In-Reply-To: <20190703011020.151615-1-saravanak@google.com>
This allows finding a device's OPP table (when it has multiple) from a
phandle to the OPP table in DT.
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/opp/of.c | 42 ++++++++++++++++++++++++++++++++++--------
include/linux/pm_opp.h | 7 +++++++
2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/drivers/opp/of.c b/drivers/opp/of.c
index 54fa70ed2adc..34c51905f56d 100644
--- a/drivers/opp/of.c
+++ b/drivers/opp/of.c
@@ -42,14 +42,9 @@ struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev)
}
EXPORT_SYMBOL_GPL(dev_pm_opp_of_get_opp_desc_node);
-struct opp_table *_managed_opp(struct device *dev, int index)
+struct opp_table *_find_opp_table_from_node(struct device_node *np)
{
struct opp_table *opp_table, *managed_table = NULL;
- struct device_node *np;
-
- np = _opp_of_get_opp_desc_node(dev->of_node, index);
- if (!np)
- return NULL;
list_for_each_entry(opp_table, &opp_tables, node) {
if (opp_table->np == np) {
@@ -69,11 +64,42 @@ struct opp_table *_managed_opp(struct device *dev, int index)
}
}
- of_node_put(np);
-
return managed_table;
}
+/**
+ * dev_pm_opp_of_find_table_from_node() - Find OPP table from its DT node
+ * @np: DT node used for finding the OPP table
+ *
+ * Return: OPP table corresponding to the DT node, else NULL on failure.
+ *
+ * The caller needs to put the node with of_node_put() after using it.
+ */
+struct opp_table *dev_pm_opp_of_find_table_from_node(struct device_node *np)
+{
+ struct opp_table *opp_table;
+
+ mutex_lock(&opp_table_lock);
+ opp_table = _find_opp_table_from_node(np);
+ mutex_unlock(&opp_table_lock);
+ return opp_table;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_of_find_table_from_node);
+
+struct opp_table *_managed_opp(struct device *dev, int index)
+{
+ struct device_node *np;
+ struct opp_table *opp_table;
+
+ np = _opp_of_get_opp_desc_node(dev->of_node, index);
+ if (!np)
+ return NULL;
+
+ opp_table = _find_opp_table_from_node(np);
+ of_node_put(np);
+ return opp_table;
+}
+
/* The caller must call dev_pm_opp_put() after the OPP is used */
static struct dev_pm_opp *_find_opp_of_np(struct opp_table *opp_table,
struct device_node *opp_np)
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index d4d79ac0b5b2..d80c59bcd50b 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -359,6 +359,7 @@ int dev_pm_opp_of_cpumask_add_table(const struct cpumask *cpumask);
void dev_pm_opp_of_cpumask_remove_table(const struct cpumask *cpumask);
int dev_pm_opp_of_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device *dev);
+struct opp_table *dev_pm_opp_of_find_table_from_node(struct device_node *np);
struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp);
int of_get_required_opp_performance_state(struct device_node *np, int index);
void dev_pm_opp_of_register_em(struct cpumask *cpus);
@@ -396,6 +397,12 @@ static inline struct device_node *dev_pm_opp_of_get_opp_desc_node(struct device
return NULL;
}
+static inline struct opp_table *dev_pm_opp_of_find_table_from_node(
+ struct device_node *np)
+{
+ return NULL;
+}
+
static inline struct device_node *dev_pm_opp_get_of_node(struct dev_pm_opp *opp)
{
return NULL;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH v3 6/6] interconnect: Add OPP table support for interconnects
From: Saravana Kannan @ 2019-07-03 1:10 UTC (permalink / raw)
To: Georgi Djakov, Rob Herring, Mark Rutland, Viresh Kumar,
Nishanth Menon, Stephen Boyd, Rafael J. Wysocki
Cc: Saravana Kannan, vincent.guittot, seansw, daidavid1,
Rajendra Nayak, sibis, bjorn.andersson, evgreen, kernel-team,
linux-pm, devicetree, linux-kernel
In-Reply-To: <20190703011020.151615-1-saravanak@google.com>
Interconnect paths can have different performance points. Now that OPP
framework supports bandwidth OPP tables, add OPP table support for
interconnects.
Devices can use the interconnect-opp-table DT property to specify OPP
tables for interconnect paths. And the driver can obtain the OPP table for
an interconnect path by calling icc_get_opp_table().
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
drivers/interconnect/core.c | 27 ++++++++++++++++++++++++++-
include/linux/interconnect.h | 7 +++++++
2 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/drivers/interconnect/core.c b/drivers/interconnect/core.c
index 871eb4bc4efc..881bac80bc1e 100644
--- a/drivers/interconnect/core.c
+++ b/drivers/interconnect/core.c
@@ -47,6 +47,7 @@ struct icc_req {
*/
struct icc_path {
size_t num_nodes;
+ struct opp_table *opp_table;
struct icc_req reqs[];
};
@@ -313,7 +314,7 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
{
struct icc_path *path = ERR_PTR(-EPROBE_DEFER);
struct icc_node *src_node, *dst_node;
- struct device_node *np = NULL;
+ struct device_node *np = NULL, *opp_node;
struct of_phandle_args src_args, dst_args;
int idx = 0;
int ret;
@@ -381,10 +382,34 @@ struct icc_path *of_icc_get(struct device *dev, const char *name)
dev_err(dev, "%s: invalid path=%ld\n", __func__, PTR_ERR(path));
mutex_unlock(&icc_lock);
+ opp_node = of_parse_phandle(np, "interconnect-opp-table", idx);
+ if (opp_node) {
+ path->opp_table = dev_pm_opp_of_find_table_from_node(opp_node);
+ of_node_put(opp_node);
+ }
+
+
return path;
}
EXPORT_SYMBOL_GPL(of_icc_get);
+/**
+ * icc_get_opp_table() - Get the OPP table that corresponds to a path
+ * @path: reference to the path returned by icc_get()
+ *
+ * This function will return the OPP table that corresponds to a path handle.
+ * If the interconnect API is disabled, NULL is returned and the consumer
+ * drivers will still build. Drivers are free to handle this specifically, but
+ * they don't have to.
+ *
+ * Return: opp_table pointer on success. NULL is returned when the API is
+ * disabled or the OPP table is missing.
+ */
+struct opp_table *icc_get_opp_table(struct icc_path *path)
+{
+ return path->opp_table;
+}
+
/**
* icc_set_bw() - set bandwidth constraints on an interconnect path
* @path: reference to the path returned by icc_get()
diff --git a/include/linux/interconnect.h b/include/linux/interconnect.h
index dc25864755ba..0c0bc55f0e89 100644
--- a/include/linux/interconnect.h
+++ b/include/linux/interconnect.h
@@ -9,6 +9,7 @@
#include <linux/mutex.h>
#include <linux/types.h>
+#include <linux/pm_opp.h>
/* macros for converting to icc units */
#define Bps_to_icc(x) ((x) / 1000)
@@ -28,6 +29,7 @@ struct device;
struct icc_path *icc_get(struct device *dev, const int src_id,
const int dst_id);
struct icc_path *of_icc_get(struct device *dev, const char *name);
+struct opp_table *icc_get_opp_table(struct icc_path *path);
void icc_put(struct icc_path *path);
int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw);
@@ -49,6 +51,11 @@ static inline void icc_put(struct icc_path *path)
{
}
+static inline struct opp_table *icc_get_opp_table(struct icc_path *path)
+{
+ return NULL;
+}
+
static inline int icc_set_bw(struct icc_path *path, u32 avg_bw, u32 peak_bw)
{
return 0;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox