* Re: [PATCH v3] PM / wakeup: show wakeup sources stats in sysfs
From: Tri Vo @ 2019-07-15 21:48 UTC (permalink / raw)
To: Greg KH
Cc: Rafael J. Wysocki, Viresh Kumar, Rafael J. Wysocki,
Hridya Valsaraju, Sandeep Patil, kaleshsingh, LKML, Linux PM,
Cc: Android Kernel
In-Reply-To: <20190715203651.GA7513@kroah.com>
On Mon, Jul 15, 2019 at 1:37 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 15, 2019 at 01:11:16PM -0700, Tri Vo wrote:
> > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > blocking wakeup sources over device's boot cycle. This information can
> > then be used (1) for power-specific bug reporting and (2) towards
> > attributing battery consumption to specific processes over a period of
> > time.
> >
> > However, debugfs doesn't have stable ABI. For this reason, create a
> > 'struct device' to expose wakeup sources statistics in sysfs under
> > /sys/class/wakeup/<name>/.
> >
> > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > source statistics in sysfs.
> >
> > Suggested-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Tri Vo <trong@android.com>
> > Tested-by: Tri Vo <trong@android.com>
>
> Co-Developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> perhaps given that I rewrote the whole file last time? :)
Thanks again for the help!
>
>
> > ---
> > Documentation/ABI/testing/sysfs-power | 73 ++++++++++++-
> > drivers/base/power/Makefile | 1 +
> > drivers/base/power/wakeup.c | 12 ++-
> > drivers/base/power/wakeup_stats.c | 148 ++++++++++++++++++++++++++
> > include/linux/pm_wakeup.h | 19 ++++
> > kernel/power/Kconfig | 10 ++
> > kernel/power/wakelock.c | 10 ++
> > 7 files changed, 270 insertions(+), 3 deletions(-)
> > create mode 100644 drivers/base/power/wakeup_stats.c
>
> What changed from v2? :)
Oops, my bad.
> And I am guessing that you actually tested this all out, and it works
> for you?
Yes, I played around with wakelocks to make sure that wakeup source
stats are added/updated/removed as expected.
> Have you changed Android userspace to use the new api with no
> problems?
Kalesh helped me test this patch (added him in Tested-by: field in
latest patch version). We haven't tested beyond booting and manual
inspection on android devices. Android userspace changes should be
fairly trivial though.
^ permalink raw reply
* Re: [PATCH v4] PM / wakeup: show wakeup sources stats in sysfs
From: Rafael J. Wysocki @ 2019-07-15 21:48 UTC (permalink / raw)
To: Tri Vo
Cc: Rafael J. Wysocki, Greg Kroah-Hartman, Viresh Kumar,
Rafael J. Wysocki, Hridya Valsaraju, sspatil, kaleshsingh,
Linux Kernel Mailing List, Linux PM, Cc: Android Kernel
In-Reply-To: <20190715214348.81865-1-trong@android.com>
On Mon, Jul 15, 2019 at 11:44 PM Tri Vo <trong@android.com> wrote:
>
> Userspace can use wakeup_sources debugfs node to plot history of suspend
> blocking wakeup sources over device's boot cycle. This information can
> then be used (1) for power-specific bug reporting and (2) towards
> attributing battery consumption to specific processes over a period of
> time.
>
> However, debugfs doesn't have stable ABI. For this reason, create a
> 'struct device' to expose wakeup sources statistics in sysfs under
> /sys/class/wakeup/<name>/.
>
> Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> source statistics in sysfs.
I'm not sure if this is really needed, but I'll let Greg decide.
Apart from this
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Co-developed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Tri Vo <trong@android.com>
> Tested-by: Tri Vo <trong@android.com>
> Tested-by: Kalesh Singh <kaleshsingh@google.com>
> ---
> Documentation/ABI/testing/sysfs-class-wakeup | 70 +++++++++
> drivers/base/power/Makefile | 1 +
> drivers/base/power/wakeup.c | 12 +-
> drivers/base/power/wakeup_stats.c | 149 +++++++++++++++++++
> include/linux/pm_wakeup.h | 19 +++
> kernel/power/Kconfig | 10 ++
> kernel/power/wakelock.c | 10 ++
> 7 files changed, 269 insertions(+), 2 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-class-wakeup
> create mode 100644 drivers/base/power/wakeup_stats.c
>
> v2:
> - Updated Documentation/ABI/, as per Greg.
> - Removed locks in attribute functions, as per Greg.
> - Lifetimes of struct wakelock and struck wakeup_source are now different due to
> the latter embedding a refcounted kobject. Changed it so that struct wakelock
> only has a pointer to struct wakeup_source, instead of embedding it.
> - Added CONFIG_PM_SLEEP_STATS that enables/disables wakeup source statistics in
> sysfs.
>
> v3:
> Changes by Greg:
> - Reworked code to use 'struct device' instead of raw kobjects.
> - Updated documentation file.
> - Only link wakeup_stats.o when CONFIG_PM_SLEEP_STATS is enabled.
> Changes by Tri:
> - Reverted changes to kernel/power/wakelock.c. 'struct device' hides kobject
> operations. So no need to handle lifetimes in wakelock.c
>
> v4:
> - Added 'Co-developed-by:' and 'Tested-by:' fields to commit message.
> - Moved new documentation to a separate file
> Documentation/ABI/testing/sysfs-class-wakeup, as per Greg.
> - Fixed copyright header in drivers/base/power/wakeup_stats.c, as per Greg.
>
> diff --git a/Documentation/ABI/testing/sysfs-class-wakeup b/Documentation/ABI/testing/sysfs-class-wakeup
> new file mode 100644
> index 000000000000..30fb23eb9112
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-wakeup
> @@ -0,0 +1,70 @@
> +What: /sys/class/wakeup/
> +Date: June 2019
> +Contact: Tri Vo <trong@android.com>
> +Description:
> + The /sys/class/wakeup/ directory contains pointers to all
> + wakeup sources in the kernel at that moment in time.
> +
> +What: /sys/class/wakeup/.../active_count
> +Date: June 2019
> +Contact: Tri Vo <trong@android.com>
> +Description:
> + This file contains the number of times the wakeup source was
> + activated.
> +
> +What: /sys/class/wakeup/.../event_count
> +Date: June 2019
> +Contact: Tri Vo <trong@android.com>
> +Description:
> + This file contains the number of signaled wakeup events
> + associated with the wakeup source.
> +
> +What: /sys/class/wakeup/.../wakeup_count
> +Date: June 2019
> +Contact: Tri Vo <trong@android.com>
> +Description:
> + This file contains the number of times the wakeup source might
> + abort suspend.
> +
> +What: /sys/class/wakeup/.../expire_count
> +Date: June 2019
> +Contact: Tri Vo <trong@android.com>
> +Description:
> + This file contains the number of times the wakeup source's
> + timeout has expired.
> +
> +What: /sys/class/wakeup/.../active_time_ms
> +Date: June 2019
> +Contact: Tri Vo <trong@android.com>
> +Description:
> + This file contains the amount of time the wakeup source has
> + been continuously active, in milliseconds. If the wakeup
> + source is not active, this file contains '0'.
> +
> +What: /sys/class/wakeup/.../total_time_ms
> +Date: June 2019
> +Contact: Tri Vo <trong@android.com>
> +Description:
> + This file contains the total amount of time this wakeup source
> + has been active, in milliseconds.
> +
> +What: /sys/class/wakeup/.../max_time_ms
> +Date: June 2019
> +Contact: Tri Vo <trong@android.com>
> +Description:
> + This file contains the maximum amount of time this wakeup
> + source has been continuously active, in milliseconds.
> +
> +What: /sys/class/wakeup/.../last_change_ms
> +Date: June 2019
> +Contact: Tri Vo <trong@android.com>
> +Description:
> + This file contains the monotonic clock time when the wakeup
> + source was touched last time, in milliseconds.
> +
> +What: /sys/class/wakeup/.../prevent_suspend_time_ms
> +Date: June 2019
> +Contact: Tri Vo <trong@android.com>
> +Description:
> + The file contains the total amount of time this wakeup source
> + has been preventing autosleep, in milliseconds.
> diff --git a/drivers/base/power/Makefile b/drivers/base/power/Makefile
> index e1bb691cf8f1..1963f53d9982 100644
> --- a/drivers/base/power/Makefile
> +++ b/drivers/base/power/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0
> obj-$(CONFIG_PM) += sysfs.o generic_ops.o common.o qos.o runtime.o wakeirq.o
> obj-$(CONFIG_PM_SLEEP) += main.o wakeup.o
> +obj-$(CONFIG_PM_SLEEP_STATS) += wakeup_stats.o
> obj-$(CONFIG_PM_TRACE_RTC) += trace.o
> obj-$(CONFIG_PM_GENERIC_DOMAINS) += domain.o domain_governor.o
> obj-$(CONFIG_HAVE_CLK) += clock_ops.o
> diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
> index 5b2b6a05a4f3..fd48e78c06b9 100644
> --- a/drivers/base/power/wakeup.c
> +++ b/drivers/base/power/wakeup.c
> @@ -205,11 +205,18 @@ EXPORT_SYMBOL_GPL(wakeup_source_remove);
> struct wakeup_source *wakeup_source_register(const char *name)
> {
> struct wakeup_source *ws;
> + int ret;
>
> ws = wakeup_source_create(name);
> - if (ws)
> + if (ws) {
> + ret = wakeup_source_sysfs_add(ws);
> + if (ret) {
> + kfree_const(ws->name);
> + kfree(ws);
> + return NULL;
> + }
> wakeup_source_add(ws);
> -
> + }
> return ws;
> }
> EXPORT_SYMBOL_GPL(wakeup_source_register);
> @@ -222,6 +229,7 @@ void wakeup_source_unregister(struct wakeup_source *ws)
> {
> if (ws) {
> wakeup_source_remove(ws);
> + wakeup_source_sysfs_remove(ws);
> wakeup_source_destroy(ws);
> }
> }
> diff --git a/drivers/base/power/wakeup_stats.c b/drivers/base/power/wakeup_stats.c
> new file mode 100644
> index 000000000000..45e15856105b
> --- /dev/null
> +++ b/drivers/base/power/wakeup_stats.c
> @@ -0,0 +1,149 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Wakeup statistics in sysfs
> + *
> + * Copyright (c) 2019 Linux Foundation
> + * Copyright (c) 2019 Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> + * Copyright (c) 2019 Google Inc.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kdev_t.h>
> +
> +#include "power.h"
> +
> +static struct class *wakeup_class;
> +
> +#define wakeup_attr(_name) \
> +static ssize_t _name##_show(struct device *dev, \
> + struct device_attribute *attr, char *buf) \
> +{ \
> + struct wakeup_source *ws = dev_get_drvdata(dev); \
> + \
> + return sprintf(buf, "%lu\n", ws->_name); \
> +} \
> +static DEVICE_ATTR_RO(_name)
> +
> +wakeup_attr(active_count);
> +wakeup_attr(event_count);
> +wakeup_attr(wakeup_count);
> +wakeup_attr(expire_count);
> +
> +static ssize_t active_time_ms_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct wakeup_source *ws = dev_get_drvdata(dev);
> + ktime_t active_time =
> + ws->active ? ktime_sub(ktime_get(), ws->last_time) : 0;
> +
> + return sprintf(buf, "%lld\n", ktime_to_ms(active_time));
> +}
> +static DEVICE_ATTR_RO(active_time_ms);
> +
> +static ssize_t total_time_ms_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct wakeup_source *ws = dev_get_drvdata(dev);
> + ktime_t active_time;
> + ktime_t total_time = ws->total_time;
> +
> + if (ws->active) {
> + active_time = ktime_sub(ktime_get(), ws->last_time);
> + total_time = ktime_add(total_time, active_time);
> + }
> + return sprintf(buf, "%lld\n", ktime_to_ms(total_time));
> +}
> +static DEVICE_ATTR_RO(total_time_ms);
> +
> +static ssize_t max_time_ms_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct wakeup_source *ws = dev_get_drvdata(dev);
> + ktime_t active_time;
> + ktime_t max_time = ws->max_time;
> +
> + if (ws->active) {
> + active_time = ktime_sub(ktime_get(), ws->last_time);
> + if (active_time > max_time)
> + max_time = active_time;
> + }
> + return sprintf(buf, "%lld\n", ktime_to_ms(max_time));
> +}
> +static DEVICE_ATTR_RO(max_time_ms);
> +
> +static ssize_t last_change_ms_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct wakeup_source *ws = dev_get_drvdata(dev);
> +
> + return sprintf(buf, "%lld\n", ktime_to_ms(ws->last_time));
> +}
> +static DEVICE_ATTR_RO(last_change_ms);
> +
> +static ssize_t prevent_suspend_time_ms_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct wakeup_source *ws = dev_get_drvdata(dev);
> + ktime_t prevent_sleep_time = ws->prevent_sleep_time;
> +
> + if (ws->active && ws->autosleep_enabled) {
> + prevent_sleep_time = ktime_add(prevent_sleep_time,
> + ktime_sub(ktime_get(), ws->start_prevent_time));
> + }
> + return sprintf(buf, "%lld\n", ktime_to_ms(prevent_sleep_time));
> +}
> +static DEVICE_ATTR_RO(prevent_suspend_time_ms);
> +
> +static struct attribute *wakeup_source_attrs[] = {
> + &dev_attr_active_count.attr,
> + &dev_attr_event_count.attr,
> + &dev_attr_wakeup_count.attr,
> + &dev_attr_expire_count.attr,
> + &dev_attr_active_time_ms.attr,
> + &dev_attr_total_time_ms.attr,
> + &dev_attr_max_time_ms.attr,
> + &dev_attr_last_change_ms.attr,
> + &dev_attr_prevent_suspend_time_ms.attr,
> + NULL,
> +};
> +ATTRIBUTE_GROUPS(wakeup_source);
> +
> +/**
> + * wakeup_source_sysfs_add - Add wakeup_source attributes to sysfs.
> + * @ws: Wakeup source to be added in sysfs.
> + */
> +int wakeup_source_sysfs_add(struct wakeup_source *ws)
> +{
> + struct device *dev;
> +
> + dev = device_create_with_groups(wakeup_class, NULL, MKDEV(0, 0), ws,
> + wakeup_source_groups, "%s", ws->name);
> + if (IS_ERR(dev))
> + return PTR_ERR(dev);
> +
> + ws->dev = dev;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(wakeup_source_sysfs_add);
> +
> +/**
> + * wakeup_source_sysfs_remove - Remove wakeup_source attributes from sysfs.
> + * @ws: Wakeup source to be removed from sysfs.
> + */
> +void wakeup_source_sysfs_remove(struct wakeup_source *ws)
> +{
> + device_unregister(ws->dev);
> +}
> +EXPORT_SYMBOL_GPL(wakeup_source_sysfs_remove);
> +
> +static int __init wakeup_sources_sysfs_init(void)
> +{
> + wakeup_class = class_create(THIS_MODULE, "wakeup");
> + if (IS_ERR(wakeup_class))
> + return PTR_ERR(wakeup_class);
> +
> + return 0;
> +}
> +
> +postcore_initcall(wakeup_sources_sysfs_init);
> diff --git a/include/linux/pm_wakeup.h b/include/linux/pm_wakeup.h
> index ce57771fab9b..734141869c17 100644
> --- a/include/linux/pm_wakeup.h
> +++ b/include/linux/pm_wakeup.h
> @@ -35,6 +35,7 @@ struct wake_irq;
> * @relax_count: Number of times the wakeup source was deactivated.
> * @expire_count: Number of times the wakeup source's timeout has expired.
> * @wakeup_count: Number of times the wakeup source might abort suspend.
> + * @dev: Struct device for sysfs statistics about the wakeup source.
> * @active: Status of the wakeup source.
> * @has_timeout: The wakeup source has been activated with a timeout.
> */
> @@ -55,6 +56,7 @@ struct wakeup_source {
> unsigned long relax_count;
> unsigned long expire_count;
> unsigned long wakeup_count;
> + struct device *dev;
> bool active:1;
> bool autosleep_enabled:1;
> };
> @@ -181,6 +183,23 @@ static inline void pm_wakeup_dev_event(struct device *dev, unsigned int msec,
>
> #endif /* !CONFIG_PM_SLEEP */
>
> +#ifdef CONFIG_PM_SLEEP_STATS
> +
> +/* drivers/base/power/wakeup_stats.c */
> +int wakeup_source_sysfs_add(struct wakeup_source *ws);
> +void wakeup_source_sysfs_remove(struct wakeup_source *ws);
> +
> +#else /* !CONFIG_PM_SLEEP_STATS */
> +
> +static inline int wakeup_source_sysfs_add(struct wakeup_source *ws)
> +{
> + return 0;
> +}
> +static inline void wakeup_source_sysfs_remove(struct wakeup_source *ws)
> +{ }
> +
> +#endif /* !CONFIG_PM_SLEEP_STATS */
> +
> static inline void wakeup_source_init(struct wakeup_source *ws,
> const char *name)
> {
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index ff8592ddedee..604e1f087f14 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -195,6 +195,16 @@ config PM_SLEEP_DEBUG
> def_bool y
> depends on PM_DEBUG && PM_SLEEP
>
> +config PM_SLEEP_STATS
> + bool "Wakeup sources statistics"
> + depends on PM_SLEEP
> + help
> + Expose wakeup sources statistics to user space via sysfs. Collected
> + statistics are located under /sys/power/wakeup_sources. For more
> + information, read <file:Documentation/ABI/testing/sysfs-class-wakeup>.
> +
> + If in doubt, say N.
> +
> config DPM_WATCHDOG
> bool "Device suspend/resume watchdog"
> depends on PM_DEBUG && PSTORE && EXPERT
> diff --git a/kernel/power/wakelock.c b/kernel/power/wakelock.c
> index 4210152e56f0..32726da3d6e6 100644
> --- a/kernel/power/wakelock.c
> +++ b/kernel/power/wakelock.c
> @@ -122,6 +122,7 @@ static void __wakelocks_gc(struct work_struct *work)
>
> if (!active) {
> wakeup_source_remove(&wl->ws);
> + wakeup_source_sysfs_remove(&wl->ws);
> rb_erase(&wl->node, &wakelocks_tree);
> list_del(&wl->lru);
> kfree(wl->name);
> @@ -153,6 +154,7 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
> struct rb_node **node = &wakelocks_tree.rb_node;
> struct rb_node *parent = *node;
> struct wakelock *wl;
> + int ret;
>
> while (*node) {
> int diff;
> @@ -189,6 +191,14 @@ static struct wakelock *wakelock_lookup_add(const char *name, size_t len,
> }
> wl->ws.name = wl->name;
> wl->ws.last_time = ktime_get();
> +
> + ret = wakeup_source_sysfs_add(&wl->ws);
> + if (ret) {
> + kfree(wl->name);
> + kfree(wl);
> + return ERR_PTR(ret);
> + }
> +
> wakeup_source_add(&wl->ws);
> rb_link_node(&wl->node, parent, node);
> rb_insert_color(&wl->node, &wakelocks_tree);
> --
> 2.22.0.510.g264f2c817a-goog
>
^ permalink raw reply
* Re: [PATCH v2 2/2] interconnect: qcom: Add tagging and wake/sleep support for sdm845
From: David Dai @ 2019-07-15 23:34 UTC (permalink / raw)
To: Evan Green, Georgi Djakov
Cc: linux-pm, Vincent Guittot, Bjorn Andersson, amit.kucheria,
Doug Anderson, Sean Sweeney, LKML, linux-arm Mailing List,
linux-arm-msm
In-Reply-To: <CAE=gft7=ZbK3ARtWyv8n_hWJ4kuXRs0UA0QsE420pqL8R1quUQ@mail.gmail.com>
Hi Evan,
Thanks for the continued help in reviewing these patches!
On 7/11/2019 10:06 AM, Evan Green wrote:
> Hi Georgi and David,
>
> On Tue, Jun 18, 2019 at 2:17 AM Georgi Djakov <georgi.djakov@linaro.org> wrote:
>> From: David Dai <daidavid1@codeaurora.org>
>>
>> Add support for wake and sleep commands by using a tag to indicate
>> whether or not the aggregate and set requests fall into execution
>> state specific bucket.
>>
>> Signed-off-by: David Dai <daidavid1@codeaurora.org>
>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org>
>> ---
>> drivers/interconnect/qcom/sdm845.c | 129 ++++++++++++++++++++++-------
>> 1 file changed, 98 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/interconnect/qcom/sdm845.c b/drivers/interconnect/qcom/sdm845.c
>> index fb526004c82e..c100aab39415 100644
>> --- a/drivers/interconnect/qcom/sdm845.c
>> +++ b/drivers/interconnect/qcom/sdm845.c
>> @@ -66,6 +66,17 @@ struct bcm_db {
>> #define SDM845_MAX_BCM_PER_NODE 2
>> #define SDM845_MAX_VCD 10
>>
>> +#define QCOM_ICC_BUCKET_AMC 0
> What is AMC again? Is it the "right now" bucket? Maybe a comment on
> the meaning of this bucket would be helpful.
That's correct. Will add a comment for this.
>
>> +#define QCOM_ICC_BUCKET_WAKE 1
>> +#define QCOM_ICC_BUCKET_SLEEP 2
>> +#define QCOM_ICC_NUM_BUCKETS 3
>> +#define QCOM_ICC_TAG_AMC BIT(QCOM_ICC_BUCKET_AMC)
>> +#define QCOM_ICC_TAG_WAKE BIT(QCOM_ICC_BUCKET_WAKE)
>> +#define QCOM_ICC_TAG_SLEEP BIT(QCOM_ICC_BUCKET_SLEEP)
>> +#define QCOM_ICC_TAG_ACTIVE_ONLY (QCOM_ICC_TAG_AMC | QCOM_ICC_TAG_WAKE)
>> +#define QCOM_ICC_TAG_ALWAYS (QCOM_ICC_TAG_AMC | QCOM_ICC_TAG_WAKE |\
>> + QCOM_ICC_TAG_SLEEP)
>> +
>> /**
>> * struct qcom_icc_node - Qualcomm specific interconnect nodes
>> * @name: the node name used in debugfs
>> @@ -75,7 +86,9 @@ struct bcm_db {
>> * @channels: num of channels at this node
>> * @buswidth: width of the interconnect between a node and the bus
>> * @sum_avg: current sum aggregate value of all avg bw requests
>> + * @sum_avg_cached: previous sum aggregate value of all avg bw requests
>> * @max_peak: current max aggregate value of all peak bw requests
>> + * @max_peak_cached: previous max aggregate value of all peak bw requests
>> * @bcms: list of bcms associated with this logical node
>> * @num_bcms: num of @bcms
>> */
>> @@ -86,8 +99,10 @@ struct qcom_icc_node {
>> u16 num_links;
>> u16 channels;
>> u16 buswidth;
>> - u64 sum_avg;
>> - u64 max_peak;
>> + u64 sum_avg[QCOM_ICC_NUM_BUCKETS];
>> + u64 sum_avg_cached[QCOM_ICC_NUM_BUCKETS];
>> + u64 max_peak[QCOM_ICC_NUM_BUCKETS];
>> + u64 max_peak_cached[QCOM_ICC_NUM_BUCKETS];
>> struct qcom_icc_bcm *bcms[SDM845_MAX_BCM_PER_NODE];
>> size_t num_bcms;
>> };
>> @@ -112,8 +127,8 @@ struct qcom_icc_bcm {
>> const char *name;
>> u32 type;
>> u32 addr;
>> - u64 vote_x;
>> - u64 vote_y;
>> + u64 vote_x[QCOM_ICC_NUM_BUCKETS];
>> + u64 vote_y[QCOM_ICC_NUM_BUCKETS];
>> bool dirty;
>> bool keepalive;
>> struct bcm_db aux_data;
>> @@ -555,7 +570,7 @@ inline void tcs_cmd_gen(struct tcs_cmd *cmd, u64 vote_x, u64 vote_y,
>> cmd->wait = true;
>> }
>>
>> -static void tcs_list_gen(struct list_head *bcm_list,
>> +static void tcs_list_gen(struct list_head *bcm_list, int bucket,
>> struct tcs_cmd tcs_list[SDM845_MAX_VCD],
>> int n[SDM845_MAX_VCD])
>> {
>> @@ -573,8 +588,8 @@ static void tcs_list_gen(struct list_head *bcm_list,
>> commit = true;
>> cur_vcd_size = 0;
>> }
>> - tcs_cmd_gen(&tcs_list[idx], bcm->vote_x, bcm->vote_y,
>> - bcm->addr, commit);
>> + tcs_cmd_gen(&tcs_list[idx], bcm->vote_x[bucket],
>> + bcm->vote_y[bucket], bcm->addr, commit);
>> idx++;
>> n[batch]++;
>> /*
>> @@ -595,32 +610,39 @@ static void tcs_list_gen(struct list_head *bcm_list,
>>
>> static void bcm_aggregate(struct qcom_icc_bcm *bcm)
>> {
>> - size_t i;
>> - u64 agg_avg = 0;
>> - u64 agg_peak = 0;
>> + size_t i, bucket;
>> + u64 agg_avg[QCOM_ICC_NUM_BUCKETS] = {0};
>> + u64 agg_peak[QCOM_ICC_NUM_BUCKETS] = {0};
>> u64 temp;
>>
>> - for (i = 0; i < bcm->num_nodes; i++) {
>> - temp = bcm->nodes[i]->sum_avg * bcm->aux_data.width;
>> - do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
>> - agg_avg = max(agg_avg, temp);
>> + for (bucket = 0; bucket < QCOM_ICC_NUM_BUCKETS; bucket++) {
>> + for (i = 0; i < bcm->num_nodes; i++) {
>> + temp = bcm->nodes[i]->sum_avg_cached[bucket] * bcm->aux_data.width;
>> + do_div(temp, bcm->nodes[i]->buswidth * bcm->nodes[i]->channels);
>> + agg_avg[bucket] = max(agg_avg[bucket], temp);
>>
>> - temp = bcm->nodes[i]->max_peak * bcm->aux_data.width;
>> - do_div(temp, bcm->nodes[i]->buswidth);
> Why is it that this one doesn't have the multiply by
> bcm->nodes[i]->channels again? I can't recall if there was a reason.
> If it's correct maybe it deserves a comment.
I think the rationale behind this is generally for consumers to target a
certain minimum threshold to satisfy some structural latency
requirements as opposed to strictly throughput, and it may be easier for
consumers to reuse certain values to support hitting some minimum NoC
frequencies without having to be concerned with the number of channels
that may change from platform to platform.
>
>> - agg_peak = max(agg_peak, temp);
>> - }
>> + temp = bcm->nodes[i]->max_peak_cached[bucket] * bcm->aux_data.width;
>> + do_div(temp, bcm->nodes[i]->buswidth);
>> + agg_peak[bucket] = max(agg_peak[bucket], temp);
>>
>> - temp = agg_avg * 1000ULL;
>> - do_div(temp, bcm->aux_data.unit);
>> - bcm->vote_x = temp;
>> + bcm->nodes[i]->sum_avg[bucket] = 0;
>> + bcm->nodes[i]->max_peak[bucket] = 0;
> I don't understand the sum_avg vs sum_avg_cached. Here's what I understand:
> 1. qcom_icc_aggregate() does the math from the incoming values on
> sum_avg, and then clobbers sum_avg_cached with those values.
> 2. bcm_aggregate() uses sum_avg_cached in its calculations, then clears sum_avg.
>
> But I don't get why that's needed. Why not just have sum_avg? Wouldn't
> it work the same? Ok, it wouldn't if you ended up calling
> bcm_aggregate() multiple times on the same bcm. But you have a dirty
> flag that prevents this from happening. So I think it's safe to remove
> the cached arrays, and just clear out the sum_avg when you aggregate.
You are correct in that the dirty flag would prevent another repeat of
the bcm_aggregate() call in the same icc_set request. But consider a
following icc_set request on a different node that shares the same BCM,
the next bcm_aggregate() would result in an incorrect aggregate sum_avg
for the BCM since the avg_sum from the previous node(from the previous
icc_set) was cleared out. We need a way to retain the current state of
all nodes to accurately aggregate the bw values for the BCM.
>> + }
>>
>> - temp = agg_peak * 1000ULL;
>> - do_div(temp, bcm->aux_data.unit);
>> - bcm->vote_y = temp;
>> + temp = agg_avg[bucket] * 1000ULL;
>> + do_div(temp, bcm->aux_data.unit);
>> + bcm->vote_x[bucket] = temp;
>>
>> - if (bcm->keepalive && bcm->vote_x == 0 && bcm->vote_y == 0) {
>> - bcm->vote_x = 1;
>> - bcm->vote_y = 1;
>> + temp = agg_peak[bucket] * 1000ULL;
>> + do_div(temp, bcm->aux_data.unit);
>> + bcm->vote_y[bucket] = temp;
>> + }
>> +
>> + if (bcm->keepalive && bcm->vote_x[0] == 0 && bcm->vote_y[0] == 0) {
>> + bcm->vote_x[QCOM_ICC_BUCKET_AMC] = 1;
>> + bcm->vote_x[QCOM_ICC_BUCKET_WAKE] = 1;
>> + bcm->vote_y[QCOM_ICC_BUCKET_AMC] = 1;
>> + bcm->vote_y[QCOM_ICC_BUCKET_WAKE] = 1;
>> }
>>
>> bcm->dirty = false;
>> @@ -631,15 +653,25 @@ static int qcom_icc_aggregate(struct icc_node *node, u32 tag, u32 avg_bw,
>> {
>> size_t i;
>> struct qcom_icc_node *qn;
>> + unsigned long tag_word = (unsigned long)tag;
>>
>> qn = node->data;
>>
>> + if (!tag)
>> + tag_word = QCOM_ICC_TAG_ALWAYS;
>> +
>> + for (i = 0; i < QCOM_ICC_NUM_BUCKETS; i++) {
>> + if (test_bit(i, &tag_word)) {
> I guess all this extra business with tag_word and casting is so that
> you can use test_bit, which is presumably a tiny bit faster? Does this
> actually make a measurable difference? Maybe in the name of simplicity
> we just do if (tag & BIT(i)), and then optimize if we find that
> conditional to be a hotspot?
Using (tag & BIT(i)) as opposed to test_bit seems reasonable to me.
>> + qn->sum_avg[i] += avg_bw;
>> + qn->max_peak[i] = max_t(u32, qn->max_peak[i], peak_bw);
>> + qn->sum_avg_cached[i] = qn->sum_avg[i];
>> + qn->max_peak_cached[i] = qn->max_peak[i];
>> + }
>> + }
>> +
>> *agg_avg += avg_bw;
>> *agg_peak = max_t(u32, *agg_peak, peak_bw);
>>
>> - qn->sum_avg = *agg_avg;
>> - qn->max_peak = *agg_peak;
>> -
>> for (i = 0; i < qn->num_bcms; i++)
>> qn->bcms[i]->dirty = true;
>>
>> @@ -675,7 +707,7 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>> * Construct the command list based on a pre ordered list of BCMs
>> * based on VCD.
>> */
>> - tcs_list_gen(&commit_list, cmds, commit_idx);
>> + tcs_list_gen(&commit_list, QCOM_ICC_BUCKET_AMC, cmds, commit_idx);
>>
>> if (!commit_idx[0])
>> return ret;
>> @@ -693,6 +725,41 @@ static int qcom_icc_set(struct icc_node *src, struct icc_node *dst)
>> return ret;
>> }
>>
>> + INIT_LIST_HEAD(&commit_list);
>> +
>> + for (i = 0; i < qp->num_bcms; i++) {
>> + /*
>> + * Only generate WAKE and SLEEP commands if a resource's
>> + * requirements change as the execution environment transitions
>> + * between different power states.
>> + */
>> + if (qp->bcms[i]->vote_x[QCOM_ICC_BUCKET_WAKE] !=
>> + qp->bcms[i]->vote_x[QCOM_ICC_BUCKET_SLEEP] ||
>> + qp->bcms[i]->vote_y[QCOM_ICC_BUCKET_WAKE] !=
>> + qp->bcms[i]->vote_y[QCOM_ICC_BUCKET_SLEEP]) {
>> + list_add_tail(&qp->bcms[i]->list, &commit_list);
>> + }
>> + }
>> +
>> + if (list_empty(&commit_list))
>> + return ret;
>> +
>> + tcs_list_gen(&commit_list, QCOM_ICC_BUCKET_WAKE, cmds, commit_idx);
>> +
>> + ret = rpmh_write_batch(qp->dev, RPMH_WAKE_ONLY_STATE, cmds, commit_idx);
>> + if (ret) {
>> + pr_err("Error sending WAKE RPMH requests (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + tcs_list_gen(&commit_list, QCOM_ICC_BUCKET_SLEEP, cmds, commit_idx);
>> +
>> + ret = rpmh_write_batch(qp->dev, RPMH_SLEEP_STATE, cmds, commit_idx);
>> + if (ret) {
>> + pr_err("Error sending SLEEP RPMH requests (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> return ret;
>> }
>>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [PATCH v3 6/6] interconnect: Add OPP table support for interconnects
From: Saravana Kannan @ 2019-07-16 0:55 UTC (permalink / raw)
To: Vincent Guittot
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,
Android Kernel Team, open list:THERMAL,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel
In-Reply-To: <CAKfTPtBE7e+hc55TY43JC0XPONvrS4FBPkZcRZ4EbzyCJKNhfg@mail.gmail.com>
On Mon, Jul 15, 2019 at 1:16 AM Vincent Guittot
<vincent.guittot@linaro.org> wrote:
>
> On Tue, 9 Jul 2019 at 21:03, Saravana Kannan <saravanak@google.com> wrote:
> >
> > On Tue, Jul 9, 2019 at 12:25 AM Vincent Guittot
> > <vincent.guittot@linaro.org> wrote:
> > >
> > > On Sun, 7 Jul 2019 at 23:48, Saravana Kannan <saravanak@google.com> wrote:
> > > >
> > > > On Thu, Jul 4, 2019 at 12:12 AM Vincent Guittot
> > > > <vincent.guittot@linaro.org> wrote:
> > > > >
> > > > > On Wed, 3 Jul 2019 at 23:33, Saravana Kannan <saravanak@google.com> wrote:
> > > > > >
> > > > > > On Tue, Jul 2, 2019 at 11:45 PM Vincent Guittot
> > > > > > <vincent.guittot@linaro.org> wrote:
> > > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > The aggregation of OPP tables of the providers is certainly the
> > > > > > superset of what a path can achieve, but to say that OPPs for
> > > > > > interconnect path should match that superset is an oversimplification
> > > > > > of the reality in hardware.
> > > > > >
> > > > > > There are lots of reasons an interconnect path might not want to use
> > > > > > all the available bandwidth options across all the interconnects in
> > > > > > the route.
> > > > > >
> > > > > > 1. That particular path might not have been validated or verified
> > > > > > during the HW design process for some of the frequencies/bandwidth
> > > > > > combinations of the providers.
> > > > >
> > > > > All these constraint are provider's constraints and not consumer's one
> > > > >
> > > > > The consumer asks for a bandwidth according to its needs and then the
> > > > > providers select the optimal bandwidth of each interconnect after
> > > > > aggregating all the request and according to what OPP have been
> > > > > validated
> > > >
> > > > Not really. The screening can be a consumer specific issue. The
> > > > consumer IP itself might have some issue with using too low of a
> > > > bandwidth or bandwidth that's not within some range. It should not be
> > >
> > > How can an IP ask for not enough bandwidth ?
> > > It asks the needed bandwidth based on its requirements
> >
> > The "enough bandwidth" is not always obvious. It's only for very
> > simple cases that you can calculate the required bandwidth. Even for
> > cases that you think might be "obvious/easy" aren't always easy.
> >
> > For example, you'd think a display IP would have a fixed bandwidth
> > requirement for a fixed resolution screen. But that's far from the
> > truth. It can also change as the number of layers change per frame.
> > For video decoder/encoder, it depends on how well the frames compress
> > with a specific compression scheme.
> > So the "required" bandwidth is often a heuristic based on the IP
> > frequency or traffic measurement.
> >
> > But that's not even the point I was making in this specific "bullet".
> >
> > A hardware IP might be screen/verified with only certain bandwidth
> > levels. Or it might have hardware bugs that prevent it from using
> > lower bandwidths even though it's technically sufficient. We need a
> > way to capture that per path. This is not even a fictional case. This
> > has been true multiple times over widely used IPs.
>
> here you are mixing HW constraint on the soc and OPP screening with
> bandwidth request from consumer
> ICC framework is about getting bandwidth request not trying to fix
> some HW/voltage dependency of the SoC
>
> >
> > > > the provider's job to take into account all the IP that might be
> > > > connected to the interconnects. If the interconnect HW itself didn't
> > >
> > > That's not what I'm saying. The provider knows which bandwidth the
> > > interconnect can provide as it is the ones which configures it. So if
> > > the interconnect has a finite number of bandwidth point based probably
> > > on the possible clock frequency and others config of the interconnect,
> > > it selects the best final config after aggregating the request of the
> > > consumer.
> >
> > I completely agree with this. What you are stating above is how it
> > should work and that's the whole point of the interconnect framework.
> >
> > But this is orthogonal to the point I'm making.
>
> It's not orthogonal because you want to add a OPP table pointer in the
> ICC path structure to fix your platform HW constraint whereas it's not
> the purpose of the framework IMO
>
> >
> > > > change, the provider driver shouldn't need to change. By your
> > > > definition, a provider driver will have to account for all the
> > > > possible bus masters that might be connected to it across all SoCs.
> > >
> > > you didn't catch my point
> >
> > Same. I think we are talking over each other. Let me try again.
> >
> > You are trying to describe how and interconnect provider and framework
> > should work. There's no disagreement there.
> >
> > My point is that consumers might not want to or can not always use all
> > the available bandwidth levels offered by the providers. There can be
> > many reasons for that (which is what I listed in my earlier emails)
> > and we need a good and generic way to capture that so that everyone
> > isn't trying to invent their own property.
>
> And my point is that you want to describe some platform or even UCs
> specific constraint in the ICC framework which is not the place to do.
>
> If the consumers might not want to use all available bandwidth because
> this is not power efficient as an example, this should be describe
> somewhere else to express that there is a shared power domain
> between some devices and we shoudl ensure that all devices in this
> power domain should use the Optimal Operating Point (optimal freq for
> a voltage)
My patch series has nothing to do with shared power domains. I think
the examples have made it amply clear.
> ICC framework describes the bandwidth request that are expressed by
> the consumers for the current running state of their IP but it doesn't
> reflect the fact that on platform A, the consumer should use bandwidth
> X because it will select a voltage level of a shared power domain that
> is optimized for the other devices B, C ... . It's up to the provider
> to know HW details of the bus that it drives and to make such
> decision; the consumer should always request the same
The change to ICC framework is practically just this. I don't have any
future changes planned for the ICC framework. This is the entirety of
it.
+ 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);
+ }
It's quite a stretch and bit hyperbolic to say this one change is
getting ICC framework to do all the things you claim above.
It's literally a simple helper function so that the consumer doesn't
have to make assumptions about indices and it's a bit more explicit
about which OPP table of the device (a device can have multiple OPP
tables) corresponds to which ICC path.
Going by your extreme argument, one can also claim that it's not the
ICC framework's job to make it easy for consumers to figure out the
source/destination endpoints or give them names and delete the
interconnect and interconnect-names properties. That's clearly just as
absurd a claim.
-Saravana
> > > > That's not good design nor is it scalable.
> > > >
> > > > > >
> > > > > > 2. Similarly during parts screening in the factory, some of the
> > > > > > combinations might not have been screened and can't be guaranteed
> > > > > > to work.
> > > > >
> > > > > As above, it's the provider's job to select the final bandwidth
> > > > > according to its constraint
> > > >
> > > > Same reply as above.
> > > >
> > > > > >
> > > > > > 3. Only a certain set of bandwidth levels might make sense to use from
> > > > > > a power/performance balance given the device using it. For example:
> > > > > > - The big CPU might not want to use some of the lower bandwidths
> > > > > > but the little CPU might want to.
> > > > > > - The big CPU might not want to use some intermediate bandwidth
> > > > > > points if they don't save a lot of power compared to a higher
> > > > > > bandwidth levels, but the little CPU might want to.
> > > > > > - The little CPU might never want to use the higher set of
> > > > > > bandwidth levels since they won't be power efficient for the use
> > > > > > cases that might run on it.
> > > > >
> > > > > These example are quite vague about the reasons why little might never
> > > > > want to use higher bandwidth.
> > > >
> > > > How is it vague? I just said because of power/performance balance.
> > > >
> > > > > But then, if little doesn't ask high bandwidth it will not use them.
> > > >
> > > > If you are running a heuristics based algorithm to pick bandwidth,
> > > > this is how it'll know NOT to use some of the bandwidth levels.
> > >
> > > so you want to set a bandwidth according to the cpu frequency which is
> > > what has been proposed in other thread
> >
> > Nope, that's just one heuristic. Often times it's based on hardware
> > monitors measuring interconnect activity. If you go look at the SDM845
> > in a Pixel 3, almost nothing is directly tied to the CPU frequency.
> >
> > Even if you are scaling bandwidth based on other hardware
> > measurements, you might want to avoid some bandwidth level provided by
> > the interconnect providers because it's suboptimal.
> >
> > For example, when making bandwidth votes to accommodate the big CPUs,
> > you might never want to use some of the lower bandwidth levels because
> > they are not power efficient for any CPU frequency or any bandwidth
> > level. Because at those levels the memory/interconnect is so slow that
> > it has a non-trivial utilization increase (because the CPU is
> > stalling) of the big CPUs.
> >
> > Again, this is completely different from what the providers/icc
> > framework does. Which is, once the request is made, they aggregate and
> > set the actual interconnect frequencies correctly.
> >
> > > >
> > > > > >
> > > > > > 4. It might not make sense from a system level power perspective.
> > > > > > Let's take an example of a path S (source) -> A -> B -> C -> D
> > > > > > (destination).
> > > > > > - A supports only 2, 5, 7 and 10 GB/s. B supports 1, 2 ... 10 GB/s.
> > > > > > C supports 5 and 10 GB/s
> > > > > > - If you combine and list the superset of bandwidth levels
> > > > > > supported in that path, that'd be 1, 2, 3, ... 10 GB/s.
> > > > > > - Which set of bandwidth levels make sense will depend on the
> > > > > > hardware characteristics of the interconnects.
> > > > > > - If B is the biggest power sink, then you might want to use all 10
> > > > > > levels.
> > > > > > - If A is the biggest power sink, then you might want to use all 2,
> > > > > > 5 and 10 GB/s of the levels.
> > > > > > - If C is the biggest power sink then you might only want to use 5
> > > > > > and 10 GB/s
> > > > > > - The more hops and paths you get the more convoluted this gets.
> > > > > >
> > > > > > 5. The design of the interconnects themselves might have an impact on
> > > > > > which bandwidth levels are used.
> > > > > > - For example, the FIFO depth between two specific interconnects
> > > > > > might affect the valid bandwidth levels for a specific path.
> > > > > > - Say S1 -> A -> B -> D1, S2 -> C -> B -> D1 and S2 -> C -> D2 are
> > > > > > three paths.
> > > > > > - If C <-> B FIFO depth is small, then there might be a requirement
> > > > > > that C and B be closely performance matched to avoid system level
> > > > > > congestion due to back pressure.
> > > > > > - So S2 -> D1 path can't use all the bandwidth levels supported by
> > > > > > C-B combination.
> > > > > > - But S2 -> D2 can use all the bandwidth levels supported by C.
> > > > > > - And S1 -> D1 can use all the levels supported by A-B combination.
> > > > > >
> > > > >
> > > > > All the examples above makes sense but have to be handle by the
> > > > > provider not the consumer. The consumer asks for a bandwidth according
> > > > > to its constraints. Then the provider which is the driver that manages
> > > > > the interconnect IP, should manage all this hardware and platform
> > > > > specific stuff related to the interconnect IP in order to set the
> > > > > optimal bandwidth that fit both consumer constraint and platform
> > > > > specific configuration.
> > > >
> > > > Sure, but the provider itself can have interconnect properties to
> > > > indicate which other interconnects it's tied to. And the provider will
> > > > still need the interconnect-opp-table to denote which bandwidth levels
> > > > are sensible to use with each of its connections.
> >
> > You seem to have missed this comment.
> >
> > Thanks,
> > Saravana
> >
> > > > So in some instances the interconnect-opp-table covers the needs of
> > > > purely consumers and in some instances purely providers. But in either
> > > > case, it's still needed to describe the hardware properly.
> > > >
> > > > -Saravana
> > > >
> > > > > > These are just some of the reasons I could recollect in a few minutes.
> > > > > > These are all real world cases I had to deal with in the past several
> > > > > > years of dealing with scaling interconnects. I'm sure vendors and SoCs
> > > > > > I'm not familiar with have other good reasons I'm not aware of.
> > > > > >
> > > > > > Trying to figure this all out by aggregating OPP tables of
> > > > > > interconnect providers just isn't feasible nor is it efficient. The
> > > > > > OPP tables for an interconnect path is describing the valid BW levels
> > > > > > supported by that path and verified in hardware and makes a lot of
> > > > > > sense to capture it clearly in DT.
> > > > > >
> > > > > > > So such kind of OPP table should be at
> > > > > > > provider level but not at path level.
> > > > > >
> > > > > > They can also use it if they want to, but they'll probably want to use
> > > > > > a frequency OPP table.
> > > > > >
> > > > > >
> > > > > > -Saravana
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > 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
> > > > > > > >
> > > > >
> > > > > --
> > > > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.
> > > > >
^ permalink raw reply
* Re: [PATCH v4] PM / wakeup: show wakeup sources stats in sysfs
From: Greg Kroah-Hartman @ 2019-07-16 2:11 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tri Vo, Rafael J. Wysocki, Viresh Kumar, Hridya Valsaraju,
sspatil, kaleshsingh, Linux Kernel Mailing List, Linux PM,
Cc: Android Kernel
In-Reply-To: <CAJZ5v0gEzZkbeLFtW5yadwxBryvL3vWcUoQTkUy3VkxiTV+UrA@mail.gmail.com>
On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote:
> On Mon, Jul 15, 2019 at 11:44 PM Tri Vo <trong@android.com> wrote:
> >
> > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > blocking wakeup sources over device's boot cycle. This information can
> > then be used (1) for power-specific bug reporting and (2) towards
> > attributing battery consumption to specific processes over a period of
> > time.
> >
> > However, debugfs doesn't have stable ABI. For this reason, create a
> > 'struct device' to expose wakeup sources statistics in sysfs under
> > /sys/class/wakeup/<name>/.
> >
> > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > source statistics in sysfs.
>
> I'm not sure if this is really needed, but I'll let Greg decide.
You are right. Having zillions of config options is a pain, who is
going to turn this off?
But we can always remove the option before 5.4-rc1, so I'll take this
as-is for now :)
> Apart from this
>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
thanks for the review! I'll wait for 5.3-rc1 to come out before adding
this to my tree.
greg k-h
^ permalink raw reply
* Re: [PATCH v3] PM / wakeup: show wakeup sources stats in sysfs
From: Greg KH @ 2019-07-16 2:12 UTC (permalink / raw)
To: Tri Vo
Cc: Rafael J. Wysocki, Viresh Kumar, Rafael J. Wysocki,
Hridya Valsaraju, Sandeep Patil, kaleshsingh, LKML, Linux PM,
Cc: Android Kernel
In-Reply-To: <CANA+-vC+_Gp4RwdLH_XYDAepRONB66PdekvaCxNT3ZVGx73G6A@mail.gmail.com>
On Mon, Jul 15, 2019 at 02:48:13PM -0700, Tri Vo wrote:
> > And I am guessing that you actually tested this all out, and it works
> > for you?
>
> Yes, I played around with wakelocks to make sure that wakeup source
> stats are added/updated/removed as expected.
Great!
> > Have you changed Android userspace to use the new api with no
> > problems?
>
> Kalesh helped me test this patch (added him in Tested-by: field in
> latest patch version). We haven't tested beyond booting and manual
> inspection on android devices. Android userspace changes should be
> fairly trivial though.
Ok, I know some people wanted to do some premature optimization with the
original discussion about this, glad to see it's not needed...
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
From: Chanwoo Choi @ 2019-07-16 3:56 UTC (permalink / raw)
To: Kamil Konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <20190715120416.3561-3-k.konieczny@partner.samsung.com>
Hi Kamil,
Looks good to me. But, this patch has some issue.
I added the detailed reviews.
I recommend that you make the separate patches as following
in order to clarify the role of which apply the dev_pm_opp_* function.
First patch,
Need to consolidate the following two function into one function.
because the original exynos-bus.c has the problem that the regulator
of parent devfreq device have to be enabled before enabling the clock.
This issue did not happen because bootloader enables the bus-related
regulators before kernel booting.
- exynos_bus_parse_of()
- exynos_bus_parent_parse_of()
Second patch,
Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Reuse opp core code for setting bus clock and voltage. As a side
> effect this allow useage of coupled regulators feature (required
> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
> uses regulator_set_voltage_triplet() for setting regulator voltage
> while the old code used regulator_set_voltage_tol() with fixed
> tolerance. This patch also removes no longer needed parsing of DT
> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
> it).
>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
> drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
> 1 file changed, 66 insertions(+), 106 deletions(-)
>
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 486cc5b422f1..7fc4f76bd848 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -25,7 +25,6 @@
> #include <linux/slab.h>
>
> #define DEFAULT_SATURATION_RATIO 40
> -#define DEFAULT_VOLTAGE_TOLERANCE 2
>
> struct exynos_bus {
> struct device *dev;
> @@ -37,9 +36,9 @@ struct exynos_bus {
>
> unsigned long curr_freq;
>
> - struct regulator *regulator;
> + struct opp_table *opp_table;
> +
> struct clk *clk;
> - unsigned int voltage_tolerance;
> unsigned int ratio;
> };
>
> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
> {
> struct exynos_bus *bus = dev_get_drvdata(dev);
> struct dev_pm_opp *new_opp;
> - unsigned long old_freq, new_freq, new_volt, tol;
> int ret = 0;
> -
> - /* Get new opp-bus instance according to new bus clock */
> + /*
> + * New frequency for bus may not be exactly matched to opp, adjust
> + * *freq to correct value.
> + */
You better to change this comment with following styles
to keep the consistency:
/* Get correct frequency for bus ... */
> new_opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(new_opp)) {
> dev_err(dev, "failed to get recommended opp instance\n");
> return PTR_ERR(new_opp);
> }
>
> - new_freq = dev_pm_opp_get_freq(new_opp);
> - new_volt = dev_pm_opp_get_voltage(new_opp);
> dev_pm_opp_put(new_opp);
>
> - old_freq = bus->curr_freq;
> -
> - if (old_freq == new_freq)
> - return 0;
> - tol = new_volt * bus->voltage_tolerance / 100;
> -
> /* Change voltage and frequency according to new OPP level */
> mutex_lock(&bus->lock);
> + ret = dev_pm_opp_set_rate(dev, *freq);
> + if (!ret)
> + bus->curr_freq = *freq;
Have to print the error log if ret has minus error value.
Modify it as following:
if (ret < 0) {
dev_err(dev, "failed to set bus rate\n");
goto err:
}
bus->curr_freq = *freq;
err:
mutex_unlock(&bus->lock);
return ret;
>
> - if (old_freq < new_freq) {
> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> - if (ret < 0) {
> - dev_err(bus->dev, "failed to set voltage\n");
> - goto out;
> - }
> - }
> -
> - ret = clk_set_rate(bus->clk, new_freq);
> - if (ret < 0) {
> - dev_err(dev, "failed to change clock of bus\n");
> - clk_set_rate(bus->clk, old_freq);
> - goto out;
> - }
> -
> - if (old_freq > new_freq) {
> - ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> - if (ret < 0) {
> - dev_err(bus->dev, "failed to set voltage\n");
> - goto out;
> - }
> - }
> - bus->curr_freq = new_freq;
> -
> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> - old_freq, new_freq, clk_get_rate(bus->clk));
> -out:
> mutex_unlock(&bus->lock);
>
> return ret;
> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
> if (ret < 0)
> dev_warn(dev, "failed to disable the devfreq-event devices\n");
>
> - if (bus->regulator)
> - regulator_disable(bus->regulator);
> + if (bus->opp_table)
> + dev_pm_opp_put_regulators(bus->opp_table);
Have to disable regulator after disabling the clock
to prevent the h/w fault.
I think that you should call them with following sequence:
clk_disable_unprepare(bus->clk);
if (bus->opp_table)
dev_pm_opp_put_regulators(bus->opp_table);
dev_pm_opp_of_remove_table(dev);
>
> dev_pm_opp_of_remove_table(dev);
> +
> clk_disable_unprepare(bus->clk);
> }
>
> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
> {
> struct exynos_bus *bus = dev_get_drvdata(dev);
> struct dev_pm_opp *new_opp;
> - unsigned long old_freq, new_freq;
> - int ret = 0;
> + int ret;
>
> - /* Get new opp-bus instance according to new bus clock */
> + /*
> + * New frequency for bus may not be exactly matched to opp, adjust
> + * *freq to correct value.
> + */
You better to change this comment with following styles
to keep the consistency:
/* Get correct frequency for bus ... */
> new_opp = devfreq_recommended_opp(dev, freq, flags);
> if (IS_ERR(new_opp)) {
> dev_err(dev, "failed to get recommended opp instance\n");
> return PTR_ERR(new_opp);
> }
>
> - new_freq = dev_pm_opp_get_freq(new_opp);
> dev_pm_opp_put(new_opp);
>
> - old_freq = bus->curr_freq;
> -
> - if (old_freq == new_freq)
> - return 0;
> -
> /* Change the frequency according to new OPP level */
> mutex_lock(&bus->lock);
> + ret = dev_pm_opp_set_rate(dev, *freq);
> + if (!ret)
> + bus->curr_freq = *freq;
ditto. Have to print the error log, check above comment.
>
> - ret = clk_set_rate(bus->clk, new_freq);
> - if (ret < 0) {
> - dev_err(dev, "failed to set the clock of bus\n");
> - goto out;
> - }
> -
> - *freq = new_freq;
> - bus->curr_freq = new_freq;
> -
> - dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> - old_freq, new_freq, clk_get_rate(bus->clk));
> -out:
> mutex_unlock(&bus->lock);
>
> return ret;
> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> struct exynos_bus *bus)
> {
> struct device *dev = bus->dev;
> - int i, ret, count, size;
> -
> - /* Get the regulator to provide each bus with the power */
> - bus->regulator = devm_regulator_get(dev, "vdd");
> - if (IS_ERR(bus->regulator)) {
> - dev_err(dev, "failed to get VDD regulator\n");
> - return PTR_ERR(bus->regulator);
> - }
> -
> - ret = regulator_enable(bus->regulator);
> - if (ret < 0) {
> - dev_err(dev, "failed to enable VDD regulator\n");
> - return ret;
> - }
> + int i, count, size;
>
> /*
> * Get the devfreq-event devices to get the current utilization of
> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> count = devfreq_event_get_edev_count(dev);
> if (count < 0) {
> dev_err(dev, "failed to get the count of devfreq-event dev\n");
> - ret = count;
> - goto err_regulator;
> + return count;
> }
> +
> bus->edev_count = count;
>
> size = sizeof(*bus->edev) * count;
> bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
> - if (!bus->edev) {
> - ret = -ENOMEM;
> - goto err_regulator;
> - }
> + if (!bus->edev)
> + return -ENOMEM;
>
> for (i = 0; i < count; i++) {
> bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
> - if (IS_ERR(bus->edev[i])) {
> - ret = -EPROBE_DEFER;
> - goto err_regulator;
> - }
> + if (IS_ERR(bus->edev[i]))
> + return -EPROBE_DEFER;
> }
>
> /*
> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
> if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
> bus->ratio = DEFAULT_SATURATION_RATIO;
>
> - if (of_property_read_u32(np, "exynos,voltage-tolerance",
> - &bus->voltage_tolerance))
> - bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
> -
> return 0;
> -
> -err_regulator:
> - regulator_disable(bus->regulator);
> -
> - return ret;
> }
>
> static int exynos_bus_parse_of(struct device_node *np,
> - struct exynos_bus *bus)
> + struct exynos_bus *bus, bool passive)
> {
> struct device *dev = bus->dev;
> + struct opp_table *opp_table;
> + const char *vdd = "vdd";
> struct dev_pm_opp *opp;
> unsigned long rate;
> int ret;
> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
> return ret;
> }
>
> + if (!passive) {
> + opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> + if (IS_ERR(opp_table)) {
> + ret = PTR_ERR(opp_table);
> + dev_err(dev, "failed to set regulators %d\n", ret);
> + goto err_clk;/
> + }
> +
> + bus->opp_table = opp_table;
> + }
This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
because the regulator is only used by parent devfreq device.
> +
> /* Get the freq and voltage from OPP table to scale the bus freq */
> ret = dev_pm_opp_of_add_table(dev);
> if (ret < 0) {
> dev_err(dev, "failed to get OPP table\n");
> - goto err_clk;
> + goto err_regulator;
> }
>
> rate = clk_get_rate(bus->clk);
> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
> ret = PTR_ERR(opp);
> goto err_opp;
> }
> +
> bus->curr_freq = dev_pm_opp_get_freq(opp);
> dev_pm_opp_put(opp);
>
> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>
> err_opp:
> dev_pm_opp_of_remove_table(dev);
> +
> +err_regulator:
> + if (bus->opp_table) {
> + dev_pm_opp_put_regulators(bus->opp_table);
> + bus->opp_table = NULL;
> + }
As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
after removing the opp_table by dev_pm_opp_of_remove_table().
> +
> err_clk:
> clk_disable_unprepare(bus->clk);
>
> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
> struct exynos_bus *bus;
> int ret, max_state;
> unsigned long min_freq, max_freq;
> + bool passive = false;
>
> if (!np) {
> dev_err(dev, "failed to find devicetree node\n");
> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
> bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> if (!bus)
> return -ENOMEM;
> +
> mutex_init(&bus->lock);
> bus->dev = &pdev->dev;
> platform_set_drvdata(pdev, bus);
> + node = of_parse_phandle(dev->of_node, "devfreq", 0);
> + if (node) {
> + of_node_put(node);
> + passive = true;
> + }
>
> /* Parse the device-tree to get the resource information */
> - ret = exynos_bus_parse_of(np, bus);
> + ret = exynos_bus_parse_of(np, bus, passive);
> if (ret < 0)
> return ret;
>
> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
> goto err;
> }
>
> - node = of_parse_phandle(dev->of_node, "devfreq", 0);
> - if (node) {
> - of_node_put(node);
> + if (passive)
> goto passive;
> - } else {
> - ret = exynos_bus_parent_parse_of(np, bus);
> - }
> +
> + ret = exynos_bus_parent_parse_of(np, bus);
>
Remove unneeded blank line.
> if (ret < 0)
> goto err;
> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>
> err:
> dev_pm_opp_of_remove_table(dev);
> + if (bus->opp_table) {
> + dev_pm_opp_put_regulators(bus->opp_table);
> + bus->opp_table = NULL;
> + }
> +
ditto.
Have to disable regulator after disabling the clock
to prevent the h/w fault.
I think that you should call them with following sequence:
clk_disable_unprepare(bus->clk);
if (bus->opp_table)
dev_pm_opp_put_regulators(bus->opp_table);
dev_pm_opp_of_remove_table(dev);
> clk_disable_unprepare(bus->clk);
>
> return ret;
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 1/4] opp: core: add regulators enable and disable
From: Chanwoo Choi @ 2019-07-16 4:03 UTC (permalink / raw)
To: Kamil Konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <20190715120416.3561-2-k.konieczny@partner.samsung.com>
Hi Kamil,
On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Add enable regulators to dev_pm_opp_set_regulators() and disable
> regulators to dev_pm_opp_put_regulators(). This prepares for
> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
IMHO, it is not proper to mention the specific driver name.
If you explain the reason why enable the regulator before using it,
it is enough description.
>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> --
> Changes in v2:
>
> - move regulator enable and disable into loop
>
> ---
> drivers/opp/core.c | 18 +++++++++++++++---
> 1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..069c5cf8827e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
> goto free_regulators;
> }
>
> + ret = regulator_enable(reg);
> + if (ret < 0)
> + goto disable;
> +
> opp_table->regulators[i] = reg;
> }
>
> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>
> return opp_table;
>
> +disable:
> + regulator_put(reg);
> + --i;
> +
> free_regulators:
> - while (i != 0)
> - regulator_put(opp_table->regulators[--i]);
> + for (; i >= 0; --i) {
> + regulator_disable(opp_table->regulators[i]);
> + regulator_put(opp_table->regulators[i]);
> + }
>
> kfree(opp_table->regulators);
> opp_table->regulators = NULL;
> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
> /* Make sure there are no concurrent readers while updating opp_table */
> WARN_ON(!list_empty(&opp_table->opp_list));
>
> - for (i = opp_table->regulator_count - 1; i >= 0; i--)
> + for (i = opp_table->regulator_count - 1; i >= 0; i--) {
> + regulator_disable(opp_table->regulators[i]);
> regulator_put(opp_table->regulators[i]);
> + }
>
> _free_set_opp_data(opp_table);
>
>
I agree to enable the regulator before using it.
The bootloader might not enable the regulators
and the kernel need to enable regulator in order to increase
the reference count explicitly event if bootloader enables it.
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH 7/9] x86/pci: Pass lockdep condition to pcm_mmcfg_list iterator (v1)
From: Joel Fernandes @ 2019-07-16 4:03 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: linux-kernel, Alexey Kuznetsov, Borislav Petkov, c0d1n61at3,
David S. Miller, edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI,
H. Peter Anvin, Ingo Molnar, Jonathan Corbet, Josh Triplett,
keescook, kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Oleg Nesterov, Paul E. McKenney, Pavel Machek,
peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In-Reply-To: <20190715200235.GG46935@google.com>
On Mon, Jul 15, 2019 at 03:02:35PM -0500, Bjorn Helgaas wrote:
> On Mon, Jul 15, 2019 at 10:37:03AM -0400, Joel Fernandes (Google) wrote:
> > The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
> > reader-lock held, because the pci_mmcfg_lock is already held. Make this
> > known to the list macro so that it fixes new lockdep warnings that
> > trigger due to lockdep checks added to list_for_each_entry_rcu().
> >
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>
> Ingo takes care of most patches to this file, but FWIW,
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Thanks.
> I would personally prefer if you capitalized the subject to match the
> "x86/PCI:" convention that's used fairly consistently in
> arch/x86/pci/.
>
> Also, I didn't apply this to be sure, but it looks like this might
> make a line or two wider than 80 columns, which I would rewrap if I
> were applying this.
Updated below is the patch with the nits corrected:
---8<-----------------------
From 73fab09d7e33ca2110c24215f8ed428c12625dbe Mon Sep 17 00:00:00 2001
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Date: Sat, 1 Jun 2019 15:05:49 -0400
Subject: [PATCH] x86/PCI: Pass lockdep condition to pcm_mmcfg_list iterator
(v1)
The pcm_mmcfg_list is traversed with list_for_each_entry_rcu without a
reader-lock held, because the pci_mmcfg_lock is already held. Make this
known to the list macro so that it fixes new lockdep warnings that
trigger due to lockdep checks added to list_for_each_entry_rcu().
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
arch/x86/pci/mmconfig-shared.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/arch/x86/pci/mmconfig-shared.c b/arch/x86/pci/mmconfig-shared.c
index 7389db538c30..9e3250ec5a37 100644
--- a/arch/x86/pci/mmconfig-shared.c
+++ b/arch/x86/pci/mmconfig-shared.c
@@ -29,6 +29,7 @@
static bool pci_mmcfg_running_state;
static bool pci_mmcfg_arch_init_failed;
static DEFINE_MUTEX(pci_mmcfg_lock);
+#define pci_mmcfg_lock_held() lock_is_held(&(pci_mmcfg_lock).dep_map)
LIST_HEAD(pci_mmcfg_list);
@@ -54,7 +55,8 @@ static void list_add_sorted(struct pci_mmcfg_region *new)
struct pci_mmcfg_region *cfg;
/* keep list sorted by segment and starting bus number */
- list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list) {
+ list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list,
+ pci_mmcfg_lock_held()) {
if (cfg->segment > new->segment ||
(cfg->segment == new->segment &&
cfg->start_bus >= new->start_bus)) {
@@ -118,7 +120,8 @@ struct pci_mmcfg_region *pci_mmconfig_lookup(int segment, int bus)
{
struct pci_mmcfg_region *cfg;
- list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list)
+ list_for_each_entry_rcu(cfg, &pci_mmcfg_list, list
+ pci_mmcfg_lock_held())
if (cfg->segment == segment &&
cfg->start_bus <= bus && bus <= cfg->end_bus)
return cfg;
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* [PATCH V3] cpufreq: Make cpufreq_generic_init() return void
From: Viresh Kumar @ 2019-07-16 4:06 UTC (permalink / raw)
To: Rafael Wysocki, Markus Mayer, bcm-kernel-feedback-list, Shawn Guo,
Sascha Hauer, Pengutronix Kernel Team, Fabio Estevam,
NXP Linux Team, Keguang Zhang, Jiaxun Yang, Kevin Hilman,
Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
Kukjin Kim, Krzysztof Kozlowski, Thierry Reding, Jonathan Hunter
Cc: Viresh Kumar, linux-pm, Vincent Guittot, linux-kernel,
linux-arm-kernel, linux-mips, linux-omap, linuxppc-dev,
linux-samsung-soc, linux-tegra
It always returns 0 (success) and its return type should really be void.
Over that, many drivers have added error handling code based on its
return value, which is not required at all.
change its return type to void and update all the callers.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2->V3:
- Update bmips cpufreq driver to avoid "warning: 'ret' may be used
uninitialized".
- Build bot reported this issue almost after 4 days of posting this
patch, I was expecting this a lot earlier :)
drivers/cpufreq/bmips-cpufreq.c | 17 ++++++-----------
drivers/cpufreq/cpufreq.c | 4 +---
drivers/cpufreq/davinci-cpufreq.c | 3 ++-
drivers/cpufreq/imx6q-cpufreq.c | 6 ++----
drivers/cpufreq/kirkwood-cpufreq.c | 3 ++-
drivers/cpufreq/loongson1-cpufreq.c | 8 +++-----
drivers/cpufreq/loongson2_cpufreq.c | 3 ++-
drivers/cpufreq/maple-cpufreq.c | 3 ++-
drivers/cpufreq/omap-cpufreq.c | 15 +++++----------
drivers/cpufreq/pasemi-cpufreq.c | 3 ++-
drivers/cpufreq/pmac32-cpufreq.c | 3 ++-
drivers/cpufreq/pmac64-cpufreq.c | 3 ++-
drivers/cpufreq/s3c2416-cpufreq.c | 9 ++-------
drivers/cpufreq/s3c64xx-cpufreq.c | 15 +++------------
drivers/cpufreq/s5pv210-cpufreq.c | 3 ++-
drivers/cpufreq/sa1100-cpufreq.c | 3 ++-
drivers/cpufreq/sa1110-cpufreq.c | 3 ++-
drivers/cpufreq/spear-cpufreq.c | 3 ++-
drivers/cpufreq/tegra20-cpufreq.c | 8 +-------
include/linux/cpufreq.h | 2 +-
20 files changed, 46 insertions(+), 71 deletions(-)
diff --git a/drivers/cpufreq/bmips-cpufreq.c b/drivers/cpufreq/bmips-cpufreq.c
index 56a4ebbf00e0..f7c23fa468f0 100644
--- a/drivers/cpufreq/bmips-cpufreq.c
+++ b/drivers/cpufreq/bmips-cpufreq.c
@@ -131,23 +131,18 @@ static int bmips_cpufreq_exit(struct cpufreq_policy *policy)
static int bmips_cpufreq_init(struct cpufreq_policy *policy)
{
struct cpufreq_frequency_table *freq_table;
- int ret;
freq_table = bmips_cpufreq_get_freq_table(policy);
if (IS_ERR(freq_table)) {
- ret = PTR_ERR(freq_table);
- pr_err("%s: couldn't determine frequency table (%d).\n",
- BMIPS_CPUFREQ_NAME, ret);
- return ret;
+ pr_err("%s: couldn't determine frequency table (%ld).\n",
+ BMIPS_CPUFREQ_NAME, PTR_ERR(freq_table));
+ return PTR_ERR(freq_table);
}
- ret = cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
- if (ret)
- bmips_cpufreq_exit(policy);
- else
- pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
+ cpufreq_generic_init(policy, freq_table, TRANSITION_LATENCY);
+ pr_info("%s: registered\n", BMIPS_CPUFREQ_NAME);
- return ret;
+ return 0;
}
static struct cpufreq_driver bmips_cpufreq_driver = {
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 4d6043ee7834..8dda62367816 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -159,7 +159,7 @@ EXPORT_SYMBOL_GPL(arch_set_freq_scale);
* - set policies transition latency
* - policy->cpus with all possible CPUs
*/
-int cpufreq_generic_init(struct cpufreq_policy *policy,
+void cpufreq_generic_init(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *table,
unsigned int transition_latency)
{
@@ -171,8 +171,6 @@ int cpufreq_generic_init(struct cpufreq_policy *policy,
* share the clock and voltage and clock.
*/
cpumask_setall(policy->cpus);
-
- return 0;
}
EXPORT_SYMBOL_GPL(cpufreq_generic_init);
diff --git a/drivers/cpufreq/davinci-cpufreq.c b/drivers/cpufreq/davinci-cpufreq.c
index 3de48ae60c29..297d23cad8b5 100644
--- a/drivers/cpufreq/davinci-cpufreq.c
+++ b/drivers/cpufreq/davinci-cpufreq.c
@@ -90,7 +90,8 @@ static int davinci_cpu_init(struct cpufreq_policy *policy)
* Setting the latency to 2000 us to accommodate addition of drivers
* to pre/post change notification list.
*/
- return cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+ cpufreq_generic_init(policy, freq_table, 2000 * 1000);
+ return 0;
}
static struct cpufreq_driver davinci_driver = {
diff --git a/drivers/cpufreq/imx6q-cpufreq.c b/drivers/cpufreq/imx6q-cpufreq.c
index 47ccfa6b17b7..648a09a1778a 100644
--- a/drivers/cpufreq/imx6q-cpufreq.c
+++ b/drivers/cpufreq/imx6q-cpufreq.c
@@ -190,14 +190,12 @@ static int imx6q_set_target(struct cpufreq_policy *policy, unsigned int index)
static int imx6q_cpufreq_init(struct cpufreq_policy *policy)
{
- int ret;
-
policy->clk = clks[ARM].clk;
- ret = cpufreq_generic_init(policy, freq_table, transition_latency);
+ cpufreq_generic_init(policy, freq_table, transition_latency);
policy->suspend_freq = max_freq;
dev_pm_opp_of_register_em(policy->cpus);
- return ret;
+ return 0;
}
static struct cpufreq_driver imx6q_cpufreq_driver = {
diff --git a/drivers/cpufreq/kirkwood-cpufreq.c b/drivers/cpufreq/kirkwood-cpufreq.c
index 7ab564c1f7ae..cb74bdc5baaa 100644
--- a/drivers/cpufreq/kirkwood-cpufreq.c
+++ b/drivers/cpufreq/kirkwood-cpufreq.c
@@ -85,7 +85,8 @@ static int kirkwood_cpufreq_target(struct cpufreq_policy *policy,
/* Module init and exit code */
static int kirkwood_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, kirkwood_freq_table, 5000);
+ cpufreq_generic_init(policy, kirkwood_freq_table, 5000);
+ return 0;
}
static struct cpufreq_driver kirkwood_cpufreq_driver = {
diff --git a/drivers/cpufreq/loongson1-cpufreq.c b/drivers/cpufreq/loongson1-cpufreq.c
index 21c9ce8526c0..0ea88778882a 100644
--- a/drivers/cpufreq/loongson1-cpufreq.c
+++ b/drivers/cpufreq/loongson1-cpufreq.c
@@ -81,7 +81,7 @@ static int ls1x_cpufreq_init(struct cpufreq_policy *policy)
struct device *cpu_dev = get_cpu_device(policy->cpu);
struct cpufreq_frequency_table *freq_tbl;
unsigned int pll_freq, freq;
- int steps, i, ret;
+ int steps, i;
pll_freq = clk_get_rate(cpufreq->pll_clk) / 1000;
@@ -103,11 +103,9 @@ static int ls1x_cpufreq_init(struct cpufreq_policy *policy)
freq_tbl[i].frequency = CPUFREQ_TABLE_END;
policy->clk = cpufreq->clk;
- ret = cpufreq_generic_init(policy, freq_tbl, 0);
- if (ret)
- kfree(freq_tbl);
+ cpufreq_generic_init(policy, freq_tbl, 0);
- return ret;
+ return 0;
}
static int ls1x_cpufreq_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/loongson2_cpufreq.c b/drivers/cpufreq/loongson2_cpufreq.c
index da344696beed..890813e0bb76 100644
--- a/drivers/cpufreq/loongson2_cpufreq.c
+++ b/drivers/cpufreq/loongson2_cpufreq.c
@@ -95,7 +95,8 @@ static int loongson2_cpufreq_cpu_init(struct cpufreq_policy *policy)
}
policy->clk = cpuclk;
- return cpufreq_generic_init(policy, &loongson2_clockmod_table[0], 0);
+ cpufreq_generic_init(policy, &loongson2_clockmod_table[0], 0);
+ return 0;
}
static int loongson2_cpufreq_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/maple-cpufreq.c b/drivers/cpufreq/maple-cpufreq.c
index f5220b3d4ec5..28d346062166 100644
--- a/drivers/cpufreq/maple-cpufreq.c
+++ b/drivers/cpufreq/maple-cpufreq.c
@@ -140,7 +140,8 @@ static unsigned int maple_cpufreq_get_speed(unsigned int cpu)
static int maple_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, maple_cpu_freqs, 12000);
+ cpufreq_generic_init(policy, maple_cpu_freqs, 12000);
+ return 0;
}
static struct cpufreq_driver maple_cpufreq_driver = {
diff --git a/drivers/cpufreq/omap-cpufreq.c b/drivers/cpufreq/omap-cpufreq.c
index 29643f06a3c3..8d14b42a8c6f 100644
--- a/drivers/cpufreq/omap-cpufreq.c
+++ b/drivers/cpufreq/omap-cpufreq.c
@@ -122,23 +122,18 @@ static int omap_cpu_init(struct cpufreq_policy *policy)
dev_err(mpu_dev,
"%s: cpu%d: failed creating freq table[%d]\n",
__func__, policy->cpu, result);
- goto fail;
+ clk_put(policy->clk);
+ return result;
}
}
atomic_inc_return(&freq_table_users);
/* FIXME: what's the actual transition time? */
- result = cpufreq_generic_init(policy, freq_table, 300 * 1000);
- if (!result) {
- dev_pm_opp_of_register_em(policy->cpus);
- return 0;
- }
+ cpufreq_generic_init(policy, freq_table, 300 * 1000);
+ dev_pm_opp_of_register_em(policy->cpus);
- freq_table_free();
-fail:
- clk_put(policy->clk);
- return result;
+ return 0;
}
static int omap_cpu_exit(struct cpufreq_policy *policy)
diff --git a/drivers/cpufreq/pasemi-cpufreq.c b/drivers/cpufreq/pasemi-cpufreq.c
index 6b1e4abe3248..93f39a1d4c3d 100644
--- a/drivers/cpufreq/pasemi-cpufreq.c
+++ b/drivers/cpufreq/pasemi-cpufreq.c
@@ -196,7 +196,8 @@ static int pas_cpufreq_cpu_init(struct cpufreq_policy *policy)
policy->cur = pas_freqs[cur_astate].frequency;
ppc_proc_freq = policy->cur * 1000ul;
- return cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
+ cpufreq_generic_init(policy, pas_freqs, get_gizmo_latency());
+ return 0;
out_unmap_sdcpwr:
iounmap(sdcpwr_mapbase);
diff --git a/drivers/cpufreq/pmac32-cpufreq.c b/drivers/cpufreq/pmac32-cpufreq.c
index 650104d729f3..73621bc11976 100644
--- a/drivers/cpufreq/pmac32-cpufreq.c
+++ b/drivers/cpufreq/pmac32-cpufreq.c
@@ -372,7 +372,8 @@ static int pmac_cpufreq_target( struct cpufreq_policy *policy,
static int pmac_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, pmac_cpu_freqs, transition_latency);
+ cpufreq_generic_init(policy, pmac_cpu_freqs, transition_latency);
+ return 0;
}
static u32 read_gpio(struct device_node *np)
diff --git a/drivers/cpufreq/pmac64-cpufreq.c b/drivers/cpufreq/pmac64-cpufreq.c
index 1af3492a000d..d7542a106e6b 100644
--- a/drivers/cpufreq/pmac64-cpufreq.c
+++ b/drivers/cpufreq/pmac64-cpufreq.c
@@ -321,7 +321,8 @@ static unsigned int g5_cpufreq_get_speed(unsigned int cpu)
static int g5_cpufreq_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, g5_cpu_freqs, transition_latency);
+ cpufreq_generic_init(policy, g5_cpu_freqs, transition_latency);
+ return 0;
}
static struct cpufreq_driver g5_cpufreq_driver = {
diff --git a/drivers/cpufreq/s3c2416-cpufreq.c b/drivers/cpufreq/s3c2416-cpufreq.c
index f7ff1ed7fef1..106910351c41 100644
--- a/drivers/cpufreq/s3c2416-cpufreq.c
+++ b/drivers/cpufreq/s3c2416-cpufreq.c
@@ -447,21 +447,16 @@ static int s3c2416_cpufreq_driver_init(struct cpufreq_policy *policy)
/* Datasheet says PLL stabalisation time must be at least 300us,
* so but add some fudge. (reference in LOCKCON0 register description)
*/
- ret = cpufreq_generic_init(policy, s3c_freq->freq_table,
+ cpufreq_generic_init(policy, s3c_freq->freq_table,
(500 * 1000) + s3c_freq->regulator_latency);
- if (ret)
- goto err_freq_table;
-
register_reboot_notifier(&s3c2416_cpufreq_reboot_notifier);
return 0;
-err_freq_table:
#ifdef CONFIG_ARM_S3C2416_CPUFREQ_VCORESCALE
- regulator_put(s3c_freq->vddarm);
err_vddarm:
-#endif
clk_put(s3c_freq->armclk);
+#endif
err_armclk:
clk_put(s3c_freq->hclk);
err_hclk:
diff --git a/drivers/cpufreq/s3c64xx-cpufreq.c b/drivers/cpufreq/s3c64xx-cpufreq.c
index 37df2d892eb0..af0c00dabb22 100644
--- a/drivers/cpufreq/s3c64xx-cpufreq.c
+++ b/drivers/cpufreq/s3c64xx-cpufreq.c
@@ -144,7 +144,6 @@ static void s3c64xx_cpufreq_config_regulator(void)
static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
{
- int ret;
struct cpufreq_frequency_table *freq;
if (policy->cpu != 0)
@@ -165,8 +164,7 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
#ifdef CONFIG_REGULATOR
vddarm = regulator_get(NULL, "vddarm");
if (IS_ERR(vddarm)) {
- ret = PTR_ERR(vddarm);
- pr_err("Failed to obtain VDDARM: %d\n", ret);
+ pr_err("Failed to obtain VDDARM: %ld\n", PTR_ERR(vddarm));
pr_err("Only frequency scaling available\n");
vddarm = NULL;
} else {
@@ -196,16 +194,9 @@ static int s3c64xx_cpufreq_driver_init(struct cpufreq_policy *policy)
* the PLLs, which we don't currently) is ~300us worst case,
* but add some fudge.
*/
- ret = cpufreq_generic_init(policy, s3c64xx_freq_table,
+ cpufreq_generic_init(policy, s3c64xx_freq_table,
(500 * 1000) + regulator_latency);
- if (ret != 0) {
- pr_err("Failed to configure frequency table: %d\n",
- ret);
- regulator_put(vddarm);
- clk_put(policy->clk);
- }
-
- return ret;
+ return 0;
}
static struct cpufreq_driver s3c64xx_cpufreq_driver = {
diff --git a/drivers/cpufreq/s5pv210-cpufreq.c b/drivers/cpufreq/s5pv210-cpufreq.c
index e5cb17d4be7b..5d10030f2560 100644
--- a/drivers/cpufreq/s5pv210-cpufreq.c
+++ b/drivers/cpufreq/s5pv210-cpufreq.c
@@ -541,7 +541,8 @@ static int s5pv210_cpu_init(struct cpufreq_policy *policy)
s5pv210_dram_conf[1].freq = clk_get_rate(dmc1_clk);
policy->suspend_freq = SLEEP_FREQ;
- return cpufreq_generic_init(policy, s5pv210_freq_table, 40000);
+ cpufreq_generic_init(policy, s5pv210_freq_table, 40000);
+ return 0;
out_dmc1:
clk_put(dmc0_clk);
diff --git a/drivers/cpufreq/sa1100-cpufreq.c b/drivers/cpufreq/sa1100-cpufreq.c
index ab5cab93e638..5c075ef6adc0 100644
--- a/drivers/cpufreq/sa1100-cpufreq.c
+++ b/drivers/cpufreq/sa1100-cpufreq.c
@@ -181,7 +181,8 @@ static int sa1100_target(struct cpufreq_policy *policy, unsigned int ppcr)
static int __init sa1100_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ return 0;
}
static struct cpufreq_driver sa1100_driver __refdata = {
diff --git a/drivers/cpufreq/sa1110-cpufreq.c b/drivers/cpufreq/sa1110-cpufreq.c
index dab54e051c0e..d9d04d935b3a 100644
--- a/drivers/cpufreq/sa1110-cpufreq.c
+++ b/drivers/cpufreq/sa1110-cpufreq.c
@@ -303,7 +303,8 @@ static int sa1110_target(struct cpufreq_policy *policy, unsigned int ppcr)
static int __init sa1110_cpu_init(struct cpufreq_policy *policy)
{
- return cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ cpufreq_generic_init(policy, sa11x0_freq_table, 0);
+ return 0;
}
/* sa1110_driver needs __refdata because it must remain after init registers
diff --git a/drivers/cpufreq/spear-cpufreq.c b/drivers/cpufreq/spear-cpufreq.c
index 4074e2615522..73bd8dc47074 100644
--- a/drivers/cpufreq/spear-cpufreq.c
+++ b/drivers/cpufreq/spear-cpufreq.c
@@ -153,8 +153,9 @@ static int spear_cpufreq_target(struct cpufreq_policy *policy,
static int spear_cpufreq_init(struct cpufreq_policy *policy)
{
policy->clk = spear_cpufreq.clk;
- return cpufreq_generic_init(policy, spear_cpufreq.freq_tbl,
+ cpufreq_generic_init(policy, spear_cpufreq.freq_tbl,
spear_cpufreq.transition_latency);
+ return 0;
}
static struct cpufreq_driver spear_cpufreq_driver = {
diff --git a/drivers/cpufreq/tegra20-cpufreq.c b/drivers/cpufreq/tegra20-cpufreq.c
index 3c32cc7b0671..f84ecd22f488 100644
--- a/drivers/cpufreq/tegra20-cpufreq.c
+++ b/drivers/cpufreq/tegra20-cpufreq.c
@@ -118,17 +118,11 @@ static int tegra_target(struct cpufreq_policy *policy, unsigned int index)
static int tegra_cpu_init(struct cpufreq_policy *policy)
{
struct tegra20_cpufreq *cpufreq = cpufreq_get_driver_data();
- int ret;
clk_prepare_enable(cpufreq->cpu_clk);
/* FIXME: what's the actual transition time? */
- ret = cpufreq_generic_init(policy, freq_table, 300 * 1000);
- if (ret) {
- clk_disable_unprepare(cpufreq->cpu_clk);
- return ret;
- }
-
+ cpufreq_generic_init(policy, freq_table, 300 * 1000);
policy->clk = cpufreq->cpu_clk;
policy->suspend_freq = freq_table[0].frequency;
return 0;
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index d757a56a74dc..536a049d7ecc 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -992,7 +992,7 @@ extern struct freq_attr *cpufreq_generic_attr[];
int cpufreq_table_validate_and_sort(struct cpufreq_policy *policy);
unsigned int cpufreq_generic_get(unsigned int cpu);
-int cpufreq_generic_init(struct cpufreq_policy *policy,
+void cpufreq_generic_init(struct cpufreq_policy *policy,
struct cpufreq_frequency_table *table,
unsigned int transition_latency);
#endif /* _LINUX_CPUFREQ_H */
--
2.21.0.rc0.269.g1a574e7a288b
^ permalink raw reply related
* [PATCH] rculist: Add build check for single optional list argument
From: Joel Fernandes (Google) @ 2019-07-16 4:07 UTC (permalink / raw)
To: linux-kernel
Cc: Joel Fernandes (Google), Paul McKenney, Alexey Kuznetsov,
Bjorn Helgaas, Borislav Petkov, c0d1n61at3, David S. Miller,
edumazet, Greg Kroah-Hartman, Hideaki YOSHIFUJI, H. Peter Anvin,
Ingo Molnar, Jonathan Corbet, Josh Triplett, keescook,
kernel-hardening, kernel-team, Lai Jiangshan, Len Brown,
linux-acpi, linux-doc, linux-pci, linux-pm, Mathieu Desnoyers,
neilb, netdev, Oleg Nesterov, Paul E. McKenney, Pavel Machek,
peterz, Rafael J. Wysocki, Rasmus Villemoes, rcu, Steven Rostedt,
Tejun Heo, Thomas Gleixner, will,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT)
In a previous patch series [1], we added an optional lockdep expression
argument to list_for_each_entry_rcu() and the hlist equivalent. This
also meant more than one optional argument can be passed to them with
that error going unnoticed. To fix this, let us force a compiler error
more than one optional argument is passed.
[1] https://lore.kernel.org/patchwork/project/lkml/list/?series=402150
Suggested-by: Paul McKenney <paulmck@linux.vnet.ibm.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
include/linux/rculist.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index 1048160625bb..86659f6d72dc 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -44,14 +44,18 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
* Check during list traversal that we are within an RCU reader
*/
+#define check_arg_count_one(dummy)
+
#ifdef CONFIG_PROVE_RCU_LIST
-#define __list_check_rcu(dummy, cond, ...) \
+#define __list_check_rcu(dummy, cond, extra...) \
({ \
+ check_arg_count_one(extra); \
RCU_LOCKDEP_WARN(!cond && !rcu_read_lock_any_held(), \
"RCU-list traversed in non-reader section!"); \
})
#else
-#define __list_check_rcu(dummy, cond, ...) ({})
+#define __list_check_rcu(dummy, cond, extra...) \
+ ({ check_arg_count_one(extra); })
#endif
/*
--
2.22.0.510.g264f2c817a-goog
^ permalink raw reply related
* Re: [PATCH v4] PM / wakeup: show wakeup sources stats in sysfs
From: Tri Vo @ 2019-07-16 4:16 UTC (permalink / raw)
To: Greg Kroah-Hartman, Rafael J. Wysocki
Cc: Rafael J. Wysocki, Viresh Kumar, Hridya Valsaraju, Sandeep Patil,
kaleshsingh, Linux Kernel Mailing List, Linux PM,
Cc: Android Kernel
In-Reply-To: <20190716021102.GA8310@kroah.com>
On Tue, Jul 16, 2019 at 11:13 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Jul 15, 2019 at 11:44 PM Tri Vo <trong@android.com> wrote:
> > >
> > > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > > blocking wakeup sources over device's boot cycle. This information can
> > > then be used (1) for power-specific bug reporting and (2) towards
> > > attributing battery consumption to specific processes over a period of
> > > time.
> > >
> > > However, debugfs doesn't have stable ABI. For this reason, create a
> > > 'struct device' to expose wakeup sources statistics in sysfs under
> > > /sys/class/wakeup/<name>/.
> > >
> > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > > source statistics in sysfs.
> >
> > I'm not sure if this is really needed, but I'll let Greg decide.
>
> You are right. Having zillions of config options is a pain, who is
> going to turn this off?
>
> But we can always remove the option before 5.4-rc1, so I'll take this
> as-is for now :)
>
> > Apart from this
> >
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> thanks for the review! I'll wait for 5.3-rc1 to come out before adding
> this to my tree.
Greg, Rafael, thanks for taking the time to review this patch!
^ permalink raw reply
* Re: [GIT PULL] power-supply changes for 5.3
From: pr-tracker-bot @ 2019-07-16 4:35 UTC (permalink / raw)
To: Sebastian Reichel; +Cc: Linus Torvalds, linux-kernel, linux-pm, Bjorn Helgaas
In-Reply-To: <20190715194933.cq3cfz7g6j3mnbmc@earth.universe>
The pull request you sent on Mon, 15 Jul 2019 21:49:33 +0200:
> ssh://git@gitolite.kernel.org/pub/scm/linux/kernel/git/sre/linux-power-supply.git tags/for-v5.3
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/5fe7b600a116187e10317d83fb56922c4ef6b76d
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* Re: [PATCH v4] PM / wakeup: show wakeup sources stats in sysfs
From: Rafael J. Wysocki @ 2019-07-16 8:30 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rafael J. Wysocki, Tri Vo, Rafael J. Wysocki, Viresh Kumar,
Hridya Valsaraju, sspatil, kaleshsingh, Linux Kernel Mailing List,
Linux PM, Cc: Android Kernel
In-Reply-To: <20190716021102.GA8310@kroah.com>
On Tue, Jul 16, 2019 at 4:13 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Jul 15, 2019 at 11:44 PM Tri Vo <trong@android.com> wrote:
> > >
> > > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > > blocking wakeup sources over device's boot cycle. This information can
> > > then be used (1) for power-specific bug reporting and (2) towards
> > > attributing battery consumption to specific processes over a period of
> > > time.
> > >
> > > However, debugfs doesn't have stable ABI. For this reason, create a
> > > 'struct device' to expose wakeup sources statistics in sysfs under
> > > /sys/class/wakeup/<name>/.
> > >
> > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > > source statistics in sysfs.
> >
> > I'm not sure if this is really needed, but I'll let Greg decide.
>
> You are right. Having zillions of config options is a pain, who is
> going to turn this off?
>
> But we can always remove the option before 5.4-rc1, so I'll take this
> as-is for now :)
>
> > Apart from this
> >
> > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> thanks for the review! I'll wait for 5.3-rc1 to come out before adding
> this to my tree.
So it occurred to me that maybe it's better if I apply it? After all,
this is PM material. :-)
It is fine by me either way, but then I'm not sure if you want to get
future bug reports related to this if any ...
^ permalink raw reply
* Re: [PATCH v4] PM / wakeup: show wakeup sources stats in sysfs
From: Greg Kroah-Hartman @ 2019-07-16 8:39 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Tri Vo, Rafael J. Wysocki, Viresh Kumar, Hridya Valsaraju,
sspatil, kaleshsingh, Linux Kernel Mailing List, Linux PM,
Cc: Android Kernel
In-Reply-To: <CAJZ5v0hxKeK-eDEs1rGP0ZYbbCHa0q5=ZPMVCemYVHGL48Q=pg@mail.gmail.com>
On Tue, Jul 16, 2019 at 10:30:48AM +0200, Rafael J. Wysocki wrote:
> On Tue, Jul 16, 2019 at 4:13 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Jul 15, 2019 at 11:44 PM Tri Vo <trong@android.com> wrote:
> > > >
> > > > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > > > blocking wakeup sources over device's boot cycle. This information can
> > > > then be used (1) for power-specific bug reporting and (2) towards
> > > > attributing battery consumption to specific processes over a period of
> > > > time.
> > > >
> > > > However, debugfs doesn't have stable ABI. For this reason, create a
> > > > 'struct device' to expose wakeup sources statistics in sysfs under
> > > > /sys/class/wakeup/<name>/.
> > > >
> > > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > > > source statistics in sysfs.
> > >
> > > I'm not sure if this is really needed, but I'll let Greg decide.
> >
> > You are right. Having zillions of config options is a pain, who is
> > going to turn this off?
> >
> > But we can always remove the option before 5.4-rc1, so I'll take this
> > as-is for now :)
> >
> > > Apart from this
> > >
> > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > thanks for the review! I'll wait for 5.3-rc1 to come out before adding
> > this to my tree.
>
> So it occurred to me that maybe it's better if I apply it? After all,
> this is PM material. :-)
Heh, true, feel free to add:
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
to the patch then.
> It is fine by me either way, but then I'm not sure if you want to get
> future bug reports related to this if any ...
I get enough emails as it is, no need to ask for more :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH 10/18] drivers: firmware: psci: Add hierarchical domain idle states converter
From: Ulf Hansson @ 2019-07-16 8:45 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Sudeep Holla, Mark Rutland, Linux ARM, Rafael J . Wysocki,
Daniel Lezcano, Raju P . L . S . S . S . N, Amit Kucheria,
Bjorn Andersson, Stephen Boyd, Niklas Cassel, Tony Lindgren,
Kevin Hilman, Lina Iyer, Viresh Kumar, Vincent Guittot,
Geert Uytterhoeven, Souvik Chakravarty, Linux PM, linux-arm-msm,
Linux Kernel Mailing List
In-Reply-To: <20190709153138.GA22871@e121166-lin.cambridge.arm.com>
Lorenzo,
Just wanted to give some feedback to you in public, even if we have
already discussed this series, offlist, last week.
On Tue, 9 Jul 2019 at 17:31, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
>
> On Mon, May 13, 2019 at 09:22:52PM +0200, Ulf Hansson wrote:
> > If the hierarchical CPU topology is used, but the OS initiated mode isn't
> > supported, we need to rely solely on the regular cpuidle framework to
> > manage the idle state selection, rather than using genpd and its governor.
> >
> > For this reason, introduce a new PSCI DT helper function,
> > psci_dt_pm_domains_parse_states(), which parses and converts the
> > hierarchically described domain idle states from DT, into regular flattened
> > cpuidle states. The converted states are added to the existing cpuidle
> > driver's array of idle states, which make them available for cpuidle.
> >
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >
> > Changes:
> > - Some simplification of the code.
> >
> > ---
> > drivers/firmware/psci/psci.h | 5 ++
> > drivers/firmware/psci/psci_pm_domain.c | 118 +++++++++++++++++++++++++
> > 2 files changed, 123 insertions(+)
> >
> > diff --git a/drivers/firmware/psci/psci.h b/drivers/firmware/psci/psci.h
> > index 00d2e3dcef49..c36e0e6649e9 100644
> > --- a/drivers/firmware/psci/psci.h
> > +++ b/drivers/firmware/psci/psci.h
> > @@ -3,6 +3,7 @@
> > #ifndef __PSCI_H
> > #define __PSCI_H
> >
> > +struct cpuidle_driver;
> > struct device_node;
> >
> > int psci_set_osi_mode(void);
> > @@ -13,8 +14,12 @@ void psci_set_domain_state(u32 state);
> > int psci_dt_parse_state_node(struct device_node *np, u32 *state);
> > #ifdef CONFIG_PM_GENERIC_DOMAINS_OF
> > int psci_dt_init_pm_domains(struct device_node *np);
> > +int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> > + struct device_node *cpu_node, u32 *psci_states);
> > #else
> > static inline int psci_dt_init_pm_domains(struct device_node *np) { return 0; }
> > +static inline int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> > + struct device_node *cpu_node, u32 *psci_states) { return 0; }
> > #endif
> > #endif
> >
> > diff --git a/drivers/firmware/psci/psci_pm_domain.c b/drivers/firmware/psci/psci_pm_domain.c
> > index 3c6ca846caf4..3aa645dba81b 100644
> > --- a/drivers/firmware/psci/psci_pm_domain.c
> > +++ b/drivers/firmware/psci/psci_pm_domain.c
> > @@ -14,6 +14,10 @@
> > #include <linux/pm_domain.h>
> > #include <linux/slab.h>
> > #include <linux/string.h>
> > +#include <linux/cpuidle.h>
> > +#include <linux/cpu_pm.h>
> > +
> > +#include <asm/cpuidle.h>
> >
> > #include "psci.h"
> >
> > @@ -104,6 +108,53 @@ static void psci_pd_free_states(struct genpd_power_state *states,
> > kfree(states);
> > }
> >
> > +static int psci_pd_enter_pc(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv, int idx)
> > +{
> > + return CPU_PM_CPU_IDLE_ENTER(arm_cpuidle_suspend, idx);
> > +}
> > +
> > +static void psci_pd_enter_s2idle_pc(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv, int idx)
> > +{
> > + psci_pd_enter_pc(dev, drv, idx);
> > +}
> > +
> > +static void psci_pd_convert_states(struct cpuidle_state *idle_state,
> > + u32 *psci_state, struct genpd_power_state *state)
> > +{
> > + u32 *state_data = state->data;
> > + u64 target_residency_us = state->residency_ns;
> > + u64 exit_latency_us = state->power_on_latency_ns +
> > + state->power_off_latency_ns;
> > +
> > + *psci_state = *state_data;
> > + do_div(target_residency_us, 1000);
> > + idle_state->target_residency = target_residency_us;
> > + do_div(exit_latency_us, 1000);
> > + idle_state->exit_latency = exit_latency_us;
> > + idle_state->enter = &psci_pd_enter_pc;
> > + idle_state->enter_s2idle = &psci_pd_enter_s2idle_pc;
> > + idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
>
> This is arbitrary and not necessarily true.
The arbitrary thing you refer to here, is that the
CPUIDLE_FLAG_TIMER_STOP? Or are you referring to the complete function
psci_pd_convert_states()?
>
> I think that this patch is useful to represent my reservations about the
> current approach. As a matter of fact, idle state entry will always be a
> CPUidle decision.
>
> You only need PM domain information to understand when all CPUs
> in a power domain are actually idle but that's all genPD can do
> in this respect.
>
> I think this patchset would be much simpler if both CPUidle and
> genPD governor would work on *one* set of idle states, globally
> indexed (and that would be true for PSCI suspend parameters too).
>
> To work with a unified set of idle states between CPUidle and genPD
> (tossing some ideas around):
>
> - We can implement a genPD CPUidle governor that in its select method
> takes into account genPD information (for instance by avoiding
> selection of idle states that require multiple cpus to be in idle
> to be effectively active)
> - We can use genPD to enable/disable CPUidle states through runtime
> PM information
I don't understand how to make this work.
The CPUidle governor works on per CPU basis. The genpd governor works
on per PM domain basis, which typically can be a group of CPUs (and
even other devices) via subdomains, for example.
1.
In case of Linux being in *charge* of what idle state to pick for a
group of CPUs, that decision is best done by the genpd governor as it
operates on "groups" of CPUs. This is used for PSCI OSI mode. Of
course, there are idle states also per CPU, which potentially could be
managed by the genpd governor as well, but at this point I decided to
re-use the cpuidle governor as it's already doing the job.
2. In case the decision of what idle state to enter for the group is
done by the FW, we can rely solely on the cpuidle governor and let it
select states per CPU basis. This is used for PSCI PC mode.
>
> There may be other ways. My point is that current code, with two (or
> more if the hierarchy grows) sets of idle states across two subsystems
> (CPUidle and genPD) is not very well defined and honestly very hard to
> grasp and prone to errors.
The complexity is there, I admit that.
I guess some deeper insight about genpd+its governor+runtime PM are
needed when reviewing this, of course. As an option, you may also have
a look at my slides [1] from OSPM (Pisa) in May this year, which via
flow charts try to describes things in more detail.
In our offlist meeting, we discussed that perhaps moving some of the
new PSCI code introduced in this series, into a cpuidle driver
instead, could make things more clear. For sure, I can explore that
option, but before I go there, I think we should agree on it publicly.
In principle what it means is to invent a special cpuidle driver for
PSCI, so we would need access to some of the PSCI internal functions,
for example.
One thing though, the initialization of the PSCI PM domain topology is
a separate step, managed via the new initcall, psci_dt_topology_init()
(introduced in patch 11). That part still seems to be belong to the
PSCI code, don't you think?
>
> > +
> > + strncpy(idle_state->name, to_of_node(state->fwnode)->name,
> > + CPUIDLE_NAME_LEN - 1);
> > + strncpy(idle_state->desc, to_of_node(state->fwnode)->name,
> > + CPUIDLE_NAME_LEN - 1);
> > +}
> > +
> > +static bool psci_pd_is_provider(struct device_node *np)
> > +{
> > + struct psci_pd_provider *pd_prov, *it;
> > +
> > + list_for_each_entry_safe(pd_prov, it, &psci_pd_providers, link) {
> > + if (pd_prov->node == np)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > static int psci_pd_init(struct device_node *np)
> > {
> > struct generic_pm_domain *pd;
> > @@ -265,4 +316,71 @@ int psci_dt_init_pm_domains(struct device_node *np)
> > pr_err("failed to create CPU PM domains ret=%d\n", ret);
> > return ret;
> > }
> > +
> > +int psci_dt_pm_domains_parse_states(struct cpuidle_driver *drv,
> > + struct device_node *cpu_node, u32 *psci_states)
> > +{
> > + struct genpd_power_state *pd_states;
> > + struct of_phandle_args args;
> > + int ret, pd_state_count, i, state_idx, psci_idx;
> > + u32 cpu_psci_state = psci_states[drv->state_count - 2];
>
> This (-2) is very dodgy and I doubt it would work on hierarchies going
> above "cluster" level.
>
> As I say above, I think we should work towards a single array of
> idle states to be selected by a CPUidle governor using genPD
> runtime information to bias the results according to the number
> of CPUs in a genPD that entered/exit idle.
>
> To be more precise, all idles states should be "domain-idle-state"
> compatible, even the CPU ones, the distinction between what CPUidle
> and genPD manage is a bit stretched IMO in this patchset.
>
> We will have a chance to talk about this but I thought I would
> comment publically if anyone else is willing to chime in, this
> is not a PSCI problem at all, it is a CPUidle/genPD coexistence
> design problem which is much broader.
To move this forward, I think we need to move from vague ideas to
clear and distinct plans. Whatever that means. :-)
I understand you are concerned about the level of changes introduced
to the PSCI code. As I stated somewhere in earlier replies, I picked
that approach as I thought it was better to implement things in a PSCI
specific manner to start with, then we could move things around, when
we realize that it make sense.
Anyway, as a suggestion to address your concern, how about this:
1. Move some things out to a PSCI cpuidle driver. We need to decide
more exactly on what to move and find the right level for the
interfaces.
2. Don't attach the CPU to the PM domain topology in case the PSCI PC
mode is used. I think this makes it easier, at least as a first step,
to understand when runtime PM needs to be used/enabled.
3. Would it help if I volunteer to help you guys as a maintainer for
PSCI. At least for the part of the new code that becomes introduced?
[...]
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH v2 4/4] dt-bindings: devfreq: exynos-bus: remove unused property
From: Chanwoo Choi @ 2019-07-16 8:54 UTC (permalink / raw)
To: Kamil Konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <20190715120416.3561-5-k.konieczny@partner.samsung.com>
Hi,
On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Remove unused DT property "exynos,voltage-tolerance".
>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
> Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 2 --
> 1 file changed, 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> index f8e946471a58..e71f752cc18f 100644
> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> @@ -50,8 +50,6 @@ Required properties only for passive bus device:
> Optional properties only for parent bus device:
> - exynos,saturation-ratio: the percentage value which is used to calibrate
> the performance count against total cycle count.
> -- exynos,voltage-tolerance: the percentage value for bus voltage tolerance
> - which is used to calculate the max voltage.
>
> Detailed correlation between sub-blocks and power line according to Exynos SoC:
> - In case of Exynos3250, there are two power line as following:
>
Acked-by: Chanwoo Choi <cw00.choi@samsung.com>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v2 3/4] ARM: dts: exynos: add initial data for coupled regulators for Exynos5422/5800
From: Chanwoo Choi @ 2019-07-16 9:00 UTC (permalink / raw)
To: Kamil Konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Krzysztof Kozlowski,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc
In-Reply-To: <20190715120416.3561-4-k.konieczny@partner.samsung.com>
Hi,
On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Declare Exynos5422/5800 voltage ranges for opp points for big cpu core and
> bus wcore and couple their voltage supllies as vdd_arm and vdd_int should
> be in 300mV range.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
> arch/arm/boot/dts/exynos5420.dtsi | 34 +++++++++----------
> arch/arm/boot/dts/exynos5422-odroid-core.dtsi | 4 +++
> arch/arm/boot/dts/exynos5800-peach-pi.dts | 4 +++
> arch/arm/boot/dts/exynos5800.dtsi | 32 ++++++++---------
> 4 files changed, 41 insertions(+), 33 deletions(-)
Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> index 5fb2326875dc..0cbf74750553 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -48,62 +48,62 @@
> opp-shared;
> opp-1800000000 {
> opp-hz = /bits/ 64 <1800000000>;
> - opp-microvolt = <1250000>;
> + opp-microvolt = <1250000 1250000 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-1700000000 {
> opp-hz = /bits/ 64 <1700000000>;
> - opp-microvolt = <1212500>;
> + opp-microvolt = <1212500 1212500 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-1600000000 {
> opp-hz = /bits/ 64 <1600000000>;
> - opp-microvolt = <1175000>;
> + opp-microvolt = <1175000 1175000 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-1500000000 {
> opp-hz = /bits/ 64 <1500000000>;
> - opp-microvolt = <1137500>;
> + opp-microvolt = <1137500 1137500 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-1400000000 {
> opp-hz = /bits/ 64 <1400000000>;
> - opp-microvolt = <1112500>;
> + opp-microvolt = <1112500 1112500 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-1300000000 {
> opp-hz = /bits/ 64 <1300000000>;
> - opp-microvolt = <1062500>;
> + opp-microvolt = <1062500 1062500 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-1200000000 {
> opp-hz = /bits/ 64 <1200000000>;
> - opp-microvolt = <1037500>;
> + opp-microvolt = <1037500 1037500 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-1100000000 {
> opp-hz = /bits/ 64 <1100000000>;
> - opp-microvolt = <1012500>;
> + opp-microvolt = <1012500 1012500 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-1000000000 {
> opp-hz = /bits/ 64 <1000000000>;
> - opp-microvolt = < 987500>;
> + opp-microvolt = < 987500 987500 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-900000000 {
> opp-hz = /bits/ 64 <900000000>;
> - opp-microvolt = < 962500>;
> + opp-microvolt = < 962500 962500 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-800000000 {
> opp-hz = /bits/ 64 <800000000>;
> - opp-microvolt = < 937500>;
> + opp-microvolt = < 937500 937500 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-700000000 {
> opp-hz = /bits/ 64 <700000000>;
> - opp-microvolt = < 912500>;
> + opp-microvolt = < 912500 912500 1500000>;
> clock-latency-ns = <140000>;
> };
> };
> @@ -1100,23 +1100,23 @@
>
> opp00 {
> opp-hz = /bits/ 64 <84000000>;
> - opp-microvolt = <925000>;
> + opp-microvolt = <925000 925000 1400000>;
> };
> opp01 {
> opp-hz = /bits/ 64 <111000000>;
> - opp-microvolt = <950000>;
> + opp-microvolt = <950000 950000 1400000>;
> };
> opp02 {
> opp-hz = /bits/ 64 <222000000>;
> - opp-microvolt = <950000>;
> + opp-microvolt = <950000 950000 1400000>;
> };
> opp03 {
> opp-hz = /bits/ 64 <333000000>;
> - opp-microvolt = <950000>;
> + opp-microvolt = <950000 950000 1400000>;
> };
> opp04 {
> opp-hz = /bits/ 64 <400000000>;
> - opp-microvolt = <987500>;
> + opp-microvolt = <987500 987500 1400000>;
> };
> };
>
> diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> index 25d95de15c9b..65d094256b54 100644
> --- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> +++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> @@ -428,6 +428,8 @@
> regulator-max-microvolt = <1500000>;
> regulator-always-on;
> regulator-boot-on;
> + regulator-coupled-with = <&buck3_reg>;
> + regulator-coupled-max-spread = <300000>;
> };
>
> buck3_reg: BUCK3 {
> @@ -436,6 +438,8 @@
> regulator-max-microvolt = <1400000>;
> regulator-always-on;
> regulator-boot-on;
> + regulator-coupled-with = <&buck2_reg>;
> + regulator-coupled-max-spread = <300000>;
> };
>
> buck4_reg: BUCK4 {
> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> index e0f470fe54c8..5c1e965ed7e9 100644
> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> @@ -257,6 +257,8 @@
> regulator-always-on;
> regulator-boot-on;
> regulator-ramp-delay = <12500>;
> + regulator-coupled-with = <&buck3_reg>;
> + regulator-coupled-max-spread = <300000>;
> regulator-state-mem {
> regulator-off-in-suspend;
> };
> @@ -269,6 +271,8 @@
> regulator-always-on;
> regulator-boot-on;
> regulator-ramp-delay = <12500>;
> + regulator-coupled-with = <&buck2_reg>;
> + regulator-coupled-max-spread = <300000>;
> regulator-state-mem {
> regulator-off-in-suspend;
> };
> diff --git a/arch/arm/boot/dts/exynos5800.dtsi b/arch/arm/boot/dts/exynos5800.dtsi
> index 57d3b319fd65..2a74735d161c 100644
> --- a/arch/arm/boot/dts/exynos5800.dtsi
> +++ b/arch/arm/boot/dts/exynos5800.dtsi
> @@ -22,61 +22,61 @@
>
> &cluster_a15_opp_table {
> opp-1700000000 {
> - opp-microvolt = <1250000>;
> + opp-microvolt = <1250000 1250000 1500000>;
> };
> opp-1600000000 {
> - opp-microvolt = <1250000>;
> + opp-microvolt = <1250000 1250000 1500000>;
> };
> opp-1500000000 {
> - opp-microvolt = <1100000>;
> + opp-microvolt = <1100000 1100000 1500000>;
> };
> opp-1400000000 {
> - opp-microvolt = <1100000>;
> + opp-microvolt = <1100000 1100000 1500000>;
> };
> opp-1300000000 {
> - opp-microvolt = <1100000>;
> + opp-microvolt = <1100000 1100000 1500000>;
> };
> opp-1200000000 {
> - opp-microvolt = <1000000>;
> + opp-microvolt = <1000000 1000000 1500000>;
> };
> opp-1100000000 {
> - opp-microvolt = <1000000>;
> + opp-microvolt = <1000000 1000000 1500000>;
> };
> opp-1000000000 {
> - opp-microvolt = <1000000>;
> + opp-microvolt = <1000000 1000000 1500000>;
> };
> opp-900000000 {
> - opp-microvolt = <1000000>;
> + opp-microvolt = <1000000 1000000 1500000>;
> };
> opp-800000000 {
> - opp-microvolt = <900000>;
> + opp-microvolt = <900000 900000 1500000>;
> };
> opp-700000000 {
> - opp-microvolt = <900000>;
> + opp-microvolt = <900000 900000 1500000>;
> };
> opp-600000000 {
> opp-hz = /bits/ 64 <600000000>;
> - opp-microvolt = <900000>;
> + opp-microvolt = <900000 900000 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-500000000 {
> opp-hz = /bits/ 64 <500000000>;
> - opp-microvolt = <900000>;
> + opp-microvolt = <900000 900000 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-400000000 {
> opp-hz = /bits/ 64 <400000000>;
> - opp-microvolt = <900000>;
> + opp-microvolt = <900000 900000 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-300000000 {
> opp-hz = /bits/ 64 <300000000>;
> - opp-microvolt = <900000>;
> + opp-microvolt = <900000 900000 1500000>;
> clock-latency-ns = <140000>;
> };
> opp-200000000 {
> opp-hz = /bits/ 64 <200000000>;
> - opp-microvolt = <900000>;
> + opp-microvolt = <900000 900000 1500000>;
> clock-latency-ns = <140000>;
> };
> };
>
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply
* Re: [PATCH v6] cpufreq/pasemi: fix an use-after-free inpas_cpufreq_cpu_init()
From: Viresh Kumar @ 2019-07-16 9:00 UTC (permalink / raw)
To: wen.yang99
Cc: rjw, linuxppc-dev, linux-pm, linux-kernel, xue.zhihong, wang.yi59,
cheng.shengyu, mpe
In-Reply-To: <201907161626465333445@zte.com.cn>
On 16-07-19, 16:26, wen.yang99@zte.com.cn wrote:
> Okay thank you.
> Now this patch
> (https://lore.kernel.org/lkml/ee8cf5fb4b4a01fdf9199037ff6d835b935cfd13.1562902877.git.viresh.kumar@linaro.org/)
> seems to have not been merged into the linux-next.
>
> In order to avoid code conflicts, we will wait until this patch is merged in and then send v7.
Please rebase on PM tree's linux-next branch instead and resend your
patch.
git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git
--
viresh
^ permalink raw reply
* Re: [PATCH AUTOSEL 4.19 123/158] cpufreq: Don't skip frequency validation for has_target() drivers
From: Rafael J. Wysocki @ 2019-07-16 9:21 UTC (permalink / raw)
To: Sasha Levin, linux-kernel, stable; +Cc: Viresh Kumar, linux-pm
In-Reply-To: <20190715141809.8445-123-sashal@kernel.org>
On 7/15/2019 4:17 PM, Sasha Levin wrote:
> From: Viresh Kumar <viresh.kumar@linaro.org>
>
> [ Upstream commit 9801522840cc1073f8064b4c979b7b6995c74bca ]
>
> CPUFREQ_CONST_LOOPS was introduced in a very old commit from pre-2.6
> kernel release by commit 6a4a93f9c0d5 ("[CPUFREQ] Fix 'out of sync'
> issue").
>
> Basically, that commit does two things:
>
> - It adds the frequency verification code (which is quite similar to
> what we have today as well).
>
> - And it sets the CPUFREQ_CONST_LOOPS flag only for setpolicy drivers,
> rightly so based on the code we had then. The idea was to avoid
> frequency validation for setpolicy drivers as the cpufreq core doesn't
> know what frequency the hardware is running at and so no point in
> doing frequency verification.
>
> The problem happened when we started to use the same CPUFREQ_CONST_LOOPS
> flag for constant loops-per-jiffy thing as well and many has_target()
> drivers started using the same flag and unknowingly skipped the
> verification of frequency. There is no logical reason behind skipping
> frequency validation because of the presence of CPUFREQ_CONST_LOOPS
> flag otherwise.
>
> Fix this issue by skipping frequency validation only for setpolicy
> drivers and always doing it for has_target() drivers irrespective of
> the presence or absence of CPUFREQ_CONST_LOOPS flag.
>
> cpufreq_notify_transition() is only called for has_target() type driver
> and not for set_policy type, and the check is simply redundant. Remove
> it as well.
>
> Also remove () around freq comparison statement as they aren't required
> and checkpatch also warns for them.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> drivers/cpufreq/cpufreq.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index d3213594d1a7..80942ec34efd 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -321,12 +321,10 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy,
> * which is not equal to what the cpufreq core thinks is
> * "old frequency".
> */
> - if (!(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
> - if (policy->cur && (policy->cur != freqs->old)) {
> - pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
> - freqs->old, policy->cur);
> - freqs->old = policy->cur;
> - }
> + if (policy->cur && policy->cur != freqs->old) {
> + pr_debug("Warning: CPU frequency is %u, cpufreq assumed %u kHz\n",
> + freqs->old, policy->cur);
> + freqs->old = policy->cur;
> }
>
> for_each_cpu(freqs->cpu, policy->cpus) {
> @@ -1543,8 +1541,7 @@ static unsigned int __cpufreq_get(struct cpufreq_policy *policy)
> if (policy->fast_switch_enabled)
> return ret_freq;
>
> - if (ret_freq && policy->cur &&
> - !(cpufreq_driver->flags & CPUFREQ_CONST_LOOPS)) {
> + if (has_target() && ret_freq && policy->cur) {
> /* verify no discrepancy between actual and
> saved value exists */
> if (unlikely(ret_freq != policy->cur)) {
This is not -stable material, please drop it.
^ permalink raw reply
* Re: [PATCH v2 3/4] ARM: dts: exynos: add initial data for coupled regulators for Exynos5422/5800
From: Krzysztof Kozlowski @ 2019-07-16 9:22 UTC (permalink / raw)
To: Kamil Konieczny
Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski, Chanwoo Choi,
Kukjin Kim, Kyungmin Park, Mark Rutland, MyungJoo Ham,
Nishanth Menon, Rob Herring, Stephen Boyd, Viresh Kumar,
devicetree, linux-arm-kernel, linux-kernel, linux-pm,
linux-samsung-soc@vger.kernel.org
In-Reply-To: <20190715120416.3561-4-k.konieczny@partner.samsung.com>
On Mon, 15 Jul 2019 at 14:04, Kamil Konieczny
<k.konieczny@partner.samsung.com> wrote:
>
> Declare Exynos5422/5800 voltage ranges for opp points for big cpu core and
> bus wcore and couple their voltage supllies as vdd_arm and vdd_int should
> be in 300mV range.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
This one was previously from Marek, now it is from you. Any changes here?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH AUTOSEL 5.2 190/249] cpufreq: Avoid calling cpufreq_verify_current_freq() from handle_update()
From: Rafael J. Wysocki @ 2019-07-16 9:25 UTC (permalink / raw)
To: Sasha Levin, linux-kernel, stable; +Cc: Viresh Kumar, linux-pm
In-Reply-To: <20190715134655.4076-190-sashal@kernel.org>
On 7/15/2019 3:45 PM, Sasha Levin wrote:
> From: Viresh Kumar <viresh.kumar@linaro.org>
>
> [ Upstream commit 70a59fde6e69d1d8579f84bf4555bfffb3ce452d ]
>
> On some occasions cpufreq_verify_current_freq() schedules a work whose
> callback is handle_update(), which further calls cpufreq_update_policy()
> which may end up calling cpufreq_verify_current_freq() again.
>
> On the other hand, when cpufreq_update_policy() is called from
> handle_update(), the pointer to the cpufreq policy is already
> available, but cpufreq_cpu_acquire() is still called to get it in
> cpufreq_update_policy(), which should be avoided as well.
>
> To fix these issues, create a new helper, refresh_frequency_limits(),
> and make both handle_update() call it cpufreq_update_policy().
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> [ rjw: Rename reeval_frequency_limits() as refresh_frequency_limits() ]
> [ rjw: Changelog ]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
> drivers/cpufreq/cpufreq.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index e84bf0eb7239..876a4cb09de3 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1114,13 +1114,25 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy, unsigned int cp
> return ret;
> }
>
> +static void refresh_frequency_limits(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_policy new_policy = *policy;
> +
> + pr_debug("updating policy for CPU %u\n", policy->cpu);
> +
> + new_policy.min = policy->user_policy.min;
> + new_policy.max = policy->user_policy.max;
> +
> + cpufreq_set_policy(policy, &new_policy);
> +}
> +
> static void handle_update(struct work_struct *work)
> {
> struct cpufreq_policy *policy =
> container_of(work, struct cpufreq_policy, update);
> - unsigned int cpu = policy->cpu;
> - pr_debug("handle_update for cpu %u called\n", cpu);
> - cpufreq_update_policy(cpu);
> +
> + pr_debug("handle_update for cpu %u called\n", policy->cpu);
> + refresh_frequency_limits(policy);
> }
>
> static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> @@ -2392,7 +2404,6 @@ int cpufreq_set_policy(struct cpufreq_policy *policy,
> void cpufreq_update_policy(unsigned int cpu)
> {
> struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
> - struct cpufreq_policy new_policy;
>
> if (!policy)
> return;
> @@ -2405,12 +2416,7 @@ void cpufreq_update_policy(unsigned int cpu)
> (cpufreq_suspended || WARN_ON(!cpufreq_update_current_freq(policy))))
> goto unlock;
>
> - pr_debug("updating policy for CPU %u\n", cpu);
> - memcpy(&new_policy, policy, sizeof(*policy));
> - new_policy.min = policy->user_policy.min;
> - new_policy.max = policy->user_policy.max;
> -
> - cpufreq_set_policy(policy, &new_policy);
> + refresh_frequency_limits(policy);
>
> unlock:
> cpufreq_cpu_release(policy);
I don't think this is suitable for -stable.
^ permalink raw reply
* Re: [PATCH] driver core: Remove device link creation limitation
From: Marek Szyprowski @ 2019-07-16 9:29 UTC (permalink / raw)
To: Rafael J. Wysocki, Greg Kroah-Hartman
Cc: LKML, Linux PM, Saravana Kannan, Lukas Wunner, Jon Hunter,
Ulf Hansson
In-Reply-To: <3121545.4lOhFoIcdQ@kreacher>
Hi Rafael,
On 2019-07-10 12:19, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If device_link_add() is called for a consumer/supplier pair with an
> existing device link between them and the existing link's type is
> not in agreement with the flags passed to that function by its
> caller, NULL will be returned. That is seriously inconvenient,
> because it forces the callers of device_link_add() to worry about
> what others may or may not do even if that is not relevant to them
> for any other reasons.
>
> It turns out, however, that this limitation can be made go away
> relatively easily.
>
> The underlying observation is that if DL_FLAG_STATELESS has been
> passed to device_link_add() in flags for the given consumer/supplier
> pair at least once, calling either device_link_del() or
> device_link_remove() to release the link returned by it should work,
> but there are no other requirements associated with that flag. In
> turn, if at least one of the callers of device_link_add() for the
> given consumer/supplier pair has not passed DL_FLAG_STATELESS to it
> in flags, the driver core should track the status of the link and act
> on it as appropriate (ie. the link should be treated as "managed").
> This means that DL_FLAG_STATELESS needs to be set for managed device
> links and it should be valid to call device_link_del() or
> device_link_remove() to drop references to them in certain
> sutiations.
>
> To allow that to happen, introduce a new (internal) device link flag
> called DL_FLAG_MANAGED and make device_link_add() set it automatically
> whenever DL_FLAG_STATELESS is not passed to it. Also make it take
> additional references to existing device links that were previously
> stateless (that is, with DL_FLAG_STATELESS set and DL_FLAG_MANAGED
> unset) and will need to be managed going forward and initialize
> their status (which has been DL_STATE_NONE so far).
>
> Accordingly, when a managed device link is dropped automatically
> by the driver core, make it clear DL_FLAG_MANAGED, reset the link's
> status back to DL_STATE_NONE and drop the reference to it associated
> with DL_FLAG_MANAGED instead of just deleting it right away (to
> allow it to stay around in case it still needs to be released
> explicitly by someone).
>
> With that, since setting DL_FLAG_STATELESS doesn't mean that the
> device link in question is not managed any more, replace all of the
> status-tracking checks against DL_FLAG_STATELESS with analogous
> checks against DL_FLAG_MANAGED and update the documentation to
> reflect these changes.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Exynos IOMMU works fine with those changes.
> ---
> Documentation/driver-api/device_link.rst | 4
> drivers/base/core.c | 169 +++++++++++++++++--------------
> drivers/base/power/runtime.c | 4
> include/linux/device.h | 4
> 4 files changed, 102 insertions(+), 79 deletions(-)
>
> Index: linux-pm/drivers/base/core.c
> ===================================================================
> --- linux-pm.orig/drivers/base/core.c
> +++ linux-pm/drivers/base/core.c
> @@ -124,6 +124,50 @@ static int device_is_dependent(struct de
> return ret;
> }
>
> +static void device_link_init_status(struct device_link *link,
> + struct device *consumer,
> + struct device *supplier)
> +{
> + switch (supplier->links.status) {
> + case DL_DEV_PROBING:
> + switch (consumer->links.status) {
> + case DL_DEV_PROBING:
> + /*
> + * A consumer driver can create a link to a supplier
> + * that has not completed its probing yet as long as it
> + * knows that the supplier is already functional (for
> + * example, it has just acquired some resources from the
> + * supplier).
> + */
> + link->status = DL_STATE_CONSUMER_PROBE;
> + break;
> + default:
> + link->status = DL_STATE_DORMANT;
> + break;
> + }
> + break;
> + case DL_DEV_DRIVER_BOUND:
> + switch (consumer->links.status) {
> + case DL_DEV_PROBING:
> + link->status = DL_STATE_CONSUMER_PROBE;
> + break;
> + case DL_DEV_DRIVER_BOUND:
> + link->status = DL_STATE_ACTIVE;
> + break;
> + default:
> + link->status = DL_STATE_AVAILABLE;
> + break;
> + }
> + break;
> + case DL_DEV_UNBINDING:
> + link->status = DL_STATE_SUPPLIER_UNBIND;
> + break;
> + default:
> + link->status = DL_STATE_DORMANT;
> + break;
> + }
> +}
> +
> static int device_reorder_to_tail(struct device *dev, void *not_used)
> {
> struct device_link *link;
> @@ -179,9 +223,9 @@ void device_pm_move_to_tail(struct devic
> * of the link. If DL_FLAG_PM_RUNTIME is not set, DL_FLAG_RPM_ACTIVE will be
> * ignored.
> *
> - * If DL_FLAG_STATELESS is set in @flags, the link is not going to be managed by
> - * the driver core and, in particular, the caller of this function is expected
> - * to drop the reference to the link acquired by it directly.
> + * If DL_FLAG_STATELESS is set in @flags, the caller of this function is
> + * expected to release the link returned by it directly with the help of either
> + * device_link_del() or device_link_remove().
> *
> * If that flag is not set, however, the caller of this function is handing the
> * management of the link over to the driver core entirely and its return value
> @@ -201,9 +245,16 @@ void device_pm_move_to_tail(struct devic
> * be used to request the driver core to automaticall probe for a consmer
> * driver after successfully binding a driver to the supplier device.
> *
> - * The combination of DL_FLAG_STATELESS and either DL_FLAG_AUTOREMOVE_CONSUMER
> - * or DL_FLAG_AUTOREMOVE_SUPPLIER set in @flags at the same time is invalid and
> - * will cause NULL to be returned upfront.
> + * The combination of DL_FLAG_STATELESS and one of DL_FLAG_AUTOREMOVE_CONSUMER,
> + * DL_FLAG_AUTOREMOVE_SUPPLIER, or DL_FLAG_AUTOPROBE_CONSUMER set in @flags at
> + * the same time is invalid and will cause NULL to be returned upfront.
> + * However, if a device link between the given @consumer and @supplier pair
> + * exists already when this function is called for them, the existing link will
> + * be returned regardless of its current type and status (the link's flags may
> + * be modified then). The caller of this function is then expected to treat
> + * the link as though it has just been created, so (in particular) if
> + * DL_FLAG_STATELESS was passed in @flags, the link needs to be released
> + * explicitly when not needed any more (as stated above).
> *
> * A side effect of the link creation is re-ordering of dpm_list and the
> * devices_kset list by moving the consumer device and all devices depending
> @@ -223,7 +274,8 @@ struct device_link *device_link_add(stru
> (flags & DL_FLAG_STATELESS &&
> flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
> DL_FLAG_AUTOREMOVE_SUPPLIER |
> - DL_FLAG_AUTOPROBE_CONSUMER)) ||
> + DL_FLAG_AUTOPROBE_CONSUMER |
> + DL_FLAG_MANAGED)) ||
> (flags & DL_FLAG_AUTOPROBE_CONSUMER &&
> flags & (DL_FLAG_AUTOREMOVE_CONSUMER |
> DL_FLAG_AUTOREMOVE_SUPPLIER)))
> @@ -236,6 +288,9 @@ struct device_link *device_link_add(stru
> }
> }
>
> + if (!(flags & DL_FLAG_STATELESS))
> + flags |= DL_FLAG_MANAGED;
> +
> device_links_write_lock();
> device_pm_lock();
>
> @@ -262,15 +317,6 @@ struct device_link *device_link_add(stru
> if (link->consumer != consumer)
> continue;
>
> - /*
> - * Don't return a stateless link if the caller wants a stateful
> - * one and vice versa.
> - */
> - if (WARN_ON((flags & DL_FLAG_STATELESS) != (link->flags & DL_FLAG_STATELESS))) {
> - link = NULL;
> - goto out;
> - }
> -
> if (flags & DL_FLAG_PM_RUNTIME) {
> if (!(link->flags & DL_FLAG_PM_RUNTIME)) {
> pm_runtime_new_link(consumer);
> @@ -281,6 +327,7 @@ struct device_link *device_link_add(stru
> }
>
> if (flags & DL_FLAG_STATELESS) {
> + link->flags |= DL_FLAG_STATELESS;
> kref_get(&link->kref);
> goto out;
> }
> @@ -299,6 +346,11 @@ struct device_link *device_link_add(stru
> link->flags &= ~(DL_FLAG_AUTOREMOVE_CONSUMER |
> DL_FLAG_AUTOREMOVE_SUPPLIER);
> }
> + if (!(link->flags & DL_FLAG_MANAGED)) {
> + kref_get(&link->kref);
> + link->flags |= DL_FLAG_MANAGED;
> + device_link_init_status(link, consumer, supplier);
> + }
> goto out;
> }
>
> @@ -325,48 +377,10 @@ struct device_link *device_link_add(stru
> kref_init(&link->kref);
>
> /* Determine the initial link state. */
> - if (flags & DL_FLAG_STATELESS) {
> + if (flags & DL_FLAG_STATELESS)
> link->status = DL_STATE_NONE;
> - } else {
> - switch (supplier->links.status) {
> - case DL_DEV_PROBING:
> - switch (consumer->links.status) {
> - case DL_DEV_PROBING:
> - /*
> - * A consumer driver can create a link to a
> - * supplier that has not completed its probing
> - * yet as long as it knows that the supplier is
> - * already functional (for example, it has just
> - * acquired some resources from the supplier).
> - */
> - link->status = DL_STATE_CONSUMER_PROBE;
> - break;
> - default:
> - link->status = DL_STATE_DORMANT;
> - break;
> - }
> - break;
> - case DL_DEV_DRIVER_BOUND:
> - switch (consumer->links.status) {
> - case DL_DEV_PROBING:
> - link->status = DL_STATE_CONSUMER_PROBE;
> - break;
> - case DL_DEV_DRIVER_BOUND:
> - link->status = DL_STATE_ACTIVE;
> - break;
> - default:
> - link->status = DL_STATE_AVAILABLE;
> - break;
> - }
> - break;
> - case DL_DEV_UNBINDING:
> - link->status = DL_STATE_SUPPLIER_UNBIND;
> - break;
> - default:
> - link->status = DL_STATE_DORMANT;
> - break;
> - }
> - }
> + else
> + device_link_init_status(link, consumer, supplier);
>
> /*
> * Some callers expect the link creation during consumer driver probe to
> @@ -528,7 +542,7 @@ static void device_links_missing_supplie
> * mark the link as "consumer probe in progress" to make the supplier removal
> * wait for us to complete (or bad things may happen).
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> */
> int device_links_check_suppliers(struct device *dev)
> {
> @@ -538,7 +552,7 @@ int device_links_check_suppliers(struct
> device_links_write_lock();
>
> list_for_each_entry(link, &dev->links.suppliers, c_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> if (link->status != DL_STATE_AVAILABLE) {
> @@ -563,7 +577,7 @@ int device_links_check_suppliers(struct
> *
> * Also change the status of @dev's links to suppliers to "active".
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> */
> void device_links_driver_bound(struct device *dev)
> {
> @@ -572,7 +586,7 @@ void device_links_driver_bound(struct de
> device_links_write_lock();
>
> list_for_each_entry(link, &dev->links.consumers, s_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> /*
> @@ -593,7 +607,7 @@ void device_links_driver_bound(struct de
> }
>
> list_for_each_entry(link, &dev->links.suppliers, c_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> WARN_ON(link->status != DL_STATE_CONSUMER_PROBE);
> @@ -605,6 +619,13 @@ void device_links_driver_bound(struct de
> device_links_write_unlock();
> }
>
> +static void device_link_drop_managed(struct device_link *link)
> +{
> + link->flags &= ~DL_FLAG_MANAGED;
> + WRITE_ONCE(link->status, DL_STATE_NONE);
> + kref_put(&link->kref, __device_link_del);
> +}
> +
> /**
> * __device_links_no_driver - Update links of a device without a driver.
> * @dev: Device without a drvier.
> @@ -615,18 +636,18 @@ void device_links_driver_bound(struct de
> * unless they already are in the "supplier unbind in progress" state in which
> * case they need not be updated.
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> */
> static void __device_links_no_driver(struct device *dev)
> {
> struct device_link *link, *ln;
>
> list_for_each_entry_safe_reverse(link, ln, &dev->links.suppliers, c_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> if (link->flags & DL_FLAG_AUTOREMOVE_CONSUMER)
> - __device_link_del(&link->kref);
> + device_link_drop_managed(link);
> else if (link->status == DL_STATE_CONSUMER_PROBE ||
> link->status == DL_STATE_ACTIVE)
> WRITE_ONCE(link->status, DL_STATE_AVAILABLE);
> @@ -643,7 +664,7 @@ static void __device_links_no_driver(str
> * %__device_links_no_driver() to update links to suppliers for it as
> * appropriate.
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> */
> void device_links_no_driver(struct device *dev)
> {
> @@ -652,7 +673,7 @@ void device_links_no_driver(struct devic
> device_links_write_lock();
>
> list_for_each_entry(link, &dev->links.consumers, s_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> /*
> @@ -680,7 +701,7 @@ void device_links_no_driver(struct devic
> * invoke %__device_links_no_driver() to update links to suppliers for it as
> * appropriate.
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> */
> void device_links_driver_cleanup(struct device *dev)
> {
> @@ -689,7 +710,7 @@ void device_links_driver_cleanup(struct
> device_links_write_lock();
>
> list_for_each_entry_safe(link, ln, &dev->links.consumers, s_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> WARN_ON(link->flags & DL_FLAG_AUTOREMOVE_CONSUMER);
> @@ -702,7 +723,7 @@ void device_links_driver_cleanup(struct
> */
> if (link->status == DL_STATE_SUPPLIER_UNBIND &&
> link->flags & DL_FLAG_AUTOREMOVE_SUPPLIER)
> - __device_link_del(&link->kref);
> + device_link_drop_managed(link);
>
> WRITE_ONCE(link->status, DL_STATE_DORMANT);
> }
> @@ -724,7 +745,7 @@ void device_links_driver_cleanup(struct
> *
> * Return 'false' if there are no probing or active consumers.
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> */
> bool device_links_busy(struct device *dev)
> {
> @@ -734,7 +755,7 @@ bool device_links_busy(struct device *de
> device_links_write_lock();
>
> list_for_each_entry(link, &dev->links.consumers, s_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> if (link->status == DL_STATE_CONSUMER_PROBE
> @@ -764,7 +785,7 @@ bool device_links_busy(struct device *de
> * driver to unbind and start over (the consumer will not re-probe as we have
> * changed the state of the link already).
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> */
> void device_links_unbind_consumers(struct device *dev)
> {
> @@ -776,7 +797,7 @@ void device_links_unbind_consumers(struc
> list_for_each_entry(link, &dev->links.consumers, s_node) {
> enum device_link_state status;
>
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> status = link->status;
> Index: linux-pm/drivers/base/power/runtime.c
> ===================================================================
> --- linux-pm.orig/drivers/base/power/runtime.c
> +++ linux-pm/drivers/base/power/runtime.c
> @@ -1624,7 +1624,7 @@ void pm_runtime_remove(struct device *de
> * runtime PM references to the device, drop the usage counter of the device
> * (as many times as needed).
> *
> - * Links with the DL_FLAG_STATELESS flag set are ignored.
> + * Links with the DL_FLAG_MANAGED flag unset are ignored.
> *
> * Since the device is guaranteed to be runtime-active at the point this is
> * called, nothing else needs to be done here.
> @@ -1641,7 +1641,7 @@ void pm_runtime_clean_up_links(struct de
> idx = device_links_read_lock();
>
> list_for_each_entry_rcu(link, &dev->links.consumers, s_node) {
> - if (link->flags & DL_FLAG_STATELESS)
> + if (!(link->flags & DL_FLAG_MANAGED))
> continue;
>
> while (refcount_dec_not_one(&link->rpm_active))
> Index: linux-pm/include/linux/device.h
> ===================================================================
> --- linux-pm.orig/include/linux/device.h
> +++ linux-pm/include/linux/device.h
> @@ -829,12 +829,13 @@ enum device_link_state {
> /*
> * Device link flags.
> *
> - * STATELESS: The core won't track the presence of supplier/consumer drivers.
> + * STATELESS: The core will not remove this link automatically.
> * AUTOREMOVE_CONSUMER: Remove the 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_SUPPLIER: Remove the link automatically on supplier driver unbind.
> * AUTOPROBE_CONSUMER: Probe consumer driver automatically after supplier binds.
> + * MANAGED: The core tracks presence of supplier/consumer drivers (internal).
> */
> #define DL_FLAG_STATELESS BIT(0)
> #define DL_FLAG_AUTOREMOVE_CONSUMER BIT(1)
> @@ -842,6 +843,7 @@ enum device_link_state {
> #define DL_FLAG_RPM_ACTIVE BIT(3)
> #define DL_FLAG_AUTOREMOVE_SUPPLIER BIT(4)
> #define DL_FLAG_AUTOPROBE_CONSUMER BIT(5)
> +#define DL_FLAG_MANAGED BIT(6)
>
> /**
> * struct device_link - Device link representation.
> Index: linux-pm/Documentation/driver-api/device_link.rst
> ===================================================================
> --- linux-pm.orig/Documentation/driver-api/device_link.rst
> +++ linux-pm/Documentation/driver-api/device_link.rst
> @@ -78,8 +78,8 @@ typically deleted in its ``->remove`` ca
> driver is compiled as a module, the device link is added on module load and
> orderly deleted on unload. The same restrictions that apply to device link
> addition (e.g. exclusion of a parallel suspend/resume transition) apply equally
> -to deletion. Device links with ``DL_FLAG_STATELESS`` unset (i.e. managed
> -device links) are deleted automatically by the driver core.
> +to deletion. Device links managed by the driver core are deleted automatically
> +by it.
>
> Several flags may be specified on device link addition, two of which
> have already been mentioned above: ``DL_FLAG_STATELESS`` to express that no
>
>
>
>
>
Best regards
--
Marek Szyprowski, PhD
Samsung R&D Institute Poland
^ permalink raw reply
* Re: [PATCH v4] PM / wakeup: show wakeup sources stats in sysfs
From: Rafael J. Wysocki @ 2019-07-16 9:36 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Rafael J. Wysocki, Tri Vo, Rafael J. Wysocki, Viresh Kumar,
Hridya Valsaraju, sspatil, kaleshsingh, Linux Kernel Mailing List,
Linux PM, Cc: Android Kernel
In-Reply-To: <20190716083926.GA2505@kroah.com>
On Tue, Jul 16, 2019 at 10:39 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jul 16, 2019 at 10:30:48AM +0200, Rafael J. Wysocki wrote:
> > On Tue, Jul 16, 2019 at 4:13 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Mon, Jul 15, 2019 at 11:48:27PM +0200, Rafael J. Wysocki wrote:
> > > > On Mon, Jul 15, 2019 at 11:44 PM Tri Vo <trong@android.com> wrote:
> > > > >
> > > > > Userspace can use wakeup_sources debugfs node to plot history of suspend
> > > > > blocking wakeup sources over device's boot cycle. This information can
> > > > > then be used (1) for power-specific bug reporting and (2) towards
> > > > > attributing battery consumption to specific processes over a period of
> > > > > time.
> > > > >
> > > > > However, debugfs doesn't have stable ABI. For this reason, create a
> > > > > 'struct device' to expose wakeup sources statistics in sysfs under
> > > > > /sys/class/wakeup/<name>/.
> > > > >
> > > > > Introduce CONFIG_PM_SLEEP_STATS that enables/disables showing wakeup
> > > > > source statistics in sysfs.
> > > >
> > > > I'm not sure if this is really needed, but I'll let Greg decide.
> > >
> > > You are right. Having zillions of config options is a pain, who is
> > > going to turn this off?
> > >
> > > But we can always remove the option before 5.4-rc1, so I'll take this
> > > as-is for now :)
> > >
> > > > Apart from this
> > > >
> > > > Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >
> > > thanks for the review! I'll wait for 5.3-rc1 to come out before adding
> > > this to my tree.
> >
> > So it occurred to me that maybe it's better if I apply it? After all,
> > this is PM material. :-)
>
> Heh, true, feel free to add:
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> to the patch then.
I will, thank you!
^ permalink raw reply
* [PATCH] PM / Domains: Align in-parameter names for some genpd functions
From: Ulf Hansson @ 2019-07-16 9:43 UTC (permalink / raw)
To: Rafael J . Wysocki, linux-pm; +Cc: Ulf Hansson, Viresh Kumar, Kevin Hilman
Align in-parameter names for the declarations of pm_genpd_add|
remove_subdomain() and of_genpd_add_subdomain() according to their
implementations, as to improve consistency.
Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
include/linux/pm_domain.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 91d9bf497071..baf02ff91a31 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -197,9 +197,9 @@ static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev);
int pm_genpd_remove_device(struct device *dev);
int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
- struct generic_pm_domain *new_subdomain);
+ struct generic_pm_domain *subdomain);
int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
- struct generic_pm_domain *target);
+ struct generic_pm_domain *subdomain);
int pm_genpd_init(struct generic_pm_domain *genpd,
struct dev_power_governor *gov, bool is_off);
int pm_genpd_remove(struct generic_pm_domain *genpd);
@@ -226,12 +226,12 @@ static inline int pm_genpd_remove_device(struct device *dev)
return -ENOSYS;
}
static inline int pm_genpd_add_subdomain(struct generic_pm_domain *genpd,
- struct generic_pm_domain *new_sd)
+ struct generic_pm_domain *subdomain)
{
return -ENOSYS;
}
static inline int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
- struct generic_pm_domain *target)
+ struct generic_pm_domain *subdomain)
{
return -ENOSYS;
}
@@ -282,8 +282,8 @@ int of_genpd_add_provider_onecell(struct device_node *np,
struct genpd_onecell_data *data);
void of_genpd_del_provider(struct device_node *np);
int of_genpd_add_device(struct of_phandle_args *args, struct device *dev);
-int of_genpd_add_subdomain(struct of_phandle_args *parent,
- struct of_phandle_args *new_subdomain);
+int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
+ struct of_phandle_args *subdomain_spec);
struct generic_pm_domain *of_genpd_remove_last(struct device_node *np);
int of_genpd_parse_idle_states(struct device_node *dn,
struct genpd_power_state **states, int *n);
@@ -316,8 +316,8 @@ static inline int of_genpd_add_device(struct of_phandle_args *args,
return -ENODEV;
}
-static inline int of_genpd_add_subdomain(struct of_phandle_args *parent,
- struct of_phandle_args *new_subdomain)
+static inline int of_genpd_add_subdomain(struct of_phandle_args *parent_spec,
+ struct of_phandle_args *subdomain_spec)
{
return -ENODEV;
}
--
2.17.1
^ 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