* Re: [PATCH v2 1/9] PM / Domains: Drop extern declarations of functions in pm_domain.h
From: Viresh Kumar @ 2018-05-31 6:12 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J . Wysocki, linux-pm, Greg Kroah-Hartman, Jon Hunter,
Geert Uytterhoeven, Todor Tomov, Rajendra Nayak, Vincent Guittot,
Kevin Hilman, linux-kernel, linux-arm-kernel, linux-tegra
In-Reply-To: <20180529100421.31022-2-ulf.hansson@linaro.org>
On 29-05-18, 12:04, Ulf Hansson wrote:
> Using "extern" to declare a function in a public header file is somewhat
> pointless, but also doesn't hurt. However, to make all the function
> declarations in pm_domain.h to be consistent, let's drop the use of
> "extern".
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> include/linux/pm_domain.h | 51 ++++++++++++++++++---------------------
> 1 file changed, 23 insertions(+), 28 deletions(-)
Finally.
I wanted to do that earlier as checkpatch complained non stop about
such declarations :)
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply
* [RFC] PM / devfreq: Add support for alerts
From: Akhil P Oommen @ 2018-05-31 5:36 UTC (permalink / raw)
To: linux-pm, linux-kernel
Cc: jcrouse, myungjoo.ham, kyungmin.park, cw00.choi, linux-arm-msm
Currently, DEVFREQ reevaluates the device state periodically and/or
based on the OPP list changes. Private API has to be exposed to allow
the device driver to alert/notify the governor to reevaluate when a new
set of data is available. This makes the governor more coupled to a
particular device driver. We can improve here by exposing a DEVFREQ API
to allow the device drivers to send generic alerts to the governor.
Signed-off-by: Akhil P Oommen <akhilpo@codeaurora.org>
---
drivers/devfreq/devfreq.c | 21 +++++++++++++++++++++
drivers/devfreq/governor.h | 1 +
include/linux/devfreq.h | 5 +++++
3 files changed, 27 insertions(+)
diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index fe2af6a..24a8046 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -1532,3 +1532,24 @@ void devm_devfreq_unregister_notifier(struct device *dev,
devm_devfreq_dev_match, devfreq));
}
EXPORT_SYMBOL(devm_devfreq_unregister_notifier);
+
+/**
+ * devfreq_alert_governor() - Alert governor about an event
+ * @devfreq: The devfreq object.
+ * @type: Optional alert type.
+ *
+ * This function lets the device alert the governor about an event.
+ * A governor may not implement or choose to completely ignore this alert.
+ */
+void devfreq_alert_governor(struct devfreq *devfreq, unsigned int type)
+{
+ /* Don't let someone change the governor until we are done here. */
+ mutex_lock(&devfreq_list_lock);
+
+ if (devfreq)
+ devfreq->governor->event_handler(devfreq,
+ DEVFREQ_GOV_ALERT, &type);
+
+ mutex_unlock(&devfreq_list_lock);
+}
+EXPORT_SYMBOL(devfreq_alert_governor);
diff --git a/drivers/devfreq/governor.h b/drivers/devfreq/governor.h
index cfc50a6..e5da3442 100644
--- a/drivers/devfreq/governor.h
+++ b/drivers/devfreq/governor.h
@@ -24,6 +24,7 @@
#define DEVFREQ_GOV_INTERVAL 0x3
#define DEVFREQ_GOV_SUSPEND 0x4
#define DEVFREQ_GOV_RESUME 0x5
+#define DEVFREQ_GOV_ALERT 0x6
/**
* struct devfreq_governor - Devfreq policy governor
diff --git a/include/linux/devfreq.h b/include/linux/devfreq.h
index 3aae5b3..740c228 100644
--- a/include/linux/devfreq.h
+++ b/include/linux/devfreq.h
@@ -193,6 +193,7 @@ extern struct devfreq *devm_devfreq_add_device(struct device *dev,
void *data);
extern void devm_devfreq_remove_device(struct device *dev,
struct devfreq *devfreq);
+extern void devfreq_alert_governor(struct devfreq *devfreq, unsigned int type);
/* Supposed to be called by PM callbacks */
extern int devfreq_suspend_device(struct devfreq *devfreq);
@@ -306,6 +307,10 @@ static inline void devm_devfreq_remove_device(struct device *dev,
{
}
+static void devfreq_alert_governor(struct devfreq *devfreq, unsigned int type)
+{
+}
+
static inline int devfreq_suspend_device(struct devfreq *devfreq)
{
return 0;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH v9 01/15] ARM: Add Krait L2 register accessor functions
From: Sricharan R @ 2018-05-31 4:57 UTC (permalink / raw)
To: Stephen Boyd, Bjorn Andersson
Cc: robh, viresh.kumar, mark.rutland, mturquette, sboyd, linux,
andy.gross, david.brown, rjw, linux-arm-kernel, devicetree,
linux-kernel, linux-clk, linux-arm-msm, linux-soc, linux-pm,
linux
In-Reply-To: <152769571081.144038.4314499217001219157@swboyd.mtv.corp.google.com>
Hi Stephen,
On 5/30/2018 9:25 PM, Stephen Boyd wrote:
> Quoting Sricharan R (2018-05-24 22:40:11)
>> Hi Bjorn,
>>
>> On 5/24/2018 11:09 PM, Bjorn Andersson wrote:
>>> On Tue 06 Mar 06:38 PST 2018, Sricharan R wrote:
>>>
>>>> From: Stephen Boyd <sboyd@codeaurora.org>
>>>>
>>>> Krait CPUs have a handful of L2 cache controller registers that
>>>> live behind a cp15 based indirection register. First you program
>>>> the indirection register (l2cpselr) to point the L2 'window'
>>>> register (l2cpdr) at what you want to read/write. Then you
>>>> read/write the 'window' register to do what you want. The
>>>> l2cpselr register is not banked per-cpu so we must lock around
>>>> accesses to it to prevent other CPUs from re-pointing l2cpdr
>>>> underneath us.
>>>>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: Russell King <linux@arm.linux.org.uk>
>>>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>>>
>>> This should have your signed-off-by here as well.
>>>
>>
>> ok.
>>
>>> Apart from that:
>>>
>>> Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>
>>
>
> Will these patches come around again? I'll do a quick sweep on them
> today but I expect them to be resent.
Sure, i will have to resend them again, fixing couple of Bjorn's
minor comments. Will address your comments that you would give
as well along with that.
Regards,
Sricharan
--
"QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus
^ permalink raw reply
* Re: [PATCH v2 5/9] PM / Domains: dt: Allow power-domain property to be a list of specifiers
From: Rob Herring @ 2018-05-31 3:53 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J . Wysocki, linux-pm, Greg Kroah-Hartman, Jon Hunter,
Geert Uytterhoeven, Todor Tomov, Rajendra Nayak, Viresh Kumar,
Vincent Guittot, Kevin Hilman, linux-kernel, linux-arm-kernel,
linux-tegra, devicetree
In-Reply-To: <20180529100421.31022-6-ulf.hansson@linaro.org>
On Tue, May 29, 2018 at 12:04:17PM +0200, Ulf Hansson wrote:
> To be able to describe topologies where devices are partitioned across
> multiple power domains, let's extend the power-domain property to allow
> being a list of PM domain specifiers.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Suggested-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v2:
> - Fixed comments from Geert. Re-worded to "PM domain specifiers" and
> clarified DT example.
>
> ---
> .../bindings/power/power_domain.txt | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH] cpupower : Fix header name to read idle state name
From: Shuah Khan @ 2018-05-31 3:39 UTC (permalink / raw)
To: Abhishek Goel, trenn, linux-pm, linux-kernel; +Cc: s1seetee, Shuah Khan
In-Reply-To: <20180528110303.12860-1-huntbag@linux.vnet.ibm.com>
On 05/28/2018 05:03 AM, Abhishek Goel wrote:
> The names of the idle states in the output of cpupower monitor command are
> truncated to 4 characters. On POWER9, this creates ambiguity as the states
> are named "stop0", "stop1", etc.
>
> root:~# cpupower monitor
> |Idle_Stats
> PKG |CORE|CPU | snoo | stop | stop | stop | stop | stop | stop
> 0| 0| 0| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 1.90
> 0| 0| 1| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
> 0| 0| 2| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
> 0| 0| 3| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
>
> This patch modifies the output to print the state name that results in a
> legible output. The names will be printed with atmost 1 padding in left.
>
> root:~# cpupower monitor
> | Idle_Stats
> PKG|CORE| CPU|snooze|stop0L| stop0|stop1L| stop1|stop2L| stop2
> 0| 0| 0| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.72
> 0| 0| 1| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
> 0| 0| 2| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
> 0| 0| 3| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00| 0.00
>
> This patch does not affect the output for intel.
> Output for intel before applying the patch:
>
> root:~# cpupower monitor
> |Idle_Stats
> CPU | POLL | C1-S | C1E- | C3-S | C6-S | C7s- | C8-S | C9-S | C10-
> 0| 0.00| 0.14| 0.39| 0.35| 7.41| 0.00| 17.67| 1.01| 70.03
> 2| 0.00| 0.19| 0.47| 0.10| 6.50| 0.00| 29.66| 2.17| 58.07
> 1| 0.00| 0.11| 0.50| 1.50| 9.11| 0.18| 18.19| 0.40| 66.63
> 3| 0.00| 0.67| 0.42| 0.03| 5.84| 0.00| 12.58| 0.77| 77.14
>
> Output for intel after applying the patch:
>
> root:~# cpupower monitor
> | Idle_Stats
> CPU| POLL | C1-S | C1E- | C3-S | C6-S | C7s- | C8-S | C9-S | C10-
> 0| 0.03| 0.33| 1.01| 0.27| 3.03| 0.00| 19.18| 0.00| 71.24
> 2| 0.00| 1.58| 0.58| 0.42| 8.55| 0.09| 21.11| 0.99| 63.32
> 1| 0.00| 1.26| 0.88| 0.43| 9.00| 0.02| 7.78| 4.65| 71.91
> 3| 0.00| 0.30| 0.42| 0.06| 13.62| 0.21| 30.29| 0.00| 52.45
>
> Signed-off-by: Abhishek Goel <huntbag@linux.vnet.ibm.com>
> ---
>
Thanks - I have this queued for 4.18-rc1
-- Shuah
^ permalink raw reply
* Re: [PATCH 02/11] PM / devfreq: Fix handling of min/max_freq == 0
From: Matthias Kaehlcke @ 2018-05-30 21:13 UTC (permalink / raw)
To: Chanwoo Choi
Cc: MyungJoo Ham, Kyungmin Park, Arnd Bergmann, Greg Kroah-Hartman,
Rob Herring, Mark Rutland, linux-pm, devicetree, linux-kernel,
Brian Norris, Douglas Anderson, Ørjan Eide,
John Einar Reitan
In-Reply-To: <5B0E5AFE.8090703@samsung.com>
On Wed, May 30, 2018 at 05:04:14PM +0900, Chanwoo Choi wrote:
> Hi,
>
> On 2018년 05월 30일 03:57, Matthias Kaehlcke wrote:
> > On Mon, May 28, 2018 at 03:37:47PM +0900, Chanwoo Choi wrote:
> >> Hi,
> >>
> >> On 2018년 05월 26일 05:30, Matthias Kaehlcke wrote:
> >>> Commit ab8f58ad72c4 ("PM / devfreq: Set min/max_freq when adding the
> >>> devfreq device") initializes df->min/max_freq with the min/max OPP when
> >>> the device is added. Later commit f1d981eaecf8 ("PM / devfreq: Use the
> >>> available min/max frequency") adds df->scaling_min/max_freq and the
> >>> following to the frequency adjustment code:
> >>>
> >>> max_freq = MIN(devfreq->scaling_max_freq, devfreq->max_freq);
> >>>
> >>> With the current handling of min/max_freq this is incorrect:
> >>>
> >>> Even though df->max_freq is now initialized to a value != 0 user space
> >>> can still set it to 0, in this case max_freq would be 0 instead of
> >>> df->scaling_max_freq as intended. In consequence the frequency adjustment
> >>> is not performed:
> >>>
> >>> if (max_freq && freq > max_freq) {
> >>> freq = max_freq;
> >>>
> >>> To fix this set df->min/max freq to the min/max OPP in max/max_freq_store,
> >>> when the user passes a value of 0. This also prevents df->max_freq from
> >>> being set below the min OPP when df->min_freq is 0, and similar for
> >>> min_freq. Since it is now guaranteed that df->min/max_freq can't be 0 the
> >>> checks for this case can be removed.
> >>>
> >>> Fixes: f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency")
> >>> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> >>> ---
> >>> drivers/devfreq/devfreq.c | 30 ++++++++++++++++++------------
> >>> 1 file changed, 18 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
> >>> index 0057ef5b0a98..67da4e7b486b 100644
> >>> --- a/drivers/devfreq/devfreq.c
> >>> +++ b/drivers/devfreq/devfreq.c
> >>> @@ -283,11 +283,11 @@ int update_devfreq(struct devfreq *devfreq)
> >>> max_freq = MIN(devfreq->scaling_max_freq, devfreq->max_freq);
> >>> min_freq = MAX(devfreq->scaling_min_freq, devfreq->min_freq);
> >>>
> >>> - if (min_freq && freq < min_freq) {
> >>> + if (freq < min_freq) {
> >>> freq = min_freq;
> >>> flags &= ~DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use GLB */
> >>> }
> >>> - if (max_freq && freq > max_freq) {
> >>> + if (freq > max_freq) {
> >>> freq = max_freq;
> >>> flags |= DEVFREQ_FLAG_LEAST_UPPER_BOUND; /* Use LUB */
> >>> }
> >>> @@ -1123,17 +1123,20 @@ static ssize_t min_freq_store(struct device *dev, struct device_attribute *attr,
> >>> struct devfreq *df = to_devfreq(dev);
> >>> unsigned long value;
> >>> int ret;
> >>> - unsigned long max;
> >>>
> >>> ret = sscanf(buf, "%lu", &value);
> >>> if (ret != 1)
> >>> return -EINVAL;
> >>>
> >>> mutex_lock(&df->lock);
> >>> - max = df->max_freq;
> >>> - if (value && max && value > max) {
> >>> - ret = -EINVAL;
> >>> - goto unlock;
> >>> +
> >>> + if (value) {
> >>> + if (value > df->max_freq) {
> >>> + ret = -EINVAL;
> >>> + goto unlock;
> >>> + }
> >>> + } else {
> >>> + value = df->profile->freq_table[df->profile->max_state - 1];
> >>> }
> >>
> >> If you want to prevent that df->min_freq is zero,
> >> you should reinitialize 'value' as following.
> >> Because freq_table must be in ascending order.
> >> value = df->profile->freq_table[0];
> >
> > Thanks for pointing this out!
> >
> > The devfreq device I tested with (a Mali GPU) uses descending order
> > for some reason, and I assumed that's the usual order.
> >
> > https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/gpu/arm/midgard/backend/gpu/mali_kbase_devfreq.c#208
> >
> > It seems the ordering doesn't have any impact beyond this patch. If
> > the order isn't mandatory for drivers that set up their own freq_table
> > we should probably support both cases to be safe.
>
> Prior to that 'freq_table' is optional. So, patch[1] initialize the 'freq_table'
> by using OPP interface if 'freq_table' is NULL.
> [1] commit 0ec09ac2cebe ("PM / devfreq: Set the freq_table of devfreq device")
>
> Current devfreq recommend the ascending order for 'freq_table'.
> But, as you know, it might be not enough to support them.
>
> I agree that we should support the both cases (ascending or descending order).
>
> Maybe, it might be not proper to access the freq_table[] directly
> because we don't know the ordering style of 'freq_table'
> if 'freq_table' is made by devfreq user instead of devfreq core.
If we can assume that it is either ascending or descending, but not
random order than a simple check if freq_table[0] <
freq_table[max_state - 1] would be sufficient.
Otherwise we could also determine the min/max after initialization and
save the result, though that would leave us with yet another frequency
pair, which might be confusing, especially if we don't come up with
good names to distinguish between them.
^ permalink raw reply
* Re: [RFC/RFT] [PATCH 02/10] cpufreq: intel_pstate: Conditional frequency invariant accounting
From: Patrick Bellasi @ 2018-05-30 16:57 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Rafael J. Wysocki, Srinivas Pandruvada, Juri Lelli,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Len Brown,
Rafael J. Wysocki, Mel Gorman, the arch/x86 maintainers, Linux PM,
Viresh Kumar, Linux Kernel Mailing List
In-Reply-To: <20180518133353.GO30654@e110439-lin>
Hi Peter,
maybe you missed this previous my response:
20180518133353.GO30654@e110439-lin
?
Would like to have your tought about the concept of "transient maximum
capacity" I was describing...
On 18-May 14:33, Patrick Bellasi wrote:
> On 18-May 13:29, Peter Zijlstra wrote:
> > On Fri, May 18, 2018 at 11:57:42AM +0100, Patrick Bellasi wrote:
> > > Thus, my simple (maybe dumb) questions are:
> > > - why can't we just fold turbo boost frequency into the existing concepts?
> > > - what are the limitations of such a "simple" approach?
> >
> > Perhaps... but does this not further complicate the whole capacity vs
> > util thing we already have in say the misfit patches?
>
> Not sure about that...
>
> > And the util_fits_capacity() thing from the EAS ones.
>
> In this case instead, if we can track somehow (not saying we can)
> what is the currently available "transient maximum capacity"...
> then a util_fits_capacity() should just look at that.
>
> If the transient capacity is already folded into cpu_capacity, as it
> is now for RT and IRQ pressure, then likely we don't have to change
> anything.
>
> > The thing is, we either need to dynamically scale the util or the
> > capacity or both. I think for Thermal there are patches out there that
> > drop the capacity.
>
> Not sure... but I would feel more comfortable by something which caps
> the maximum capacity. Meaning, eventually you can fill up the maximum
> possible capacity only "up to" a given value, because of thermal or other
> reasons most of the scheduler maybe doesn't even have to know why?
>
> > But we'd then have to do the same for turbo/vector and all the other
> > stuff as well. Otherwise we risk things like running at low U with 0%
> > idle and not triggering the tipping point between eas and regular
> > balancing.
>
> Interacting with the tipping point and/or OPP changes is indeed an
> interesting side of the problem I was not considering so far...
>
> But again, the tipping point could not be defined as a delta
> with respect to the "transient maximum capacity" ?
>
> > So either way around we need to know the 'true' max, either to fudge
> > util or to fudge capacity.
>
> Right, but what I see from a concepts standpoint is something like:
>
> +--+--+ cpu_capacity_orig (CONSTANT at boot time)
> | | |
> | | | HW generated constraints
> | v |
> +-----+ cpu_capacity_max (depending on thermal/turbo boost)
> | | |
> | | | SW generated constraints
> | v |
> +-----+ cpu_capacity (depending on RT/IRQ pressure)
> | | |
> | | | tipping point delta
> +--v--+
> | | Energy Aware mode available capacity
> +-----+
>
> Where all the wkp/lb heuristics are updated to properly consider the
> cpu_capacity_max metrics whenever it comes to know what is the max
> speed we can reach now on a CPU.
>
> > And I'm not sure we can know in some of these cases :/
>
> Right, this schema will eventually work only under the hypothesis that
> "somehow" we can update cpu_capacity_max from HW events.
>
> Not entirely sure that's possible and/or at which time granularity on
> all different platforms.
>
> > And while Vincent's patches might have been inspired by another problem,
> > they do have the effect of always allowing util to go to 1, which is
> > nice for this.
>
> Sure, that's a nice point, but still I have the feeling that always
> reaching u=1 can defeat other interesting properties of a task,
> For example, comparing task requirements in different CPUs and/or at
> different times, which plays a big role for energy aware task
> placement decisions.
>
> --
> #include <best/regards.h>
>
> Patrick Bellasi
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v3 1/2] sched/cpufreq: always consider blocked FAIR utilization
From: Patrick Bellasi @ 2018-05-30 16:50 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux Kernel Mailing List, Linux PM, Ingo Molnar, Peter Zijlstra,
Rafael J . Wysocki, Viresh Kumar, Vincent Guittot,
Dietmar Eggemann, Morten Rasmussen, Juri Lelli, Joel Fernandes,
Steve Muckle
In-Reply-To: <CAJZ5v0iq-k4QeeEm6vzfWNLkXxNtLZa0Y9_bBcYWCGQ_FjzgDQ@mail.gmail.com>
On 27-May 11:50, Rafael J. Wysocki wrote:
> On Thu, May 24, 2018 at 4:10 PM, Patrick Bellasi
> <patrick.bellasi@arm.com> wrote:
> > Since the refactoring introduced by:
> >
> > commit 8f111bc357aa ("cpufreq/schedutil: Rewrite CPUFREQ_RT support")
> >
> > we aggregate FAIR utilization only if this class has runnable tasks.
> > This was mainly due to avoid the risk to stay on an high frequency just
> > because of the blocked utilization of a CPU not being properly decayed
> > while the CPU was idle.
> >
> > However, since:
> >
> > commit 31e77c93e432 ("sched/fair: Update blocked load when newly idle")
> >
> > the FAIR blocked utilization is properly decayed also for IDLE CPUs.
> >
> > This allows us to use the FAIR blocked utilization as a safe mechanism
> > to gracefully reduce the frequency only if no FAIR tasks show up on a
> > CPU for a reasonable period of time.
> >
> > Moreover, we also reduce the frequency drops of CPUs running periodic
> > tasks which, depending on the task periodicity and the time required
> > for a frequency switch, was increasing the chances to introduce some
> > undesirable performance variations.
> >
> > Reported-by: Vincent Guittot <vincent.guittot@linaro.org>
> > Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > Acked-by: Vincent Guittot <vincent.guittot@linaro.org>
> > Tested-by: Vincent Guittot <vincent.guittot@linaro.org>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Vincent Guittot <vincent.guittot@linaro.org>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Cc: Joel Fernandes <joelaf@google.com>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-pm@vger.kernel.org
>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Or please let me know if you want me to apply this one.
Hi Rafael, seems this patch has already been applied in tip/sched/core.
However is missing your tag above. :/
Dunno if I have / I can to do something about that.
> > ---
> > Changes in v3:
> > - add "Tested-by" and "Acked-by" Vincent tags
> >
> > Changes in v2:
> > - add "Acked-by" Viresh tag
> > ---
> > kernel/sched/cpufreq_schedutil.c | 17 ++++++++---------
> > 1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index e13df951aca7..28592b62b1d5 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -183,22 +183,21 @@ static void sugov_get_util(struct sugov_cpu *sg_cpu)
> > static unsigned long sugov_aggregate_util(struct sugov_cpu *sg_cpu)
> > {
> > struct rq *rq = cpu_rq(sg_cpu->cpu);
> > - unsigned long util;
> >
> > - if (rq->rt.rt_nr_running) {
> > - util = sg_cpu->max;
> > - } else {
> > - util = sg_cpu->util_dl;
> > - if (rq->cfs.h_nr_running)
> > - util += sg_cpu->util_cfs;
> > - }
> > + if (rq->rt.rt_nr_running)
> > + return sg_cpu->max;
> >
> > /*
> > + * Utilization required by DEADLINE must always be granted while, for
> > + * FAIR, we use blocked utilization of IDLE CPUs as a mechanism to
> > + * gracefully reduce the frequency when no tasks show up for longer
> > + * periods of time.
> > + *
> > * Ideally we would like to set util_dl as min/guaranteed freq and
> > * util_cfs + util_dl as requested freq. However, cpufreq is not yet
> > * ready for such an interface. So, we only do the latter for now.
> > */
> > - return min(util, sg_cpu->max);
> > + return min(sg_cpu->max, (sg_cpu->util_dl + sg_cpu->util_cfs));
> > }
> >
> > static void sugov_set_iowait_boost(struct sugov_cpu *sg_cpu, u64 time, unsigned int flags)
> > --
> > 2.15.1
> >
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v2 7/9] PM / Domains: Split genpd_dev_pm_attach()
From: Jon Hunter @ 2018-05-30 16:08 UTC (permalink / raw)
To: Ulf Hansson, Rafael J . Wysocki, linux-pm
Cc: Greg Kroah-Hartman, Geert Uytterhoeven, Todor Tomov,
Rajendra Nayak, Viresh Kumar, Vincent Guittot, Kevin Hilman,
linux-kernel, linux-arm-kernel, linux-tegra
In-Reply-To: <20180529100421.31022-8-ulf.hansson@linaro.org>
On 29/05/18 11:04, Ulf Hansson wrote:
> To extend genpd to deal with allowing multiple PM domains per device, some
> of the code in genpd_dev_pm_attach() can be re-used. Let's prepare for this
> by moving some of the code into a sub-function.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/base/power/domain.c | 60 ++++++++++++++++++++-----------------
> 1 file changed, 33 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 12a20f21974d..2af99bfcbe3c 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2221,38 +2221,15 @@ static void genpd_dev_pm_sync(struct device *dev)
> genpd_queue_power_off_work(pd);
> }
>
> -/**
> - * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
> - * @dev: Device to attach.
> - *
> - * Parse device's OF node to find a PM domain specifier. If such is found,
> - * attaches the device to retrieved pm_domain ops.
> - *
> - * Returns 1 on successfully attached PM domain, 0 when the device don't need a
> - * PM domain or when multiple power-domains exists for it, else a negative error
> - * code. Note that if a power-domain exists for the device, but it cannot be
> - * found or turned on, then return -EPROBE_DEFER to ensure that the device is
> - * not probed and to re-try again later.
> - */
> -int genpd_dev_pm_attach(struct device *dev)
> +static int __genpd_dev_pm_attach(struct device *dev, struct device_node *np,
> + unsigned int index)
> {
> struct of_phandle_args pd_args;
> struct generic_pm_domain *pd;
> int ret;
>
> - if (!dev->of_node)
> - return 0;
> -
> - /*
> - * Devices with multiple PM domains must be attached separately, as we
> - * can only attach one PM domain per device.
> - */
> - if (of_count_phandle_with_args(dev->of_node, "power-domains",
> - "#power-domain-cells") != 1)
> - return 0;
> -
> - ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> - "#power-domain-cells", 0, &pd_args);
> + ret = of_parse_phandle_with_args(np, "power-domains",
> + "#power-domain-cells", index, &pd_args);
> if (ret < 0)
> return ret;
>
> @@ -2290,6 +2267,35 @@ int genpd_dev_pm_attach(struct device *dev)
>
> return ret ? -EPROBE_DEFER : 1;
> }
> +
> +/**
> + * genpd_dev_pm_attach - Attach a device to its PM domain using DT.
> + * @dev: Device to attach.
> + *
> + * Parse device's OF node to find a PM domain specifier. If such is found,
> + * attaches the device to retrieved pm_domain ops.
> + *
> + * Returns 1 on successfully attached PM domain, 0 when the device don't need a
> + * PM domain or when multiple power-domains exists for it, else a negative error
> + * code. Note that if a power-domain exists for the device, but it cannot be
> + * found or turned on, then return -EPROBE_DEFER to ensure that the device is
> + * not probed and to re-try again later.
> + */
> +int genpd_dev_pm_attach(struct device *dev)
> +{
> + if (!dev->of_node)
> + return 0;
> +
> + /*
> + * Devices with multiple PM domains must be attached separately, as we
> + * can only attach one PM domain per device.
> + */
> + if (of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells") != 1)
> + return 0;
> +
> + return __genpd_dev_pm_attach(dev, dev->of_node, 0);
> +}
> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>
> static const struct of_device_id idle_state_match[] = {
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v2 6/9] PM / Domains: Don't attach devices in genpd with multi PM domains
From: Jon Hunter @ 2018-05-30 16:06 UTC (permalink / raw)
To: Ulf Hansson, Rafael J . Wysocki, linux-pm
Cc: Greg Kroah-Hartman, Geert Uytterhoeven, Todor Tomov,
Rajendra Nayak, Viresh Kumar, Vincent Guittot, Kevin Hilman,
linux-kernel, linux-arm-kernel, linux-tegra, Rob Herring,
devicetree
In-Reply-To: <20180529100421.31022-7-ulf.hansson@linaro.org>
On 29/05/18 11:04, Ulf Hansson wrote:
> The power-domain DT property may now contain a list of PM domain
> specifiers, which represents that a device are partitioned across multiple
> PM domains. This leads to a new situation in genpd_dev_pm_attach(), as only
> one PM domain can be attached per device.
>
> To remain things simple for the most common configuration, when a single PM
> domain is used, let's treat the multiple PM domain case as being specific.
>
> In other words, let's change genpd_dev_pm_attach() to check for multiple PM
> domains and prevent it from attach any PM domain for this case. Instead,
> leave this to be managed separately, from following changes to genpd.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> Suggested-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v2:
> - Minor update to changelog to mention "PM domain specifiers" rather
> than a "list of phandles".
>
> ---
> drivers/base/power/domain.c | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 7ebf7993273a..12a20f21974d 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2229,10 +2229,10 @@ static void genpd_dev_pm_sync(struct device *dev)
> * attaches the device to retrieved pm_domain ops.
> *
> * Returns 1 on successfully attached PM domain, 0 when the device don't need a
> - * PM domain or a negative error code in case of failures. Note that if a
> - * power-domain exists for the device, but it cannot be found or turned on,
> - * then return -EPROBE_DEFER to ensure that the device is not probed and to
> - * re-try again later.
> + * PM domain or when multiple power-domains exists for it, else a negative error
> + * code. Note that if a power-domain exists for the device, but it cannot be
> + * found or turned on, then return -EPROBE_DEFER to ensure that the device is
> + * not probed and to re-try again later.
> */
> int genpd_dev_pm_attach(struct device *dev)
> {
> @@ -2243,10 +2243,18 @@ int genpd_dev_pm_attach(struct device *dev)
> if (!dev->of_node)
> return 0;
>
> + /*
> + * Devices with multiple PM domains must be attached separately, as we
> + * can only attach one PM domain per device.
> + */
> + if (of_count_phandle_with_args(dev->of_node, "power-domains",
> + "#power-domain-cells") != 1)
> + return 0;
> +
> ret = of_parse_phandle_with_args(dev->of_node, "power-domains",
> "#power-domain-cells", 0, &pd_args);
> if (ret < 0)
> - return 0;
> + return ret;
>
> mutex_lock(&gpd_list_lock);
> pd = genpd_get_from_provider(&pd_args);
>
Acked-by: Jon Hunter <jonathanh@nvidia.com>
Tested-by: Jon Hunter <jonathanh@nvidia.com>
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v2 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Jon Hunter @ 2018-05-30 16:04 UTC (permalink / raw)
To: Ulf Hansson, Rafael J . Wysocki, linux-pm
Cc: Greg Kroah-Hartman, Geert Uytterhoeven, Todor Tomov,
Rajendra Nayak, Viresh Kumar, Vincent Guittot, Kevin Hilman,
linux-kernel, linux-arm-kernel, linux-tegra
In-Reply-To: <20180529100421.31022-9-ulf.hansson@linaro.org>
On 29/05/18 11:04, Ulf Hansson wrote:
> To support devices being partitioned across multiple PM domains, let's
> begin with extending genpd to cope with these kind of configurations.
>
> Therefore, add a new exported function genpd_dev_pm_attach_by_id(), which
> is similar to the existing genpd_dev_pm_attach(), but with the difference
> that it allows its callers to provide an index to the PM domain that it
> wants to attach.
>
> Note that, genpd_dev_pm_attach_by_id() shall only be called by the driver
> core / PM core, similar to how the existing dev_pm_domain_attach() makes
> use of genpd_dev_pm_attach(). However, this is implemented by following
> changes on top.
>
> Because, only one PM domain can be attached per device, genpd needs to
> create a virtual device that it can attach/detach instead. More precisely,
> let the new function genpd_dev_pm_attach_by_id() register a virtual struct
> device via calling device_register(). Then let it attach this device to the
> corresponding PM domain, rather than the one that is provided by the
> caller. The actual attaching is done via re-using the existing genpd OF
> functions.
>
> At successful attachment, genpd_dev_pm_attach_by_id() returns the created
> virtual device, which allows the caller to operate on it to deal with power
> management. Following changes on top, provides more details in this
> regards.
>
> To deal with detaching of a PM domain for the multiple PM domains case,
> let's also extend the existing genpd_dev_pm_detach() function, to cover the
> cleanup of the created virtual device, via make it call device_unregister()
> on it. In this way, there is no need to introduce a new function to deal
> with detach for the multiple PM domain case, but instead the existing one
> is re-used.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> Changes in v2:
> - Fixed comments from Jon. Clarified function descriptions
> and changelog.
>
> ---
> drivers/base/power/domain.c | 80 +++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 8 ++++
> 2 files changed, 88 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 2af99bfcbe3c..2b496d79159d 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2171,6 +2171,15 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
> }
> EXPORT_SYMBOL_GPL(of_genpd_remove_last);
>
> +static void genpd_release_dev(struct device *dev)
> +{
> + kfree(dev);
> +}
> +
> +static struct bus_type genpd_bus_type = {
> + .name = "genpd",
> +};
> +
> /**
> * genpd_dev_pm_detach - Detach a device from its PM domain.
> * @dev: Device to detach.
> @@ -2208,6 +2217,10 @@ static void genpd_dev_pm_detach(struct device *dev, bool power_off)
>
> /* Check if PM domain can be powered off after removing this device. */
> genpd_queue_power_off_work(pd);
> +
> + /* Unregister the device if it was created by genpd. */
> + if (dev->bus == &genpd_bus_type)
> + device_unregister(dev);
> }
>
> static void genpd_dev_pm_sync(struct device *dev)
> @@ -2298,6 +2311,67 @@ int genpd_dev_pm_attach(struct device *dev)
> }
> EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>
> +/**
> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> + * @dev: Device to attach.
Can you update the description of the above as well?
Thanks
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH v9 01/15] ARM: Add Krait L2 register accessor functions
From: Stephen Boyd @ 2018-05-30 15:55 UTC (permalink / raw)
To: Bjorn Andersson, Sricharan R
Cc: robh, viresh.kumar, mark.rutland, mturquette, sboyd, linux,
andy.gross, david.brown, rjw, linux-arm-kernel, devicetree,
linux-kernel, linux-clk, linux-arm-msm, linux-soc, linux-pm,
linux
In-Reply-To: <664089d4-b30d-99bb-2021-12128b2895ba@codeaurora.org>
Quoting Sricharan R (2018-05-24 22:40:11)
> Hi Bjorn,
>
> On 5/24/2018 11:09 PM, Bjorn Andersson wrote:
> > On Tue 06 Mar 06:38 PST 2018, Sricharan R wrote:
> >
> >> From: Stephen Boyd <sboyd@codeaurora.org>
> >>
> >> Krait CPUs have a handful of L2 cache controller registers that
> >> live behind a cp15 based indirection register. First you program
> >> the indirection register (l2cpselr) to point the L2 'window'
> >> register (l2cpdr) at what you want to read/write. Then you
> >> read/write the 'window' register to do what you want. The
> >> l2cpselr register is not banked per-cpu so we must lock around
> >> accesses to it to prevent other CPUs from re-pointing l2cpdr
> >> underneath us.
> >>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Russell King <linux@arm.linux.org.uk>
> >> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> >
> > This should have your signed-off-by here as well.
> >
>
> ok.
>
> > Apart from that:
> >
> > Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >
>
Will these patches come around again? I'll do a quick sweep on them
today but I expect them to be resent.
^ permalink raw reply
* Fwd: [Bug 199879] New: Very basic the Pci device is not resumed from suspend mode
From: Bjorn Helgaas @ 2018-05-30 14:41 UTC (permalink / raw)
To: linux-pci; +Cc: Linux Kernel Mailing List, Linux PM list
In-Reply-To: <bug-199879-41252@https.bugzilla.kernel.org/>
[+cc linux-pci, linux-kernel, linux-pm]
I'm not sure I understand the problem yet, so please correct me if I'm wrong:
- Your system has both Nvidia and Intel graphics devices
- When you use Intel graphics, lspci, lshw, and /proc/bus/pci for
the Nvidia device show invalid data (0xff) after suspend/resume
- When you use Nvidia graphics, suspend/resume doesn't work (instead
of resuming, you just get a blank screen)
Can you attach the output of "sudo lspci -vv" to the bugzilla, please?
---------- Forwarded message ---------
From: <bugzilla-daemon@bugzilla.kernel.org>
Date: Tue, May 29, 2018 at 1:29 PM
Subject: [Bug 199879] New: Very basic the Pci device is not resumed
from suspend mode
To: <bhelgaas@google.com>
https://bugzilla.kernel.org/show_bug.cgi?id=199879
Bug ID: 199879
Summary: Very basic the Pci device is not resumed from suspend
mode
Product: Drivers
Version: 2.5
Kernel Version: kernel-4.15.17
Hardware: x86-64
OS: Linux
Tree: Mainline
Status: NEW
Severity: high
Priority: P1
Component: PCI
Assignee: drivers_pci@kernel-bugs.osdl.org
Reporter: uzg@wp.pl
Regression: No
Hi, I have problem with very basic device. Device pci-e not resume from
suspend. Only sleep.
I have a problem with anyone interested in it, because everyone thinks it is
the fault of the device drivers themselves. But this is not a problem.
This device is a basic device. I've already installed drivers on various
hardware and it has always been ok, but not this time.
I'm an electronics technician. After diagnosing what I managed, in my opinion
the device remains asleep.
Where does my application come from?
I have multiuser mode and I do not use this device. After suspend lspci and
lshw show normal data. Normal data is in /proc/bus/pci/...
Next suspend and resume, and...
lspci see hardware, but is error
lshw see hardware as undefined device
data in /proc/bus/pci/... is only 0xFF
Hardware sleep, not work, not ready. This is bug.
But since the problem concerns the graphics card in the configuration with the
second default Intel card, everyone thinks that this is another driver problem
as always and nobody wants to take a look at it :(
The problem is easy to recognize. On the internet, I've seen a lot of
unresolved problems in which I could see exactly what I found.
My hardware is Lenovo with NVidia and Intel Graphics. Problem is with NVidia. I
tested Z710 and Z50-70. The first symptom of the problem is lspci in multiuser
mode (or when is XServer with intel graphics). After suspend NVidia have e.g
"rev. A1", after resume is "rev. FF". Next symptom lshw and /proc/bus/pci/...
When system started with normal NVidia driver, system not resume and halt, only
black screen.
There are many examples on the Internet with unsolved problems e.g
https://www.lwks.com/index.php?option=com_kunena&func=view&catid=21&id=124374&Itemid=81
--
You are receiving this mail because:
You are watching the assignee of the bug.
^ permalink raw reply
* Re: [PATCH v2 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains
From: Jon Hunter @ 2018-05-30 14:30 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rafael J . Wysocki, Linux PM, Greg Kroah-Hartman,
Geert Uytterhoeven, Todor Tomov, Rajendra Nayak, Viresh Kumar,
Vincent Guittot, Kevin Hilman, Linux Kernel Mailing List,
Linux ARM, linux-tegra
In-Reply-To: <CAPDyKFoiEctBuyw1nVS6ruQ92-jOsMMSAUsWKew42dkmk64_8A@mail.gmail.com>
On 30/05/18 12:31, Ulf Hansson wrote:
> On 30 May 2018 at 11:19, Jon Hunter <jonathanh@nvidia.com> wrote:
>> Hi Ulf,
>>
>> On 29/05/18 11:04, Ulf Hansson wrote:
>>> The existing dev_pm_domain_attach() function, allows a single PM domain to
>>> be attached per device. To be able to support devices that are partitioned
>>> across multiple PM domains, let's introduce a new interface,
>>> dev_pm_domain_attach_by_id().
>>>
>>> The dev_pm_domain_attach_by_id() returns a new allocated struct device with
>>> the corresponding attached PM domain. This enables for example a driver to
>>> operate on the new device from a power management point of view. The driver
>>> may then also benefit from using the received device, to set up so called
>>> device-links towards its original device. Depending on the situation, these
>>> links may then be dynamically changed.
>>
>> I have given this series a go with Tegra updating the XHCI driver to make
>> use of these new APIs. Good news it does appear to work fine for Tegra,
>> however, initially when looking at the device_link_add() API ...
>>
>> /**
>> * device_link_add - Create a link between two devices.
>> * @consumer: Consumer end of the link.
>> * @supplier: Supplier end of the link.
>> * @flags: Link flags.
>>
>> ... I had assumed that the 'consumer' device would be the actual XHCI
>> device (in the case of Tegra) and the 'supplier' device would be the new
>> genpd device. However, this did not work and I got the following WARN on
>> boot ...
>>
>> [ 2.050929] ---[ end trace eff0b5265e530c92 ]---
>> [ 2.055567] WARNING: CPU: 2 PID: 1 at drivers/base/core.c:446 device_links_driver_bound+0xc0/0xd0
>> [ 2.064422] Modules linked in:
>> [ 2.067471] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G W 4.17.0-rc7-next-20180529-00011-g4faf0dc0ebf3-dirty #32
>> [ 2.078667] Hardware name: Google Pixel C (DT)
>> [ 2.083101] pstate: 80000005 (Nzcv daif -PAN -UAO)
>> [ 2.087881] pc : device_links_driver_bound+0xc0/0xd0
>> [ 2.092832] lr : device_links_driver_bound+0x20/0xd0
>>
>> Switching the Tegra XHCI device to be the 'supplier' and genpd device to
>> be the 'consumer' does work, but is this correct? Seems to be opposite to
>
> It shall be the opposite. The Tegra XHCI device shall be the consumer.
>
>> what I expected. Maybe I am missing something?
>
> The problem you get is because the device returned from
> dev_pm_domain_attach_by_id(), let's call it genpd_dev, doesn't have
> the a driver.
>
> You need to use a couple of device links flag, something like this:
>
> link = device_link_add(dev, genpd_dev, DL_FLAG_STATELESS |
> DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
Thanks, adding the DL_FLAG_STATELESS flag resolved the issue. I already added
the DL_FLAG_PM_RUNTIME but I did not bother with the DL_FLAG_RPM_ACTIVE as
from the description it appears that this will always keep it on? Seems to
work fine without it.
> Moreover, you also need these commits, depending if you are running on
> something else than Rafael's tree.
>
> a0504aecba76 PM / runtime: Drop usage count for suppliers at device link removal
> 1e8378619841 PM / runtime: Fixup reference counting of device link
> suppliers at probe
Yes these are currently in -next and so I have these.
>>
>>> The new interface is typically called by drivers during their probe phase,
>>> in case they manages devices which uses multiple PM domains. If that is the
>>> case, the driver also becomes responsible of managing the detaching of the
>>> PM domains, which typically should be done at the remove phase. Detaching
>>> is done by calling the existing dev_pm_domain_detach() function and for
>>> each of the received devices from dev_pm_domain_attach_by_id().
>>>
>>> Note, currently its only genpd that supports multiple PM domains per
>>> device, but dev_pm_domain_attach_by_id() can easily by extended to cover
>>> other PM domain types, if/when needed.
>>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>> ---
>>>
>>> Changes in v2:
>>> - Fixed comments from Jon. Clarified function descriptions/changelog and
>>> return ERR_PTR(-EEXIST) in case a PM domain is already assigned.
>>> - Fix build error when CONFIG_PM is unset.
>>>
>>> ---
>>> drivers/base/power/common.c | 43 ++++++++++++++++++++++++++++++++++---
>>> include/linux/pm_domain.h | 7 ++++++
>>> 2 files changed, 47 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>>> index 7ae62b6355b8..5e5ea0c239de 100644
>>> --- a/drivers/base/power/common.c
>>> +++ b/drivers/base/power/common.c
>>> @@ -116,14 +116,51 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
>>> }
>>> EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>>>
>>> +/**
>>> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains.
>>> + * @dev: Device to attach.
>>
>> Nit ... I still don't think this is the device we are attaching to, but the
>> device the PM domains are associated with. IOW we are using this device to
>> lookup the PM domains.
>
> Right. I forgot to update that part of the description.
>
> What about:
>
> dev_pm_domain_attach_by_id - Associate a device with one of its PM domains.
> @dev: The device used to lookup the PM domain.
Perfect.
>>
>>> + * @index: The index of the PM domain.
>>> + *
>>> + * As @dev may only be attached to a single PM domain, the backend PM domain
>>> + * provider creates a virtual device to attach instead. If attachment succeeds,
>>> + * the ->detach() callback in the struct dev_pm_domain are assigned by the
>>> + * corresponding backend attach function, as to deal with detaching of the
>>> + * created virtual device.
>>> + *
>>> + * This function should typically be invoked by a driver during the probe phase,
>>> + * in case its device requires power management through multiple PM domains. The
>>> + * driver may benefit from using the received device, to configure device-links
>>> + * towards its original device. Depending on the use-case and if needed, the
>>> + * links may be dynamically changed by the driver, which allows it to control
>>> + * the power to the PM domains independently from each other.
>>> + *
>>> + * Callers must ensure proper synchronization of this function with power
>>> + * management callbacks.
>>> + *
>>> + * Returns the virtual created device when successfully attached to its PM
>>> + * domain, NULL in case @dev don't need a PM domain, else an ERR_PTR().
>>> + * Note that, to detach the returned virtual device, the driver shall call
>>> + * dev_pm_domain_detach() on it, typically during the remove phase.
>>> + */
>>> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
>>> + unsigned int index)
>>> +{
>>> + if (dev->pm_domain)
>>> + return ERR_PTR(-EEXIST);
>>> +
>>> + return genpd_dev_pm_attach_by_id(dev, index);
>>> +}
>>> +EXPORT_SYMBOL_GPL(dev_pm_domain_attach_by_id);
>>> +
>>> /**
>>> * dev_pm_domain_detach - Detach a device from its PM domain.
>>> * @dev: Device to detach.
>>> * @power_off: Used to indicate whether we should power off the device.
>>> *
>>> - * This functions will reverse the actions from dev_pm_domain_attach() and thus
>>> - * try to detach the @dev from its PM domain. Typically it should be invoked
>>> - * from subsystem level code during the remove phase.
>>> + * This functions will reverse the actions from dev_pm_domain_attach() and
>>> + * dev_pm_domain_attach_by_id(), thus it detaches @dev from its PM domain.
>>> + * Typically it should be invoked during the remove phase, either from
>>> + * subsystem level code or from drivers.
>>> *
>>> * Callers must ensure proper synchronization of this function with power
>>> * management callbacks.
>>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>>> index 82458e8e2e01..9206a4fef9ac 100644
>>> --- a/include/linux/pm_domain.h
>>> +++ b/include/linux/pm_domain.h
>>> @@ -299,6 +299,8 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
>>>
>>> #ifdef CONFIG_PM
>>> int dev_pm_domain_attach(struct device *dev, bool power_on);
>>> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
>>> + unsigned int index);
>>> void dev_pm_domain_detach(struct device *dev, bool power_off);
>>> void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd);
>>> #else
>>> @@ -306,6 +308,11 @@ static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
>>> {
>>> return 0;
>>> }
>>> +static inline struct device *dev_pm_domain_attach_by_id(struct device *dev,
>>> + unsigned int index)
>>> +{
>>> + return NULL;
>>> +}
>>> static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
>>> static inline void dev_pm_domain_set(struct device *dev,
>>> struct dev_pm_domain *pd) {}
>>>
>>
>> My only other comments on this series are ...
>>
>> 1. I think it would be nice to have an dev_pm_domain_attach_by_name() and
>> that the DT binding has a 'power-domain-names' property.
>
> I think it makes sense, but my plan was to do that as second step on
> top. Are you okay with that as well?
Yes that is fine with me.
>> 2. I am wondering if there could be value in having a
>> dev_pm_domain_attach_link_all() helper which would attach and link all
>> PM domains at once.
>
> Perhaps it can be useful, yes! However, maybe we can postpone that to
> after this series. I want to keep the series as simple as possible,
> then we can build upon it.
That's fine too and I can always add these if you prefer.
Cheers
Jon
--
nvpublic
^ permalink raw reply
* Re: [PATCH RESEND] serdev: add controller runtime PM support
From: Rob Herring @ 2018-05-30 13:20 UTC (permalink / raw)
To: Johan Hovold
Cc: Greg Kroah-Hartman, Sebastian Reichel, Tony Lindgren,
H. Nikolaus Schaller, Andreas Kemnade, Mark Rutland,
Arnd Bergmann, Pavel Machek, linux-kernel@vger.kernel.org,
open list:SERIAL DRIVERS, linux-pm
In-Reply-To: <20180530105059.21409-1-johan@kernel.org>
On Wed, May 30, 2018 at 5:50 AM, Johan Hovold <johan@kernel.org> wrote:
> Add support for controller runtime power management to serdev core. This
> is needed to allow slave drivers to manage the runtime PM state of the
> underlying serial controller when its driver, in turn, implements more
> aggressive runtime power management (e.g. using autosuspend).
>
> For some applications, for example, where loss off initial data after a
> remote-wakeup event is acceptable or where rx is not used at all,
> aggressive serial controller runtime PM may be used without further
> involvement of the slave driver. But when this is not the case, the
> slave driver must be able to indicate when incoming data is expected in
> order to avoid data loss.
>
> To facilitate the common case, where the serial controller power state
> is active whenever the port is open (which is the case with just about
> every serial driver), and where data loss is not acceptable and cannot
> even be prevented by explicit controller runtime power management, an
> RPM reference is taken in serdev open and put again at close. This
> reference can later be balanced by any serdev driver which wants and/or
> can handle aggressive controller runtime PM.
>
> Note that the .ignore_children flag is set for the serdev controller to
> allow the underlying hardware to idle when no I/O is expected, regardless
> of the slave device RPM state.
>
> Acked-by: Tony Lindgren <tony@atomide.com>
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> Signed-off-by: Johan Hovold <johan@kernel.org>
> ---
>
> Hi Rob and Greg,
>
> This is a resend of the serdev controller runtime PM patch, which you
> haven't commented on yet (possibly due to the following extensive
> discussions on how to generalise the aggressive OMAP serial runtime PM
> implementation).
>
> This patch works with what we have today, regardless of how we end up
> configuring the serial controller (active) runtime PM behaviour
> (currently done through sysfs for OMAP), which is a separate issue.
>
> No changes in this resend, besides me adding Tony's and Sebastian's ack
> and reviewed-by tags and dropping the second patch which only served as
> an example of how to use this in a serdev driver.
Acked-by: Rob Herring <robh@kernel.org>
Rob
^ permalink raw reply
* Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind
From: Vivek Gautam @ 2018-05-30 12:51 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: gregkh, lukas, dmitry.torokhov, aspriel, robin.murphy,
linux-kernel, linux-pm
In-Reply-To: <a85d44e7-3bb1-f282-088d-c4311a108bcd@intel.com>
Hi Rafael,
On 5/30/2018 4:29 PM, Rafael J. Wysocki wrote:
> On 5/30/2018 11:57 AM, Vivek Gautam wrote:
>> When using the device links without the consumers or
>> suppliers maintaining pointers to these links, a flag can
>> help in autoremoving the links on supplier driver unbind.
>> We remove these links only when the supplier's link to its
>> consumers has gone in DL_STATE_SUPPLIER_UNBIND state.
>>
>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
>> Suggested-by: Lukas Wunner <lukas@wunner.de>
>> ---
>>
>> Lukas, as suggested in the thread [1] this change adds additional flag
>> to autoremove device links on supplier unbind.
>> For arm-smmu, we want to _not_ keep references to the device links
>> added between arm-smmu, and consumer devices.
>> Robin also pointed to [2] the need to autoremove the device link on
>> supplier unbind rather than consumer unbind.
>
> Please CC device links patches to linux-pm.
Thanks for the quick review. Sure, will keep this noted from now on.
>
>> [1] https://lkml.org/lkml/2018/3/14/390
>> [2] https://lkml.org/lkml/2018/5/21/381
>>
>> drivers/base/core.c | 10 ++++++++++
>> include/linux/device.h | 2 ++
>> 2 files changed, 12 insertions(+)
>>
>> diff --git a/drivers/base/core.c b/drivers/base/core.c
>> index b610816eb887..52c7222bb3c4 100644
>> --- a/drivers/base/core.c
>> +++ b/drivers/base/core.c
>> @@ -490,6 +490,16 @@ void device_links_driver_cleanup(struct device
>> *dev)
>> WARN_ON(link->flags & DL_FLAG_AUTOREMOVE);
>> WARN_ON(link->status != DL_STATE_SUPPLIER_UNBIND);
>> +
>> + /*
>> + * autoremove the links between this @dev and its consumer
>> + * devices that are not active, i.e. where the link state
>> + * has moved to DL_STATE_SUPPLIER_UNBIND.
>> + */
>> + if (link->status == DL_STATE_SUPPLIER_UNBIND &&
>> + link->flags & DL_FLAG_AUTOREMOVE_S)
>> + kref_put(&link->kref, __device_link_del);
>> +
>> WRITE_ONCE(link->status, DL_STATE_DORMANT);
>> }
>> diff --git a/include/linux/device.h b/include/linux/device.h
>> index 477956990f5e..6033bf58453d 100644
>> --- a/include/linux/device.h
>> +++ b/include/linux/device.h
>> @@ -779,11 +779,13 @@ enum device_link_state {
>> * AUTOREMOVE: Remove this link automatically on consumer driver
>> unbind.
>> * PM_RUNTIME: If set, the runtime PM framework will use this link.
>> * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during
>> link creation.
>> + * AUTOREMOVE_S: Remove this link automatically on supplier driver
>> unbind.
>> */
>> #define DL_FLAG_STATELESS BIT(0)
>> #define DL_FLAG_AUTOREMOVE BIT(1)
>> #define DL_FLAG_PM_RUNTIME BIT(2)
>> #define DL_FLAG_RPM_ACTIVE BIT(3)
>> +#define DL_FLAG_AUTOREMOVE_S BIT(4)
>
> Couldn't you invent a better name for this one?
Frankly, I wanted to have something like DL_FLAG_AUTOREMOVE_SUPPLIER,
but that
felt a bit too long considering other flags.
Can you please consider suggesting a concise name?
Regards
Vivek
>
>> /**
>> * struct device_link - Device link representation.
>
>
^ permalink raw reply
* [PATCH 2/2] cpuidle: governors: Consolidate PM QoS handling
From: Rafael J. Wysocki @ 2018-05-30 11:43 UTC (permalink / raw)
To: Linux PM; +Cc: LKML, Peter Zijlstra, Daniel Lezcano, Ramesh Thomas
In-Reply-To: <2261593.SGWtJkXgY3@aspire.rjw.lan>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
There is some code duplication related to the PM QoS handling between
the existing cpuidle governors, so move that code to a common helper
function and call that from the governors.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpuidle/governor.c | 17 ++++++++++++++++-
drivers/cpuidle/governors/ladder.c | 9 +--------
| 9 +--------
include/linux/cpuidle.h | 1 +
4 files changed, 19 insertions(+), 17 deletions(-)
Index: linux-pm/include/linux/cpuidle.h
===================================================================
--- linux-pm.orig/include/linux/cpuidle.h
+++ linux-pm/include/linux/cpuidle.h
@@ -268,6 +268,7 @@ struct cpuidle_governor {
#ifdef CONFIG_CPU_IDLE
extern int cpuidle_register_governor(struct cpuidle_governor *gov);
+extern int cpuidle_governor_latency_req(unsigned int cpu);
#else
static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
{return 0;}
Index: linux-pm/drivers/cpuidle/governor.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governor.c
+++ linux-pm/drivers/cpuidle/governor.c
@@ -8,8 +8,10 @@
* This code is licenced under the GPL.
*/
-#include <linux/mutex.h>
+#include <linux/cpu.h>
#include <linux/cpuidle.h>
+#include <linux/mutex.h>
+#include <linux/pm_qos.h>
#include "cpuidle.h"
@@ -93,3 +95,16 @@ int cpuidle_register_governor(struct cpu
return ret;
}
+
+/**
+ * cpuidle_governor_latency_req - Compute a latency constraint for CPU
+ * @cpu: Target CPU
+ */
+int cpuidle_governor_latency_req(unsigned int cpu)
+{
+ int global_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
+ struct device *device = get_cpu_device(cpu);
+ int device_req = dev_pm_qos_raw_read_value(device);
+
+ return device_req < global_req ? device_req : global_req;
+}
Index: linux-pm/drivers/cpuidle/governors/ladder.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/ladder.c
+++ linux-pm/drivers/cpuidle/governors/ladder.c
@@ -14,10 +14,8 @@
#include <linux/kernel.h>
#include <linux/cpuidle.h>
-#include <linux/pm_qos.h>
#include <linux/jiffies.h>
#include <linux/tick.h>
-#include <linux/cpu.h>
#include <asm/io.h>
#include <linux/uaccess.h>
@@ -69,15 +67,10 @@ static int ladder_select_state(struct cp
struct cpuidle_device *dev, bool *dummy)
{
struct ladder_device *ldev = this_cpu_ptr(&ladder_devices);
- struct device *device = get_cpu_device(dev->cpu);
struct ladder_device_state *last_state;
int last_residency, last_idx = ldev->last_state_idx;
int first_idx = drv->states[0].flags & CPUIDLE_FLAG_POLLING ? 1 : 0;
- int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
- int resume_latency = dev_pm_qos_raw_read_value(device);
-
- if (resume_latency < latency_req)
- latency_req = resume_latency;
+ int latency_req = cpuidle_governor_latency_req(dev->cpu);
/* Special case when user has set very strict latency requirement */
if (unlikely(latency_req == 0)) {
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -12,7 +12,6 @@
#include <linux/kernel.h>
#include <linux/cpuidle.h>
-#include <linux/pm_qos.h>
#include <linux/time.h>
#include <linux/ktime.h>
#include <linux/hrtimer.h>
@@ -21,7 +20,6 @@
#include <linux/sched/loadavg.h>
#include <linux/sched/stat.h>
#include <linux/math64.h>
-#include <linux/cpu.h>
/*
* Please note when changing the tuning values:
@@ -286,15 +284,13 @@ static int menu_select(struct cpuidle_dr
bool *stop_tick)
{
struct menu_device *data = this_cpu_ptr(&menu_devices);
- struct device *device = get_cpu_device(dev->cpu);
- int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
+ int latency_req = cpuidle_governor_latency_req(dev->cpu);
int i;
int first_idx;
int idx;
unsigned int interactivity_req;
unsigned int expected_interval;
unsigned long nr_iowaiters, cpu_load;
- int resume_latency = dev_pm_qos_raw_read_value(device);
ktime_t delta_next;
bool prediction_override = false;
bool prediction_override_tick = false;
@@ -304,9 +300,6 @@ static int menu_select(struct cpuidle_dr
data->needs_update = 0;
}
- if (resume_latency < latency_req)
- latency_req = resume_latency;
-
/* Special case when user has set very strict latency requirement */
if (unlikely(latency_req == 0)) {
*stop_tick = false;
^ permalink raw reply
* [PATCH 1/2] cpuidle: governors: Drop redundant checks related to PM QoS
From: Rafael J. Wysocki @ 2018-05-30 11:41 UTC (permalink / raw)
To: Linux PM; +Cc: LKML, Peter Zijlstra, Daniel Lezcano, Ramesh Thomas
In-Reply-To: <2261593.SGWtJkXgY3@aspire.rjw.lan>
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
PM_QOS_RESUME_LATENCY_NO_CONSTRAINT is defined as the 32-bit integer
maximum, so it is not necessary to test the return value of
dev_pm_qos_raw_read_value() against it directly in the menu and
ladder cpuidle governors.
Drop these redundant checks.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpuidle/governors/ladder.c | 3 +--
| 3 +--
2 files changed, 2 insertions(+), 4 deletions(-)
Index: linux-pm/drivers/cpuidle/governors/ladder.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/ladder.c
+++ linux-pm/drivers/cpuidle/governors/ladder.c
@@ -76,8 +76,7 @@ static int ladder_select_state(struct cp
int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
int resume_latency = dev_pm_qos_raw_read_value(device);
- if (resume_latency < latency_req &&
- resume_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
+ if (resume_latency < latency_req)
latency_req = resume_latency;
/* Special case when user has set very strict latency requirement */
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -304,8 +304,7 @@ static int menu_select(struct cpuidle_dr
data->needs_update = 0;
}
- if (resume_latency < latency_req &&
- resume_latency != PM_QOS_RESUME_LATENCY_NO_CONSTRAINT)
+ if (resume_latency < latency_req)
latency_req = resume_latency;
/* Special case when user has set very strict latency requirement */
^ permalink raw reply
* [PATCH 0/2] cpuidle: governors: Cleanups related to PM QoS
From: Rafael J. Wysocki @ 2018-05-30 11:40 UTC (permalink / raw)
To: Linux PM; +Cc: LKML, Peter Zijlstra, Daniel Lezcano, Ramesh Thomas
Hi All,
These patches clean up a couple of things related to PM QoS in cpuidle
governors. Please refer to the changelogs for details.
Thanks,
Rafael
^ permalink raw reply
* Re: [PATCH v2 0/9] PM / Domains: Add support for multi PM domains per device
From: Ulf Hansson @ 2018-05-30 11:34 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J . Wysocki, Linux PM, Greg Kroah-Hartman, Jon Hunter,
Geert Uytterhoeven, Todor Tomov, Rajendra Nayak, Viresh Kumar,
Vincent Guittot, Kevin Hilman, Linux Kernel Mailing List,
Linux ARM, linux-tegra
In-Reply-To: <CAJZ5v0iOP-gcdXcaOjuWy6p+k+hSkHuapDuYYFeS+6gDrxev3w@mail.gmail.com>
On 30 May 2018 at 11:30, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, May 29, 2018 at 12:04 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> Changes in v2:
>> - Addressed comments from Geert around DT doc.
>> - Addressed comments from Jon around clarification of how to use this
>> and changes to returned error codes.
>> - Fixed build error in case CONFIG_PM was unset.
>>
>> There are devices that are partitioned across multiple PM domains. Currently
>> these can't be supported well by the available PM infrastructures we have in
>> the kernel. This series is an attempt to address this.
>>
>> The interesting parts happens from patch 5 an onwards, including a minor DT
>> update to the existing power-domain bindings, the 4 earlier are just trivial
>> clean-ups of some related code in genpd, which I happened to stumble over.
>>
>> Some additional background:
>>
>> One existing case where devices are partitioned across multiple PM domains, is
>> the Nvida Tegra 124/210 X-USB subsystem. A while ago Jon Hunter (Nvidia) sent a
>> series, trying to address these issues, however this is a new approach, while
>> it re-uses the same concepts from DT point of view.
>>
>> The Tegra 124/210 X-USB subsystem contains of a host controller and a device
>> controller. Each controller have its own independent PM domain, but are being
>> partitioned across another shared PM domain for the USB super-speed logic.
>>
>> Currently to make the drivers work, either the related PM domains needs to stay
>> powered on always or the PM domain topology needs to be in-correctly modelled
>> through sub-domains. In both cases PM domains may be powered on while they
>> don't need to be, so in the end this means - wasting power -.
>>
>> As stated above, this series intends to address these problem from a PM
>> infrastructure point of view. More details are available in each changelog.
>>
>> It should be noted that this series has been tested on HW, however only by using
>> a home-cooked test PM domain driver for genpd and together with a test driver.
>> This allowed me to play with PM domain (genpd), runtime PM and device links.
>>
>> Any further deployment for real use cases are greatly appreciated. I am happy to
>> to help, if needed!
>>
>> Kind regards
>> Ulf Hansson
>>
>>
>> Ulf Hansson (9):
>> PM / Domains: Drop extern declarations of functions in pm_domain.h
>> PM / Domains: Drop __pm_genpd_add_device()
>> PM / Domains: Drop genpd as in-param for pm_genpd_remove_device()
>> PM / Domains: Drop unused parameter in genpd_allocate_dev_data()
>> PM / Domains: dt: Allow power-domain property to be a list of
>> specifiers
>> PM / Domains: Don't attach devices in genpd with multi PM domains
>> PM / Domains: Split genpd_dev_pm_attach()
>> PM / Domains: Add support for multi PM domains per device to genpd
>> PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM
>> domains
>>
>> .../bindings/power/power_domain.txt | 19 ++-
>> drivers/base/power/common.c | 39 ++++-
>> drivers/base/power/domain.c | 155 ++++++++++++++----
>> drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 2 +-
>> include/linux/pm_domain.h | 79 ++++-----
>> 5 files changed, 216 insertions(+), 78 deletions(-)
>
> I can take patches [1-4/9] from this series for 4.18.
Thanks!
>
> I need an ACK from Rob on the bindings change.
Yes, of course!
>
> The rest of the series doesn't seem to be ready yet.
It's getting reviewed and tested, so let's give it some more time.
BTW, from an overall design point of view, you seems okay with it?
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v2 9/9] PM / Domains: Add dev_pm_domain_attach_by_id() to manage multi PM domains
From: Ulf Hansson @ 2018-05-30 11:31 UTC (permalink / raw)
To: Jon Hunter
Cc: Rafael J . Wysocki, Linux PM, Greg Kroah-Hartman,
Geert Uytterhoeven, Todor Tomov, Rajendra Nayak, Viresh Kumar,
Vincent Guittot, Kevin Hilman, Linux Kernel Mailing List,
Linux ARM, linux-tegra
In-Reply-To: <cc6f36ce-32b7-4b69-f9a9-98cf0b12a9d1@nvidia.com>
On 30 May 2018 at 11:19, Jon Hunter <jonathanh@nvidia.com> wrote:
> Hi Ulf,
>
> On 29/05/18 11:04, Ulf Hansson wrote:
>> The existing dev_pm_domain_attach() function, allows a single PM domain to
>> be attached per device. To be able to support devices that are partitioned
>> across multiple PM domains, let's introduce a new interface,
>> dev_pm_domain_attach_by_id().
>>
>> The dev_pm_domain_attach_by_id() returns a new allocated struct device with
>> the corresponding attached PM domain. This enables for example a driver to
>> operate on the new device from a power management point of view. The driver
>> may then also benefit from using the received device, to set up so called
>> device-links towards its original device. Depending on the situation, these
>> links may then be dynamically changed.
>
> I have given this series a go with Tegra updating the XHCI driver to make
> use of these new APIs. Good news it does appear to work fine for Tegra,
> however, initially when looking at the device_link_add() API ...
>
> /**
> * device_link_add - Create a link between two devices.
> * @consumer: Consumer end of the link.
> * @supplier: Supplier end of the link.
> * @flags: Link flags.
>
> ... I had assumed that the 'consumer' device would be the actual XHCI
> device (in the case of Tegra) and the 'supplier' device would be the new
> genpd device. However, this did not work and I got the following WARN on
> boot ...
>
> [ 2.050929] ---[ end trace eff0b5265e530c92 ]---
> [ 2.055567] WARNING: CPU: 2 PID: 1 at drivers/base/core.c:446 device_links_driver_bound+0xc0/0xd0
> [ 2.064422] Modules linked in:
> [ 2.067471] CPU: 2 PID: 1 Comm: swapper/0 Tainted: G W 4.17.0-rc7-next-20180529-00011-g4faf0dc0ebf3-dirty #32
> [ 2.078667] Hardware name: Google Pixel C (DT)
> [ 2.083101] pstate: 80000005 (Nzcv daif -PAN -UAO)
> [ 2.087881] pc : device_links_driver_bound+0xc0/0xd0
> [ 2.092832] lr : device_links_driver_bound+0x20/0xd0
>
> Switching the Tegra XHCI device to be the 'supplier' and genpd device to
> be the 'consumer' does work, but is this correct? Seems to be opposite to
It shall be the opposite. The Tegra XHCI device shall be the consumer.
> what I expected. Maybe I am missing something?
The problem you get is because the device returned from
dev_pm_domain_attach_by_id(), let's call it genpd_dev, doesn't have
the a driver.
You need to use a couple of device links flag, something like this:
link = device_link_add(dev, genpd_dev, DL_FLAG_STATELESS |
DL_FLAG_PM_RUNTIME | DL_FLAG_RPM_ACTIVE);
Moreover, you also need these commits, depending if you are running on
something else than Rafael's tree.
a0504aecba76 PM / runtime: Drop usage count for suppliers at device link removal
1e8378619841 PM / runtime: Fixup reference counting of device link
suppliers at probe
>
>> The new interface is typically called by drivers during their probe phase,
>> in case they manages devices which uses multiple PM domains. If that is the
>> case, the driver also becomes responsible of managing the detaching of the
>> PM domains, which typically should be done at the remove phase. Detaching
>> is done by calling the existing dev_pm_domain_detach() function and for
>> each of the received devices from dev_pm_domain_attach_by_id().
>>
>> Note, currently its only genpd that supports multiple PM domains per
>> device, but dev_pm_domain_attach_by_id() can easily by extended to cover
>> other PM domain types, if/when needed.
>>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>> ---
>>
>> Changes in v2:
>> - Fixed comments from Jon. Clarified function descriptions/changelog and
>> return ERR_PTR(-EEXIST) in case a PM domain is already assigned.
>> - Fix build error when CONFIG_PM is unset.
>>
>> ---
>> drivers/base/power/common.c | 43 ++++++++++++++++++++++++++++++++++---
>> include/linux/pm_domain.h | 7 ++++++
>> 2 files changed, 47 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/base/power/common.c b/drivers/base/power/common.c
>> index 7ae62b6355b8..5e5ea0c239de 100644
>> --- a/drivers/base/power/common.c
>> +++ b/drivers/base/power/common.c
>> @@ -116,14 +116,51 @@ int dev_pm_domain_attach(struct device *dev, bool power_on)
>> }
>> EXPORT_SYMBOL_GPL(dev_pm_domain_attach);
>>
>> +/**
>> + * dev_pm_domain_attach_by_id - Attach a device to one of its PM domains.
>> + * @dev: Device to attach.
>
> Nit ... I still don't think this is the device we are attaching to, but the
> device the PM domains are associated with. IOW we are using this device to
> lookup the PM domains.
Right. I forgot to update that part of the description.
What about:
dev_pm_domain_attach_by_id - Associate a device with one of its PM domains.
@dev: The device used to lookup the PM domain.
>
>> + * @index: The index of the PM domain.
>> + *
>> + * As @dev may only be attached to a single PM domain, the backend PM domain
>> + * provider creates a virtual device to attach instead. If attachment succeeds,
>> + * the ->detach() callback in the struct dev_pm_domain are assigned by the
>> + * corresponding backend attach function, as to deal with detaching of the
>> + * created virtual device.
>> + *
>> + * This function should typically be invoked by a driver during the probe phase,
>> + * in case its device requires power management through multiple PM domains. The
>> + * driver may benefit from using the received device, to configure device-links
>> + * towards its original device. Depending on the use-case and if needed, the
>> + * links may be dynamically changed by the driver, which allows it to control
>> + * the power to the PM domains independently from each other.
>> + *
>> + * Callers must ensure proper synchronization of this function with power
>> + * management callbacks.
>> + *
>> + * Returns the virtual created device when successfully attached to its PM
>> + * domain, NULL in case @dev don't need a PM domain, else an ERR_PTR().
>> + * Note that, to detach the returned virtual device, the driver shall call
>> + * dev_pm_domain_detach() on it, typically during the remove phase.
>> + */
>> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
>> + unsigned int index)
>> +{
>> + if (dev->pm_domain)
>> + return ERR_PTR(-EEXIST);
>> +
>> + return genpd_dev_pm_attach_by_id(dev, index);
>> +}
>> +EXPORT_SYMBOL_GPL(dev_pm_domain_attach_by_id);
>> +
>> /**
>> * dev_pm_domain_detach - Detach a device from its PM domain.
>> * @dev: Device to detach.
>> * @power_off: Used to indicate whether we should power off the device.
>> *
>> - * This functions will reverse the actions from dev_pm_domain_attach() and thus
>> - * try to detach the @dev from its PM domain. Typically it should be invoked
>> - * from subsystem level code during the remove phase.
>> + * This functions will reverse the actions from dev_pm_domain_attach() and
>> + * dev_pm_domain_attach_by_id(), thus it detaches @dev from its PM domain.
>> + * Typically it should be invoked during the remove phase, either from
>> + * subsystem level code or from drivers.
>> *
>> * Callers must ensure proper synchronization of this function with power
>> * management callbacks.
>> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> index 82458e8e2e01..9206a4fef9ac 100644
>> --- a/include/linux/pm_domain.h
>> +++ b/include/linux/pm_domain.h
>> @@ -299,6 +299,8 @@ struct generic_pm_domain *of_genpd_remove_last(struct device_node *np)
>>
>> #ifdef CONFIG_PM
>> int dev_pm_domain_attach(struct device *dev, bool power_on);
>> +struct device *dev_pm_domain_attach_by_id(struct device *dev,
>> + unsigned int index);
>> void dev_pm_domain_detach(struct device *dev, bool power_off);
>> void dev_pm_domain_set(struct device *dev, struct dev_pm_domain *pd);
>> #else
>> @@ -306,6 +308,11 @@ static inline int dev_pm_domain_attach(struct device *dev, bool power_on)
>> {
>> return 0;
>> }
>> +static inline struct device *dev_pm_domain_attach_by_id(struct device *dev,
>> + unsigned int index)
>> +{
>> + return NULL;
>> +}
>> static inline void dev_pm_domain_detach(struct device *dev, bool power_off) {}
>> static inline void dev_pm_domain_set(struct device *dev,
>> struct dev_pm_domain *pd) {}
>>
>
> My only other comments on this series are ...
>
> 1. I think it would be nice to have an dev_pm_domain_attach_by_name() and
> that the DT binding has a 'power-domain-names' property.
I think it makes sense, but my plan was to do that as second step on
top. Are you okay with that as well?
> 2. I am wondering if there could be value in having a
> dev_pm_domain_attach_link_all() helper which would attach and link all
> PM domains at once.
Perhaps it can be useful, yes! However, maybe we can postpone that to
after this series. I want to keep the series as simple as possible,
then we can build upon it.
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH V3] cpufreq: reinitialize new policy min/max when writing scaling_(max|min)_freq
From: Rafael J. Wysocki @ 2018-05-30 11:10 UTC (permalink / raw)
To: Kevin Wangtao
Cc: Rafael J. Wysocki, Viresh Kumar, Linux PM,
Linux Kernel Mailing List, gengyanping, sunzhaosheng
In-Reply-To: <1527667007-48226-1-git-send-email-kevin.wangtao@hisilicon.com>
On Wed, May 30, 2018 at 9:56 AM, Kevin Wangtao
<kevin.wangtao@hisilicon.com> wrote:
> consider such situation, current user_policy.min is 1000000,
> current user_policy.max is 1200000, in cpufreq_set_policy,
> other driver may update policy.min to 1200000, policy.max to
> 1300000. After that, If we input "echo 1300000 > scaling_min_freq",
> then user_policy.min will be 1300000, and user_policy.max is
> still 1200000, because the input value is checked with policy.max
> not user_policy.max. if we get all related cpus offline and
> online again, it will cause cpufreq_init_policy fail because
> user_policy.min is higher than user_policy.max.
>
> The solution is when user space tries to write scaling_(max|min)_freq,
> the min/max of new_policy should be reinitialized with min/max
> of user_policy, like what cpufreq_update_policy does.
>
> Signed-off-by: Kevin Wangtao <kevin.wangtao@hisilicon.com>
I've applied the v2 with modified subject and changelog and with the
ACK from Viresh.
Thanks!
^ permalink raw reply
* Re: [PATCH 1/2] kernel/SRCU: provide a static initializer
From: Rafael J. Wysocki @ 2018-05-30 11:09 UTC (permalink / raw)
To: Paul McKenney
Cc: Rafael J. Wysocki, Sebastian Andrzej Siewior, Linux PM,
Linux Kernel Mailing List, Rafael J. Wysocki, Viresh Kumar,
Thomas Gleixner
In-Reply-To: <20180529172611.GL3803@linux.vnet.ibm.com>
On Tue, May 29, 2018 at 7:26 PM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Tue, May 29, 2018 at 07:14:49PM +0200, Rafael J. Wysocki wrote:
>> On Tue, May 29, 2018 at 2:04 PM, Paul E. McKenney
>> <paulmck@linux.vnet.ibm.com> wrote:
>> > On Tue, May 29, 2018 at 10:09:51AM +0200, Rafael J. Wysocki wrote:
>> >> On Fri, May 25, 2018 at 12:19 PM, Sebastian Andrzej Siewior
>> >> <bigeasy@linutronix.de> wrote:
>> >> > There are macros for static initializer for the three out of four
>> >> > possible notifier types, that are:
>> >> > ATOMIC_NOTIFIER_HEAD()
>> >> > BLOCKING_NOTIFIER_HEAD()
>> >> > RAW_NOTIFIER_HEAD()
>> >> >
>> >> > This patch provides a static initilizer for the forth type to make it
>> >> > complete.
>> >> >
>> >> > Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>> >> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>> >>
>> >> I cannot apply this without an ACK from Paul.
>> >
>> > I have both queued, but if you would prefer to take them, then for the
>> > SRCU piece:
>> >
>> > Tested-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>> > Reviewed-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>>
>> Thanks!
>>
>> I'll route them along with the other cpufreq material then.
>
> Please let me know when you have them queued so that I can remove
> them from -rcu.
I've just added them to my linux-next branch.
Cheers,
Rafael
^ permalink raw reply
* Re: [PATCH v14 0/2] Kryo CPU scaling driver
From: Ilia Lin @ 2018-05-30 11:06 UTC (permalink / raw)
To: Viresh Kumar, Rafael J. Wysocki
Cc: Viresh Kumar, Nishanth Menon, Stephen Boyd, Rob Herring,
Mark Rutland, Rafael J. Wysocki, Linux PM,
devicetree@vger.kernel.org, Linux Kernel Mailing List
In-Reply-To: <20180530091535.bzvhg4bvn32mqwgq@vireshk-i7>
Just like Viresh said, this is fixed in the v15.
On May 30, 2018 12:15:35 PM GMT+03:00, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>On 30-05-18, 10:08, Rafael J. Wysocki wrote:
>> On Fri, May 25, 2018 at 2:07 PM, Ilia Lin <ilialin@codeaurora.org>
>wrote:
>> > [v14]
>> > * Addressed comment from Sudeep about DT compatible
>> > * Added MAINTAINERS entry
>>
>> This causes a build issue to occur in my bleeding-edge branch.
>>
>> Does it depend on anything new in arm-soc?
>
>He already sent a v15 and the changelog suggest he fixed that build
>issue.
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply
* Re: [PATCH 1/1] device core: Add flag to autoremove device link on supplier unbind
From: Rafael J. Wysocki @ 2018-05-30 10:59 UTC (permalink / raw)
To: Vivek Gautam
Cc: gregkh, lukas, dmitry.torokhov, aspriel, robin.murphy,
linux-kernel, linux-pm
In-Reply-To: <20180530095704.5788-1-vivek.gautam@codeaurora.org>
On 5/30/2018 11:57 AM, Vivek Gautam wrote:
> When using the device links without the consumers or
> suppliers maintaining pointers to these links, a flag can
> help in autoremoving the links on supplier driver unbind.
> We remove these links only when the supplier's link to its
> consumers has gone in DL_STATE_SUPPLIER_UNBIND state.
>
> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org>
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> ---
>
> Lukas, as suggested in the thread [1] this change adds additional flag
> to autoremove device links on supplier unbind.
> For arm-smmu, we want to _not_ keep references to the device links
> added between arm-smmu, and consumer devices.
> Robin also pointed to [2] the need to autoremove the device link on
> supplier unbind rather than consumer unbind.
Please CC device links patches to linux-pm.
> [1] https://lkml.org/lkml/2018/3/14/390
> [2] https://lkml.org/lkml/2018/5/21/381
>
> drivers/base/core.c | 10 ++++++++++
> include/linux/device.h | 2 ++
> 2 files changed, 12 insertions(+)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index b610816eb887..52c7222bb3c4 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -490,6 +490,16 @@ void device_links_driver_cleanup(struct device *dev)
>
> WARN_ON(link->flags & DL_FLAG_AUTOREMOVE);
> WARN_ON(link->status != DL_STATE_SUPPLIER_UNBIND);
> +
> + /*
> + * autoremove the links between this @dev and its consumer
> + * devices that are not active, i.e. where the link state
> + * has moved to DL_STATE_SUPPLIER_UNBIND.
> + */
> + if (link->status == DL_STATE_SUPPLIER_UNBIND &&
> + link->flags & DL_FLAG_AUTOREMOVE_S)
> + kref_put(&link->kref, __device_link_del);
> +
> WRITE_ONCE(link->status, DL_STATE_DORMANT);
> }
>
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 477956990f5e..6033bf58453d 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -779,11 +779,13 @@ enum device_link_state {
> * AUTOREMOVE: Remove this link automatically on consumer driver unbind.
> * PM_RUNTIME: If set, the runtime PM framework will use this link.
> * RPM_ACTIVE: Run pm_runtime_get_sync() on the supplier during link creation.
> + * AUTOREMOVE_S: Remove this link automatically on supplier driver unbind.
> */
> #define DL_FLAG_STATELESS BIT(0)
> #define DL_FLAG_AUTOREMOVE BIT(1)
> #define DL_FLAG_PM_RUNTIME BIT(2)
> #define DL_FLAG_RPM_ACTIVE BIT(3)
> +#define DL_FLAG_AUTOREMOVE_S BIT(4)
Couldn't you invent a better name for this one?
>
> /**
> * struct device_link - Device link representation.
^ permalink raw reply
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