* [PATCH V11 0/7] PM / Domains: Performance state support
@ 2017-10-11 7:24 Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 1/7] PM / Domains: Add support to select performance-state of domains Viresh Kumar
` (7 more replies)
0 siblings, 8 replies; 16+ messages in thread
From: Viresh Kumar @ 2017-10-11 7:24 UTC (permalink / raw)
To: Rafael Wysocki, ulf.hansson, Kevin Hilman, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Nishanth Menon, robh+dt,
lina.iyer, rnayak, sudeep.holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
Hi,
This version contains the changes we discussed during Linaro Connect.
Some platforms have the capability to configure the performance state of
their power domains. The process of configuring the performance state is
pretty much platform dependent and we may need to work with a wide range
of configurables. For some platforms, like Qcom, it can be a positive
integer value alone, while in other cases it can be voltage levels, etc.
The power-domain framework until now was only designed for the idle
state management of the device and this needs to change in order to
reuse the power-domain framework for active state management of the
devices.
The first patch updates the genpd framework to supply new APIs to
support active state management and the second patch uses them from the
OPP core. The third patch adds a new API to the OPP core to get
performance state corresponding to OPPs (This should rather come via DT
and would be removed once we have fixed bindings for performance
states).
Rest of the patches [4-7/7] are included to show how user drivers would
end up using the new APIs and these patches aren't ready to get merged
yet and are marked clearly like that. Moreover some of them may go via
SoC specific trees instead of the PM tree.
This is currently tested by:
- /me by hacking the kernel a bit with virtual power-domains for the ARM
64 hikey platform.
- Rajendra Nayak, on msm8996 platform (Qcom) with MMC controller.
Thanks Rajendra for helping me testing this out.
I also had a chat with Rajendra and we should be able to get a Qualcomm
specific power domain driver (which uses these changes) in coming weeks.
I am targeting the first 3 patches for 4.15-rc1, if possible.
Pushed here as well:
https://git.linaro.org/people/viresh.kumar/linux.git/log/?h=opp/genpd-performance-state
Rebased over: pm/linux-next
V10->V11:
- Dropped propagation to master domains of a subdomain, as we don't have
such requirements yet and we may not have 1 to 1 relation between the
states of subdomains and masters.
- Updated genpd API to get performance state directly instead of
frequency. The conversion of frequency to performance state is now
done by OPP core instead, which is more logical.
- Only the mmc patch is kept and other user patches are dropped, anyway
none of those is getting merged right now.
- Updates to the rpm power domain driver and a new OPP driver is added
as well.
V9->V10:
- Performance state of masters is updated before the state of the genpd
(Ulf).
- 2/7 and 3/7 are swapped.
V8->V9:
- Renamed genpd callbacks and internal routines.
- dev_pm_genpd_has_performance_state() simplified a lot and doesn't
check master hierarchy now. Rather a new patch (2/7) is added to
take care of that and WARN if no master has set
genpd_set_performance_state() callback.
- Update is propagated to the masters even if the genpd's callback is
already called.
- Exit _genpd_reeval_performance_state() early if no state change is
required and it gets an additional argument (new state of the
device/subdomain).
- Taken care of genpd on/off cases.
- s/parent/master everywhere in comments and logs.
- Better explanations in logs, comments etc.
- All the other patches (3-7/7) are same as V8. (Just minor update in
5/7 to use the updated callback names).
V7->V8:
- Ulf helped a lot in reviewing V7 and pointed out couple of issues,
specially in locking while dealing with a hierarchy of power domains.
- All those locking issues are sorted out now, even for the complex
cases.
- genpd_lookup_dev() is used in pm_genpd_has_performance_state() to make
sure we have a valid genpd available for the device.
- Validation of performance state callbacks isn't done anymore in
pm_genpd_init() as it gets called very early and the binding of
subdomains to their parent domains happens later. This is handled in
pm_genpd_has_performance_state() now, which is called from user
drivers.
- User driver changes (not to be merged) are included for the first time
here, to demonstrate how changes would look finally.
V6->V7:
- Almost a rewrite, only two patches against 9 in earlier version.
- No bindings updated now and domain's performance state aren't passed
via DT for now (until we know how users are going to use it).
- We also skipped the QoS framework completely and new APIs are provided
directly by genpd.
V5->V6:
- Use freq/voltage in OPP table as it is for power domain and don't
create "domain-performance-level" property
- Create new "power-domain-opp" property for the devices.
- Take care of domain providers that provide multiple domains and extend
"operating-points-v2" property to contain a list of phandles
- Update code according to those bindings.
V4->V5:
- Only 3 patches were resent and 2 of them are Acked from Ulf.
V3->V4:
- Use OPP table for genpd devices as well.
- Add struct device to genpd, in order to reuse OPP infrastructure.
- Based over: https://marc.info/?l=linux-kernel&m=148972988002317&w=2
- Fixed examples in DT document to have voltage in target,min,max order.
V2->V3:
- Based over latest pm/linux-next
- Bindings and code are merged together
- Lots of updates in bindings
- the performance-states node is present within the power-domain now,
instead of its phandle.
- performance-level property is replaced by "reg".
- domain-performance-state property of the consumers contain an
integer value now instead of phandle.
- Lots of updates to the code as well
- Patch "PM / QOS: Add default case to the switch" is merged with
other patches and the code is changed a bit as well.
- Don't pass 'type' to dev_pm_qos_add_notifier(), rather handle all
notifiers with a single list. A new patch is added for that.
- The OPP framework patch can be applied now and has proper SoB from
me.
- Dropped "PM / domain: Save/restore performance state at runtime
suspend/resume".
- Drop all WARN().
- Tested-by Rajendra nayak.
V1->V2:
- Based over latest pm/linux-next
- It is mostly a resend of what is sent earlier as this series hasn't
got any reviews so far and Rafael suggested that its better I resend
it.
- Only the 4/6 patch got an update, which was shared earlier as reply to
V1 as well. It has got several fixes for taking care of power domain
hierarchy, etc.
--
viresh
Rajendra Nayak (3):
soc: qcom: rpmpd: Add driver to model cx/mx power domains
soc: qcom: rpmpd: Add support for set performance state
mmc: sdhci-msm: Adapt the driver to use OPPs to set clocks/performance
state
Viresh Kumar (4):
PM / Domains: Add support to select performance-state of domains
OPP: Support updating performance state of device's power domain
OPP: Add dev_pm_opp_{un}register_get_pstate_helper()
OPP: qcom: Add support to get performance states corresponding to OPPs
.../devicetree/bindings/power/qcom,rpmpd.txt | 9 +
arch/arm64/boot/dts/qcom/msm8996.dtsi | 39 +++
drivers/base/power/domain.c | 179 ++++++++++-
drivers/clk/qcom/gcc-msm8996.c | 8 +-
drivers/mmc/host/sdhci-msm.c | 39 ++-
drivers/opp/Makefile | 2 +
drivers/opp/core.c | 135 ++++++++-
drivers/opp/debugfs.c | 3 +
drivers/opp/opp.h | 6 +
drivers/opp/qcom-rpmpd.c | 107 +++++++
drivers/soc/qcom/Kconfig | 9 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/rpmpd.c | 330 +++++++++++++++++++++
include/linux/pm_domain.h | 13 +
include/linux/pm_opp.h | 10 +
15 files changed, 873 insertions(+), 17 deletions(-)
create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt
create mode 100644 drivers/opp/qcom-rpmpd.c
create mode 100644 drivers/soc/qcom/rpmpd.c
--
2.7.4
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V11 1/7] PM / Domains: Add support to select performance-state of domains
2017-10-11 7:24 [PATCH V11 0/7] PM / Domains: Performance state support Viresh Kumar
@ 2017-10-11 7:24 ` Viresh Kumar
2017-10-11 11:27 ` Ulf Hansson
` (2 more replies)
2017-10-11 7:24 ` [PATCH V11 2/7] OPP: Support updating performance state of device's power domain Viresh Kumar
` (6 subsequent siblings)
7 siblings, 3 replies; 16+ messages in thread
From: Viresh Kumar @ 2017-10-11 7:24 UTC (permalink / raw)
To: Rafael Wysocki, ulf.hansson, Kevin Hilman, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Nishanth Menon, robh+dt,
lina.iyer, rnayak, sudeep.holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
Some platforms have the capability to configure the performance state of
PM domains. This patch enhances the genpd core to support such
platforms.
The performance levels (within the genpd core) are identified by
positive integer values, a lower value represents lower performance
state.
This patch adds a new genpd API, which is called by user drivers (like
OPP framework):
- int dev_pm_genpd_set_performance_state(struct device *dev,
unsigned int state);
This updates the performance state constraint of the device on its PM
domain. On success, the genpd will have its performance state set to a
value which is >= "state" passed to this routine. The genpd core calls
the genpd->genpd_set_performance_state() callback, if implemented,
else -ENODEV is returned to the caller.
The PM domain drivers need to implement the following callback if they
want to support performance states.
- int (*genpd_set_performance_state)(struct generic_pm_domain *genpd,
unsigned int state);
This is called internally by the genpd core on several occasions. The
genpd core passes the genpd pointer and the aggregate of the
performance states of the devices supported by that genpd to this
callback. This callback must update the performance state of the genpd
in a platform dependent way.
The power domains can avoid supplying above callback, if they don't
support setting performance-states.
A TODO comment is also added to _genpd_reeval_performance_state(). This
feature will be required once we have hardware that needs to propagate
the performance state changes to master domains.
Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/base/power/domain.c | 179 +++++++++++++++++++++++++++++++++++++++++++-
include/linux/pm_domain.h | 13 ++++
2 files changed, 190 insertions(+), 2 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a6e4c8d7d837..b8360bc6a8eb 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -237,6 +237,169 @@ static void genpd_update_accounting(struct generic_pm_domain *genpd)
static inline void genpd_update_accounting(struct generic_pm_domain *genpd) {}
#endif
+/* Returns negative errors or 0 on success */
+static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
+ int state)
+{
+ int ret;
+
+ ret = genpd->genpd_set_performance_state(genpd, state);
+ if (!ret)
+ genpd->performance_state = state;
+
+ return ret;
+}
+
+/*
+ * Re-evaluate performance state of a genpd. Finds the highest requested
+ * performance state by the devices within the genpd and then change genpd's
+ * performance state (if required).
+ *
+ * @genpd: PM domain whose state needs to be reevaluated.
+ * @state: Newly requested performance state of the device for which this
+ * routine is called.
+ *
+ * Returns negative errors or 0 on success.
+ */
+static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
+ int state)
+{
+ struct generic_pm_domain_data *pd_data;
+ struct pm_domain_data *pdd;
+
+ /* New requested state is same as Max requested state */
+ if (state == genpd->performance_state)
+ return 0;
+
+ /* New requested state is higher than Max requested state */
+ if (state > genpd->performance_state)
+ goto update_state;
+
+ /* Traverse all devices within the domain */
+ list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+ pd_data = to_gpd_data(pdd);
+
+ if (pd_data->performance_state > state)
+ state = pd_data->performance_state;
+ }
+
+ /*
+ * TODO: Traverse all subdomains of the genpd. This will be
+ * required once we have hardware that needs to propagate the
+ * performance state changes.
+ */
+
+update_state:
+ return _genpd_set_performance_state(genpd, state);
+}
+
+static void __genpd_dev_update_performance_state(struct device *dev, int state)
+{
+ struct generic_pm_domain_data *gpd_data;
+
+ spin_lock_irq(&dev->power.lock);
+
+ if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data) {
+ WARN_ON(1);
+ goto unlock;
+ }
+
+ gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+ gpd_data->performance_state = state;
+
+unlock:
+ spin_unlock_irq(&dev->power.lock);
+}
+
+/**
+ * dev_pm_genpd_set_performance_state- Set performance state of device's power
+ * domain.
+ *
+ * @dev: Device for which the performance-state needs to be adjusted.
+ * @state: Target performance state of the device. This can be set as 0 when the
+ * device doesn't have any performance state constraints left (And so
+ * the device wouldn't participate anymore to find the target
+ * performance state of the genpd).
+ *
+ * It is assumed that the user driver guarantees that the genpd wouldn't be
+ * detached while this routine is getting called.
+ *
+ * Returns 0 on success and negative error values on failures.
+ */
+int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
+{
+ struct generic_pm_domain *genpd;
+ int ret;
+
+ genpd = dev_to_genpd(dev);
+ if (IS_ERR(genpd))
+ return -ENODEV;
+
+ genpd_lock(genpd);
+
+ if (!genpd->genpd_set_performance_state) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+
+ if (!genpd_status_on(genpd)) {
+ ret = -EBUSY;
+ goto unlock;
+ }
+
+ ret = _genpd_reeval_performance_state(genpd, state);
+ if (!ret) {
+ /*
+ * Since we are passing "state" as well to
+ * _genpd_reeval_performance_state(), we don't need to call
+ * __genpd_dev_update_performance_state() before updating
+ * genpd's state with the above call. Update it only after the
+ * state of master domain is updated.
+ */
+ __genpd_dev_update_performance_state(dev, state);
+ }
+
+unlock:
+ genpd_unlock(genpd);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
+
+static int _genpd_on_update_performance_state(struct generic_pm_domain *genpd)
+{
+ int ret, prev = genpd->prev_performance_state;
+
+ if (likely(!prev))
+ return 0;
+
+ ret = _genpd_set_performance_state(genpd, prev);
+ if (ret) {
+ pr_err("%s: Failed to restore performance state to %d (%d)\n",
+ genpd->name, prev, ret);
+ } else {
+ genpd->prev_performance_state = 0;
+ }
+
+ return ret;
+}
+
+static void _genpd_off_update_performance_state(struct generic_pm_domain *genpd)
+{
+ int ret, state = genpd->performance_state;
+
+ if (likely(!state))
+ return;
+
+ ret = _genpd_set_performance_state(genpd, 0);
+ if (ret) {
+ pr_err("%s: Failed to set performance state to 0 (%d)\n",
+ genpd->name, ret);
+ } else {
+ genpd->prev_performance_state = state;
+ }
+}
+
static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
{
unsigned int state_idx = genpd->state_idx;
@@ -388,6 +551,8 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
return ret;
}
+ _genpd_off_update_performance_state(genpd);
+
genpd->status = GPD_STATE_POWER_OFF;
genpd_update_accounting(genpd);
@@ -437,15 +602,21 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
}
}
- ret = _genpd_power_on(genpd, true);
+ ret = _genpd_on_update_performance_state(genpd);
if (ret)
goto err;
+ ret = _genpd_power_on(genpd, true);
+ if (ret)
+ goto err_power_on;
+
genpd->status = GPD_STATE_ACTIVE;
genpd_update_accounting(genpd);
return 0;
+ err_power_on:
+ _genpd_off_update_performance_state(genpd);
err:
list_for_each_entry_continue_reverse(link,
&genpd->slave_links,
@@ -803,6 +974,8 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
if (_genpd_power_off(genpd, false))
return;
+ _genpd_off_update_performance_state(genpd);
+
genpd->status = GPD_STATE_POWER_OFF;
list_for_each_entry(link, &genpd->slave_links, slave_node) {
@@ -848,7 +1021,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
genpd_unlock(link->master);
}
- _genpd_power_on(genpd, false);
+ if (!_genpd_on_update_performance_state(genpd))
+ if (_genpd_power_on(genpd, false))
+ _genpd_off_update_performance_state(genpd);
genpd->status = GPD_STATE_ACTIVE;
}
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 84f423d5633e..81d923f058fd 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -64,8 +64,12 @@ struct generic_pm_domain {
unsigned int device_count; /* Number of devices */
unsigned int suspended_count; /* System suspend device counter */
unsigned int prepared_count; /* Suspend counter of prepared devices */
+ unsigned int performance_state; /* Max requested performance state */
+ unsigned int prev_performance_state; /* Performance state before power off */
int (*power_off)(struct generic_pm_domain *domain);
int (*power_on)(struct generic_pm_domain *domain);
+ int (*genpd_set_performance_state)(struct generic_pm_domain *genpd,
+ unsigned int state);
struct gpd_dev_ops dev_ops;
s64 max_off_time_ns; /* Maximum allowed "suspended" time. */
bool max_off_time_changed;
@@ -121,6 +125,7 @@ struct generic_pm_domain_data {
struct pm_domain_data base;
struct gpd_timing_data td;
struct notifier_block nb;
+ unsigned int performance_state;
void *data;
};
@@ -148,6 +153,8 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
extern int pm_genpd_init(struct generic_pm_domain *genpd,
struct dev_power_governor *gov, bool is_off);
extern int pm_genpd_remove(struct generic_pm_domain *genpd);
+extern int dev_pm_genpd_set_performance_state(struct device *dev,
+ unsigned int state);
extern struct dev_power_governor simple_qos_governor;
extern struct dev_power_governor pm_domain_always_on_gov;
@@ -188,6 +195,12 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
return -ENOTSUPP;
}
+static inline int dev_pm_genpd_set_performance_state(struct device *dev,
+ unsigned int state)
+{
+ return -ENOTSUPP;
+}
+
#define simple_qos_governor (*(struct dev_power_governor *)(NULL))
#define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
#endif
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH V11 2/7] OPP: Support updating performance state of device's power domain
2017-10-11 7:24 [PATCH V11 0/7] PM / Domains: Performance state support Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 1/7] PM / Domains: Add support to select performance-state of domains Viresh Kumar
@ 2017-10-11 7:24 ` Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 3/7] OPP: Add dev_pm_opp_{un}register_get_pstate_helper() Viresh Kumar
` (5 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2017-10-11 7:24 UTC (permalink / raw)
To: Rafael Wysocki, ulf.hansson, Kevin Hilman, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Nishanth Menon, robh+dt,
lina.iyer, rnayak, sudeep.holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
The genpd framework now provides an API to request device's power domain
to update its performance state. Use that interface from the OPP core
for devices whose power domains support performance states.
Note that this commit doesn't add any mechanism by which performance
states are made available to the OPP core. That would be done by a
later commit.
Note that the current implementation is restricted to the case where the
device doesn't have separate regulators for itself. We shouldn't
over engineer the code before we have real use case for them. We can
always come back and add more code to support such cases later on.
Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++-
drivers/opp/debugfs.c | 3 +++
drivers/opp/opp.h | 4 ++++
3 files changed, 63 insertions(+), 1 deletion(-)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 80c21207e48c..0ce8069d6843 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -19,6 +19,7 @@
#include <linux/slab.h>
#include <linux/device.h>
#include <linux/export.h>
+#include <linux/pm_domain.h>
#include <linux/regulator/consumer.h>
#include "opp.h"
@@ -535,6 +536,44 @@ _generic_set_opp_clk_only(struct device *dev, struct clk *clk,
return ret;
}
+static inline int
+_generic_set_opp_domain(struct device *dev, struct clk *clk,
+ unsigned long old_freq, unsigned long freq,
+ unsigned int old_pstate, unsigned int new_pstate)
+{
+ int ret;
+
+ /* Scaling up? Scale domain performance state before frequency */
+ if (freq > old_freq) {
+ ret = dev_pm_genpd_set_performance_state(dev, new_pstate);
+ if (ret)
+ return ret;
+ }
+
+ ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
+ if (ret)
+ goto restore_domain_state;
+
+ /* Scaling down? Scale domain performance state after frequency */
+ if (freq < old_freq) {
+ ret = dev_pm_genpd_set_performance_state(dev, new_pstate);
+ if (ret)
+ goto restore_freq;
+ }
+
+ return 0;
+
+restore_freq:
+ if (_generic_set_opp_clk_only(dev, clk, freq, old_freq))
+ dev_err(dev, "%s: failed to restore old-freq (%lu Hz)\n",
+ __func__, old_freq);
+restore_domain_state:
+ if (freq > old_freq)
+ dev_pm_genpd_set_performance_state(dev, old_pstate);
+
+ return ret;
+}
+
static int _generic_set_opp_regulator(const struct opp_table *opp_table,
struct device *dev,
unsigned long old_freq,
@@ -653,7 +692,16 @@ int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq)
/* Only frequency scaling */
if (!opp_table->regulators) {
- ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
+ /*
+ * We don't support devices with both regulator and
+ * domain performance-state for now.
+ */
+ if (opp_table->genpd_performance_state)
+ ret = _generic_set_opp_domain(dev, clk, old_freq, freq,
+ IS_ERR(old_opp) ? 0 : old_opp->pstate,
+ opp->pstate);
+ else
+ ret = _generic_set_opp_clk_only(dev, clk, old_freq, freq);
} else if (!opp_table->set_opp) {
ret = _generic_set_opp_regulator(opp_table, dev, old_freq, freq,
IS_ERR(old_opp) ? NULL : old_opp->supplies,
@@ -1706,6 +1754,13 @@ void _dev_pm_opp_remove_table(struct opp_table *opp_table, struct device *dev,
if (remove_all || !opp->dynamic)
dev_pm_opp_put(opp);
}
+
+ /*
+ * The OPP table is getting removed, drop the performance state
+ * constraints.
+ */
+ if (opp_table->genpd_performance_state)
+ dev_pm_genpd_set_performance_state(dev, 0);
} else {
_remove_opp_dev(_find_opp_dev(dev, opp_table), opp_table);
}
diff --git a/drivers/opp/debugfs.c b/drivers/opp/debugfs.c
index 9318848f3c67..b03c03576a62 100644
--- a/drivers/opp/debugfs.c
+++ b/drivers/opp/debugfs.c
@@ -99,6 +99,9 @@ int opp_debug_create_one(struct dev_pm_opp *opp, struct opp_table *opp_table)
if (!debugfs_create_bool("suspend", S_IRUGO, d, &opp->suspend))
return -ENOMEM;
+ if (!debugfs_create_u32("performance_state", S_IRUGO, d, &opp->pstate))
+ return -ENOMEM;
+
if (!debugfs_create_ulong("rate_hz", S_IRUGO, d, &opp->rate))
return -ENOMEM;
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index 166eef990599..e8f767ab5814 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -58,6 +58,7 @@ extern struct list_head opp_tables;
* @dynamic: not-created from static DT entries.
* @turbo: true if turbo (boost) OPP
* @suspend: true if suspend OPP
+ * @pstate: Device's power domain's performance state.
* @rate: Frequency in hertz
* @supplies: Power supplies voltage/current values
* @clock_latency_ns: Latency (in nanoseconds) of switching to this OPP's
@@ -76,6 +77,7 @@ struct dev_pm_opp {
bool dynamic;
bool turbo;
bool suspend;
+ unsigned int pstate;
unsigned long rate;
struct dev_pm_opp_supply *supplies;
@@ -135,6 +137,7 @@ enum opp_table_access {
* @clk: Device's clock handle
* @regulators: Supply regulators
* @regulator_count: Number of power supply regulators
+ * @genpd_performance_state: Device's power domain support performance state.
* @set_opp: Platform specific set_opp callback
* @set_opp_data: Data to be passed to set_opp callback
* @dentry: debugfs dentry pointer of the real device directory (not links).
@@ -170,6 +173,7 @@ struct opp_table {
struct clk *clk;
struct regulator **regulators;
unsigned int regulator_count;
+ bool genpd_performance_state;
int (*set_opp)(struct dev_pm_set_opp_data *data);
struct dev_pm_set_opp_data *set_opp_data;
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH V11 3/7] OPP: Add dev_pm_opp_{un}register_get_pstate_helper()
2017-10-11 7:24 [PATCH V11 0/7] PM / Domains: Performance state support Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 1/7] PM / Domains: Add support to select performance-state of domains Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 2/7] OPP: Support updating performance state of device's power domain Viresh Kumar
@ 2017-10-11 7:24 ` Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 4/7] soc: qcom: rpmpd: Add driver to model cx/mx power domains Viresh Kumar
` (4 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2017-10-11 7:24 UTC (permalink / raw)
To: Rafael Wysocki, ulf.hansson, Kevin Hilman, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Nishanth Menon, robh+dt,
lina.iyer, rnayak, sudeep.holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
This adds the dev_pm_opp_{un}register_get_pstate_helper() helper
routines which will be used to set the get_pstate() callback for a
device. This callback will be later called internally by the OPP core to
get performance state corresponding to an OPP.
This is required temporarily until the time we have proper DT bindings
to include the performance state information.
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/core.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++++++
drivers/opp/opp.h | 2 ++
include/linux/pm_opp.h | 10 +++++++
3 files changed, 90 insertions(+)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0ce8069d6843..92fa94a6dcc1 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1036,6 +1036,9 @@ int _opp_add(struct device *dev, struct dev_pm_opp *new_opp,
return ret;
}
+ if (opp_table->get_pstate)
+ new_opp->pstate = opp_table->get_pstate(dev, new_opp->rate);
+
list_add(&new_opp->node, head);
mutex_unlock(&opp_table->lock);
@@ -1548,6 +1551,81 @@ void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table)
EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_set_opp_helper);
/**
+ * dev_pm_opp_register_get_pstate_helper() - Register get_pstate() helper.
+ * @dev: Device for which the helper is getting registered.
+ * @get_pstate: Helper.
+ *
+ * TODO: Remove this callback after the same information is available via Device
+ * Tree.
+ *
+ * This allows a platform to initialize the performance states of individual
+ * OPPs for its devices, until we get similar information directly from DT.
+ *
+ * This must be called before the OPPs are initialized for the device.
+ */
+struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev,
+ int (*get_pstate)(struct device *dev, unsigned long rate))
+{
+ struct opp_table *opp_table;
+ int ret;
+
+ if (!get_pstate)
+ return ERR_PTR(-EINVAL);
+
+ opp_table = dev_pm_opp_get_opp_table(dev);
+ if (!opp_table)
+ return ERR_PTR(-ENOMEM);
+
+ /* This should be called before OPPs are initialized */
+ if (WARN_ON(!list_empty(&opp_table->opp_list))) {
+ ret = -EBUSY;
+ goto err;
+ }
+
+ /* Already have genpd_performance_state set */
+ if (WARN_ON(opp_table->genpd_performance_state)) {
+ ret = -EBUSY;
+ goto err;
+ }
+
+ opp_table->genpd_performance_state = true;
+ opp_table->get_pstate = get_pstate;
+
+ return opp_table;
+
+err:
+ dev_pm_opp_put_opp_table(opp_table);
+
+ return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_register_get_pstate_helper);
+
+/**
+ * dev_pm_opp_unregister_get_pstate_helper() - Releases resources blocked for
+ * get_pstate() helper
+ * @opp_table: OPP table returned from dev_pm_opp_register_get_pstate_helper().
+ *
+ * Release resources blocked for platform specific get_pstate() helper.
+ */
+void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table)
+{
+ if (!opp_table->genpd_performance_state) {
+ pr_err("%s: Doesn't have performance states set\n",
+ __func__);
+ return;
+ }
+
+ /* Make sure there are no concurrent readers while updating opp_table */
+ WARN_ON(!list_empty(&opp_table->opp_list));
+
+ opp_table->genpd_performance_state = false;
+ opp_table->get_pstate = NULL;
+
+ dev_pm_opp_put_opp_table(opp_table);
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_unregister_get_pstate_helper);
+
+/**
* dev_pm_opp_add() - Add an OPP table from a table definitions
* @dev: device for which we do this operation
* @freq: Frequency in Hz for this OPP
diff --git a/drivers/opp/opp.h b/drivers/opp/opp.h
index e8f767ab5814..4d00061648a3 100644
--- a/drivers/opp/opp.h
+++ b/drivers/opp/opp.h
@@ -140,6 +140,7 @@ enum opp_table_access {
* @genpd_performance_state: Device's power domain support performance state.
* @set_opp: Platform specific set_opp callback
* @set_opp_data: Data to be passed to set_opp callback
+ * @get_pstate: Platform specific get_pstate callback
* @dentry: debugfs dentry pointer of the real device directory (not links).
* @dentry_name: Name of the real dentry.
*
@@ -177,6 +178,7 @@ struct opp_table {
int (*set_opp)(struct dev_pm_set_opp_data *data);
struct dev_pm_set_opp_data *set_opp_data;
+ int (*get_pstate)(struct device *dev, unsigned long rate);
#ifdef CONFIG_DEBUG_FS
struct dentry *dentry;
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 849d21dc4ca7..6c2d2e88f066 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -125,6 +125,8 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char * name);
void dev_pm_opp_put_clkname(struct opp_table *opp_table);
struct opp_table *dev_pm_opp_register_set_opp_helper(struct device *dev, int (*set_opp)(struct dev_pm_set_opp_data *data));
void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table);
+struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev, int (*get_pstate)(struct device *dev, unsigned long rate));
+void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table);
int dev_pm_opp_set_rate(struct device *dev, unsigned long target_freq);
int dev_pm_opp_set_sharing_cpus(struct device *cpu_dev, const struct cpumask *cpumask);
int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask);
@@ -245,6 +247,14 @@ static inline struct opp_table *dev_pm_opp_register_set_opp_helper(struct device
static inline void dev_pm_opp_unregister_set_opp_helper(struct opp_table *opp_table) {}
+static inline struct opp_table *dev_pm_opp_register_get_pstate_helper(struct device *dev,
+ int (*get_pstate)(struct device *dev, unsigned long rate))
+{
+ return ERR_PTR(-ENOTSUPP);
+}
+
+static inline void dev_pm_opp_unregister_get_pstate_helper(struct opp_table *opp_table) {}
+
static inline struct opp_table *dev_pm_opp_set_prop_name(struct device *dev, const char *name)
{
return ERR_PTR(-ENOTSUPP);
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH V11 4/7] soc: qcom: rpmpd: Add driver to model cx/mx power domains
2017-10-11 7:24 [PATCH V11 0/7] PM / Domains: Performance state support Viresh Kumar
` (2 preceding siblings ...)
2017-10-11 7:24 ` [PATCH V11 3/7] OPP: Add dev_pm_opp_{un}register_get_pstate_helper() Viresh Kumar
@ 2017-10-11 7:24 ` Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 5/7] soc: qcom: rpmpd: Add support for set performance state Viresh Kumar
` (3 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2017-10-11 7:24 UTC (permalink / raw)
To: Rafael Wysocki, ulf.hansson, Kevin Hilman, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Nishanth Menon, robh+dt,
lina.iyer, rnayak, sudeep.holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
From: Rajendra Nayak <rnayak@codeaurora.org>
NOT FOR MERGE.
The cx/mx power domains just pass the performance state set by the
consumers to the RPM (Remote Power manager) which then takes care of
setting the appropriate voltage on the corresponding rails to meet the
performance needs.
Add data for all power domains on msm8996.
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
.../devicetree/bindings/power/qcom,rpmpd.txt | 9 +
arch/arm64/boot/dts/qcom/msm8996.dtsi | 5 +
drivers/soc/qcom/Kconfig | 9 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/rpmpd.c | 307 +++++++++++++++++++++
5 files changed, 331 insertions(+)
create mode 100644 Documentation/devicetree/bindings/power/qcom,rpmpd.txt
create mode 100644 drivers/soc/qcom/rpmpd.c
diff --git a/Documentation/devicetree/bindings/power/qcom,rpmpd.txt b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
new file mode 100644
index 000000000000..1b8b3b849e47
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
@@ -0,0 +1,9 @@
+Qualcomm RPM Power domains
+
+* For RPM power domains, we communicate a performance state to RPM
+which then translates it into a corresponding voltage on a rail.
+
+Required Properties:
+ - compatible: Should be one of the following:
+ * qcom,rpmpd-msm8996: RPM Powerdomain for the msm8996 family of SoC.
+ - power-domain-cells: number of cells in power domain specifier must be 1.
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 887b61c872dd..0be1db559d61 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -496,6 +496,11 @@
status = "disabled";
};
+ rpmpd: qcom,rpmpd {
+ compatible = "qcom,rpmpd-msm8996", "qcom,rpmpd";
+ #power-domain-cells = <1>;
+ };
+
sdhc2: sdhci@74a4900 {
status = "disabled";
compatible = "qcom,sdhci-msm-v4";
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index b00bccddcd3b..48bc7c358dd7 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -35,6 +35,15 @@ config QCOM_PM
modes. It interface with various system drivers to put the cores in
low power modes.
+config QCOM_RPMPD
+ tristate "Qualcomm RPM Power domain driver"
+ depends on MFD_QCOM_RPM && QCOM_SMD_RPM
+ help
+ QCOM RPM power domain driver to support power domain with
+ performance states. The driver communicates a performance state
+ value to RPM which then translates it into corresponding voltage
+ for the voltage rail.
+
config QCOM_SMEM
tristate "Qualcomm Shared Memory Manager (SMEM)"
depends on ARCH_QCOM
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index f151de41eb93..3fa9af1e2c93 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
obj-$(CONFIG_QCOM_SMSM) += smsm.o
obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_QCOM_RPMPD) += rpmpd.o
diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
new file mode 100644
index 000000000000..0958d7693c4f
--- /dev/null
+++ b/drivers/soc/qcom/rpmpd.c
@@ -0,0 +1,307 @@
+/*
+ * Copyright (c) 2017, The Linux Foundation. All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mfd/qcom_rpm.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/soc/qcom/smd-rpm.h>
+
+#include <dt-bindings/mfd/qcom-rpm.h>
+
+#define domain_to_rpmpd(domain) container_of(domain, struct rpmpd, pd)
+
+/* Resource types */
+#define RPMPD_SMPA 0x61706d73
+#define RPMPD_LDOA 0x616f646c
+
+/* Operation Keys */
+#define KEY_CORNER 0x6e726f63 /* corn */
+#define KEY_ENABLE 0x6e657773 /* swen */
+#define KEY_FLOOR_CORNER 0x636676 /* vfc */
+
+#define DEFINE_RPMPD_CORN_SMPA(_platform, _name, _active, r_id) \
+ static struct rpmpd _platform##_##_active; \
+ static struct rpmpd _platform##_##_name = { \
+ .pd = { .name = #_name, }, \
+ .peer = &_platform##_##_active, \
+ .res_type = RPMPD_SMPA, \
+ .res_id = r_id, \
+ .key = KEY_CORNER, \
+ }; \
+ static struct rpmpd _platform##_##_active = { \
+ .pd = { .name = #_active, }, \
+ .peer = &_platform##_##_name, \
+ .active_only = true, \
+ .res_type = RPMPD_SMPA, \
+ .res_id = r_id, \
+ .key = KEY_CORNER, \
+ }
+
+#define DEFINE_RPMPD_CORN_LDOA(_platform, _name, r_id) \
+ static struct rpmpd _platform##_##_name = { \
+ .pd = { .name = #_name, }, \
+ .res_type = RPMPD_LDOA, \
+ .res_id = r_id, \
+ .key = KEY_CORNER, \
+ }
+
+#define DEFINE_RPMPD_VFC(_platform, _name, r_id, r_type) \
+ static struct rpmpd _platform##_##_name = { \
+ .pd = { .name = #_name, }, \
+ .res_type = r_type, \
+ .res_id = r_id, \
+ .key = KEY_FLOOR_CORNER, \
+ }
+
+#define DEFINE_RPMPD_VFC_SMPA(_platform, _name, r_id) \
+ DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_SMPA)
+
+#define DEFINE_RPMPD_VFC_LDOA(_platform, _name, r_id) \
+ DEFINE_RPMPD_VFC(_platform, _name, r_id, RPMPD_LDOA)
+
+struct rpmpd_req {
+ __le32 key;
+ __le32 nbytes;
+ __le32 value;
+};
+
+struct rpmpd {
+ struct generic_pm_domain pd;
+ struct rpmpd *peer;
+ const bool active_only;
+ unsigned long corner;
+ bool enabled;
+ const char *res_name;
+ const int res_type;
+ const int res_id;
+ struct qcom_smd_rpm *rpm;
+ __le32 key;
+};
+
+struct rpmpd_desc {
+ struct rpmpd **rpmpds;
+ size_t num_pds;
+};
+
+static DEFINE_MUTEX(rpmpd_lock);
+
+/* msm8996 RPM powerdomains */
+DEFINE_RPMPD_CORN_SMPA(msm8996, vddcx, vddcx_ao, 1);
+DEFINE_RPMPD_CORN_SMPA(msm8996, vddmx, vddmx_ao, 2);
+DEFINE_RPMPD_CORN_LDOA(msm8996, vddsscx, 26);
+
+DEFINE_RPMPD_VFC_SMPA(msm8996, vddcx_vfc, 1);
+DEFINE_RPMPD_VFC_LDOA(msm8996, vddsscx_vfc, 26);
+
+static struct rpmpd *msm8996_rpmpds[] = {
+ [0] = &msm8996_vddcx,
+ [1] = &msm8996_vddcx_ao,
+ [2] = &msm8996_vddcx_vfc,
+ [3] = &msm8996_vddmx,
+ [4] = &msm8996_vddmx_ao,
+ [5] = &msm8996_vddsscx,
+ [6] = &msm8996_vddsscx_vfc,
+};
+
+static const struct rpmpd_desc msm8996_desc = {
+ .rpmpds = msm8996_rpmpds,
+ .num_pds = ARRAY_SIZE(msm8996_rpmpds),
+};
+
+static const struct of_device_id rpmpd_match_table[] = {
+ { .compatible = "qcom,rpmpd-msm8996", .data = &msm8996_desc },
+ { }
+};
+MODULE_DEVICE_TABLE(of, rpmpd_match_table);
+
+static int rpmpd_send_enable(struct rpmpd *pd, bool enable)
+{
+ struct rpmpd_req req = {
+ .key = KEY_ENABLE,
+ .nbytes = cpu_to_le32(sizeof(u32)),
+ .value = cpu_to_le32(enable),
+ };
+
+ return qcom_rpm_smd_write(pd->rpm, QCOM_RPM_ACTIVE_STATE, pd->res_type,
+ pd->res_id, &req, sizeof(req));
+}
+
+static int rpmpd_send_corner(struct rpmpd *pd, int state, unsigned int corner)
+{
+ struct rpmpd_req req = {
+ .key = pd->key,
+ .nbytes = cpu_to_le32(sizeof(u32)),
+ .value = cpu_to_le32(corner),
+ };
+
+ return qcom_rpm_smd_write(pd->rpm, state, pd->res_type, pd->res_id,
+ &req, sizeof(req));
+};
+
+static void to_active_sleep(struct rpmpd *pd, unsigned long corner,
+ unsigned long *active, unsigned long *sleep)
+{
+ *active = corner;
+
+ if (pd->active_only)
+ *sleep = 0;
+ else
+ *sleep = *active;
+}
+
+static int rpmpd_aggregate_corner(struct rpmpd *pd)
+{
+ int ret;
+ struct rpmpd *peer = pd->peer;
+ unsigned long active_corner, sleep_corner;
+ unsigned long this_corner = 0, this_sleep_corner = 0;
+ unsigned long peer_corner = 0, peer_sleep_corner = 0;
+
+ to_active_sleep(pd, pd->corner, &this_corner, &this_sleep_corner);
+
+ if (peer && peer->enabled)
+ to_active_sleep(peer, peer->corner, &peer_corner,
+ &peer_sleep_corner);
+
+ active_corner = max(this_corner, peer_corner);
+
+ ret = rpmpd_send_corner(pd, QCOM_RPM_ACTIVE_STATE, active_corner);
+ if (ret)
+ return ret;
+
+ sleep_corner = max(this_sleep_corner, peer_sleep_corner);
+
+ return rpmpd_send_corner(pd, QCOM_RPM_SLEEP_STATE, sleep_corner);
+}
+
+static int rpmpd_power_on(struct generic_pm_domain *domain)
+{
+ int ret;
+ struct rpmpd *pd = domain_to_rpmpd(domain);
+
+ mutex_lock(&rpmpd_lock);
+
+ ret = rpmpd_send_enable(pd, true);
+ if (ret)
+ goto out;
+
+ pd->enabled = true;
+
+ if (pd->corner)
+ ret = rpmpd_aggregate_corner(pd);
+
+out:
+ mutex_unlock(&rpmpd_lock);
+
+ return ret;
+}
+
+static int rpmpd_power_off(struct generic_pm_domain *domain)
+{
+ int ret;
+ struct rpmpd *pd = domain_to_rpmpd(domain);
+
+ mutex_lock(&rpmpd_lock);
+
+ ret = rpmpd_send_enable(pd, false);
+ if (!ret)
+ pd->enabled = false;
+
+ mutex_unlock(&rpmpd_lock);
+
+ return ret;
+}
+
+static int rpmpd_probe(struct platform_device *pdev)
+{
+ int i;
+ size_t num;
+ struct genpd_onecell_data *data;
+ struct qcom_smd_rpm *rpm;
+ struct rpmpd **rpmpds;
+ const struct rpmpd_desc *desc;
+
+ rpm = dev_get_drvdata(pdev->dev.parent);
+ if (!rpm) {
+ dev_err(&pdev->dev, "Unable to retrieve handle to RPM\n");
+ return -ENODEV;
+ }
+
+ desc = of_device_get_match_data(&pdev->dev);
+ if (!desc)
+ return -EINVAL;
+
+ rpmpds = desc->rpmpds;
+ num = desc->num_pds;
+
+ data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ return -ENOMEM;
+
+ data->domains = devm_kcalloc(&pdev->dev, num, sizeof(*data->domains),
+ GFP_KERNEL);
+ data->num_domains = num;
+
+ for (i = 0; i < num; i++) {
+ if (!rpmpds[i])
+ continue;
+
+ rpmpds[i]->rpm = rpm;
+ rpmpds[i]->pd.power_off = rpmpd_power_off;
+ rpmpds[i]->pd.power_on = rpmpd_power_on;
+ pm_genpd_init(&rpmpds[i]->pd, NULL, true);
+
+ data->domains[i] = &rpmpds[i]->pd;
+ }
+
+ return of_genpd_add_provider_onecell(pdev->dev.of_node, data);
+}
+
+static int rpmpd_remove(struct platform_device *pdev)
+{
+ of_genpd_del_provider(pdev->dev.of_node);
+ return 0;
+}
+
+static struct platform_driver rpmpd_driver = {
+ .driver = {
+ .name = "qcom-rpmpd",
+ .of_match_table = rpmpd_match_table,
+ },
+ .probe = rpmpd_probe,
+ .remove = rpmpd_remove,
+};
+
+static int __init rpmpd_init(void)
+{
+ return platform_driver_register(&rpmpd_driver);
+}
+core_initcall(rpmpd_init);
+
+static void __exit rpmpd_exit(void)
+{
+ platform_driver_unregister(&rpmpd_driver);
+}
+module_exit(rpmpd_exit);
+
+MODULE_DESCRIPTION("Qualcomm RPM Power Domain Driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:qcom-rpmpd");
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH V11 5/7] soc: qcom: rpmpd: Add support for set performance state
2017-10-11 7:24 [PATCH V11 0/7] PM / Domains: Performance state support Viresh Kumar
` (3 preceding siblings ...)
2017-10-11 7:24 ` [PATCH V11 4/7] soc: qcom: rpmpd: Add driver to model cx/mx power domains Viresh Kumar
@ 2017-10-11 7:24 ` Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 6/7] OPP: qcom: Add support to get performance states corresponding to OPPs Viresh Kumar
` (2 subsequent siblings)
7 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2017-10-11 7:24 UTC (permalink / raw)
To: Rafael Wysocki, ulf.hansson, Kevin Hilman, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Nishanth Menon, robh+dt,
lina.iyer, rnayak, sudeep.holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
From: Rajendra Nayak <rnayak@codeaurora.org>
NOT FOR MERGE.
With genpd now expecting power domain drivers supporting performance
state to support set performance state callbacks, add support for it in
the rpmpd driver.
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/soc/qcom/rpmpd.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
index 0958d7693c4f..34d2eb4e75bf 100644
--- a/drivers/soc/qcom/rpmpd.c
+++ b/drivers/soc/qcom/rpmpd.c
@@ -230,6 +230,28 @@ static int rpmpd_power_off(struct generic_pm_domain *domain)
return ret;
}
+static int rpmpd_set_performance(struct generic_pm_domain *domain,
+ unsigned int state)
+{
+ int ret = 0;
+ struct rpmpd *pd = domain_to_rpmpd(domain);
+
+ mutex_lock(&rpmpd_lock);
+
+ pd->corner = state;
+
+ if (!pd->enabled && (pd->key != KEY_FLOOR_CORNER))
+ goto out;
+
+ ret = rpmpd_aggregate_corner(pd);
+
+out:
+ mutex_unlock(&rpmpd_lock);
+
+ return ret;
+}
+
+
static int rpmpd_probe(struct platform_device *pdev)
{
int i;
@@ -267,6 +289,7 @@ static int rpmpd_probe(struct platform_device *pdev)
rpmpds[i]->rpm = rpm;
rpmpds[i]->pd.power_off = rpmpd_power_off;
rpmpds[i]->pd.power_on = rpmpd_power_on;
+ rpmpds[i]->pd.genpd_set_performance_state = rpmpd_set_performance;
pm_genpd_init(&rpmpds[i]->pd, NULL, true);
data->domains[i] = &rpmpds[i]->pd;
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH V11 6/7] OPP: qcom: Add support to get performance states corresponding to OPPs
2017-10-11 7:24 [PATCH V11 0/7] PM / Domains: Performance state support Viresh Kumar
` (4 preceding siblings ...)
2017-10-11 7:24 ` [PATCH V11 5/7] soc: qcom: rpmpd: Add support for set performance state Viresh Kumar
@ 2017-10-11 7:24 ` Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 7/7] mmc: sdhci-msm: Adapt the driver to use OPPs to set clocks/performance state Viresh Kumar
2017-10-11 11:43 ` [PATCH V11 0/7] PM / Domains: Performance state support Ulf Hansson
7 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2017-10-11 7:24 UTC (permalink / raw)
To: Rafael Wysocki, ulf.hansson, Kevin Hilman, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Nishanth Menon, robh+dt,
lina.iyer, rnayak, sudeep.holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
NOT FOR MERGE.
The OPP core needs to know the performance state for each OPP that the
device supports, if we want to update performance states of the device's
PM domain.
Add support for Qualcomm's rpmpd for the same.
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/opp/Makefile | 2 +
drivers/opp/qcom-rpmpd.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 109 insertions(+)
create mode 100644 drivers/opp/qcom-rpmpd.c
diff --git a/drivers/opp/Makefile b/drivers/opp/Makefile
index e70ceb406fe9..b565031ce10f 100644
--- a/drivers/opp/Makefile
+++ b/drivers/opp/Makefile
@@ -2,3 +2,5 @@ ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
obj-y += core.o cpu.o
obj-$(CONFIG_OF) += of.o
obj-$(CONFIG_DEBUG_FS) += debugfs.o
+
+obj-$(CONFIG_QCOM_RPMPD) += qcom-rpmpd.o
diff --git a/drivers/opp/qcom-rpmpd.c b/drivers/opp/qcom-rpmpd.c
new file mode 100644
index 000000000000..ef301d7003d8
--- /dev/null
+++ b/drivers/opp/qcom-rpmpd.c
@@ -0,0 +1,107 @@
+/*
+ * Copyright (c) 2017, The Linux Foundation. All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pm_opp.h>
+
+struct device;
+
+enum rpmpd_levels {
+ NONE,
+ LOWER, /* SVS2 */
+ LOW, /* SVS */
+ NOMINAL, /* NOMINAL */
+ HIGH, /* Turbo */
+ MAX_LEVEL,
+};
+
+struct rpmpd_freq_map {
+ struct device *dev;
+ unsigned long freq[MAX_LEVEL];
+};
+
+enum msm8996_devices {
+ SDHCI,
+};
+
+static struct rpmpd_freq_map msm8996_rpmpd_freq_map[] = {
+ [SDHCI] = {
+ .freq[LOWER] = 19200000,
+ .freq[LOW] = 200000000,
+ .freq[NOMINAL] = 400000000,
+ },
+};
+
+static int get_pstate(struct device *dev, unsigned long rate)
+{
+ int i, j;
+
+ for (i = 0; i < ARRAY_SIZE(msm8996_rpmpd_freq_map); i++) {
+ if (dev != msm8996_rpmpd_freq_map[i].dev)
+ continue;
+
+ for (j = 0; j < MAX_LEVEL; j++) {
+ if (msm8996_rpmpd_freq_map[i].freq[j] >= rate)
+ return j;
+ }
+
+ return MAX_LEVEL;
+ }
+
+ /* Bug */
+ WARN_ON(1);
+
+ return 0;
+}
+
+static const struct of_device_id devices[] = {
+ { .compatible = "qcom,sdhci-msm-v4", .data = (void *)SDHCI },
+ { }
+};
+
+static int __init rpmpd_opp_init(void)
+{
+ struct platform_device *pdev;
+ struct device_node *np;
+ int i, index;
+
+ if (!of_machine_is_compatible("qcom,msm8996-mtp"))
+ return 0;
+
+ for (i = 0; i < ARRAY_SIZE(devices); i++) {
+ np = of_find_compatible_node(NULL, NULL, devices[i].compatible);
+ if (!np)
+ continue;
+
+ pdev = of_find_device_by_node(np);
+ if (!pdev)
+ pdev = of_platform_device_create(np, NULL, NULL);
+
+ of_node_put(np);
+
+ if (!pdev)
+ continue;
+
+ index = (enum msm8996_devices)(devices[i].data);
+ msm8996_rpmpd_freq_map[index].dev = &pdev->dev;
+
+ dev_pm_opp_register_get_pstate_helper(&pdev->dev, get_pstate);
+ }
+
+ return 0;
+}
+subsys_initcall(rpmpd_opp_init);
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH V11 7/7] mmc: sdhci-msm: Adapt the driver to use OPPs to set clocks/performance state
2017-10-11 7:24 [PATCH V11 0/7] PM / Domains: Performance state support Viresh Kumar
` (5 preceding siblings ...)
2017-10-11 7:24 ` [PATCH V11 6/7] OPP: qcom: Add support to get performance states corresponding to OPPs Viresh Kumar
@ 2017-10-11 7:24 ` Viresh Kumar
2017-10-11 11:43 ` [PATCH V11 0/7] PM / Domains: Performance state support Ulf Hansson
7 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2017-10-11 7:24 UTC (permalink / raw)
To: Rafael Wysocki, ulf.hansson, Kevin Hilman, Stephen Boyd
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Nishanth Menon, robh+dt,
lina.iyer, rnayak, sudeep.holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
From: Rajendra Nayak <rnayak@codeaurora.org>
NOT FOR MERGE.
SDHCI driver needs to set a performance state along with scaling its
clocks. Modify the driver to use the newly introducded powerdomain
performance state based OPPs to scale clocks as well as set an
appropriate powerdomain performance state.
The patch also adds OPPs for sdhci device on msm8996.
The changes have to be validated by populating similar OPP tables
on all other devices which use the sdhci driver. This is for now
validated only on msm8996 and with missing OPP tables for other
devices is known to break those platforms.
Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
arch/arm64/boot/dts/qcom/msm8996.dtsi | 34 ++++++++++++++++++++++++++++++
drivers/clk/qcom/gcc-msm8996.c | 8 +++----
drivers/mmc/host/sdhci-msm.c | 39 ++++++++++++++++++++++++++---------
3 files changed, 67 insertions(+), 14 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 0be1db559d61..71183c009c58 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -515,8 +515,42 @@
<&gcc GCC_SDCC2_APPS_CLK>,
<&xo_board>;
bus-width = <4>;
+ power-domains = <&rpmpd 0>;
+ operating-points-v2 = <&sdhc_opp_table>;
};
+ sdhc_opp_table: opp_table {
+ compatible = "operating-points-v2";
+
+ opp@400000 {
+ opp-hz = /bits/ 64 <400000>;
+ };
+
+ opp@20000000 {
+ opp-hz = /bits/ 64 <20000000>;
+ };
+
+ opp@25000000 {
+ opp-hz = /bits/ 64 <25000000>;
+ };
+
+ opp@50000000 {
+ opp-hz = /bits/ 64 <50000000>;
+ };
+
+ opp@96000000 {
+ opp-hz = /bits/ 64 <96000000>;
+ };
+
+ opp@192000000 {
+ opp-hz = /bits/ 64 <192000000>;
+ };
+
+ opp@384000000 {
+ opp-hz = /bits/ 64 <384000000>;
+ };
+ };
+
msmgpio: pinctrl@1010000 {
compatible = "qcom,msm8996-pinctrl";
reg = <0x01010000 0x300000>;
diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
index 7ddec886fcd3..38034692f5aa 100644
--- a/drivers/clk/qcom/gcc-msm8996.c
+++ b/drivers/clk/qcom/gcc-msm8996.c
@@ -460,7 +460,7 @@ static struct clk_rcg2 sdcc1_apps_clk_src = {
.name = "sdcc1_apps_clk_src",
.parent_names = gcc_xo_gpll0_gpll4_gpll0_early_div,
.num_parents = 4,
- .ops = &clk_rcg2_floor_ops,
+ .ops = &clk_rcg2_ops,
},
};
@@ -505,7 +505,7 @@ static struct clk_rcg2 sdcc2_apps_clk_src = {
.name = "sdcc2_apps_clk_src",
.parent_names = gcc_xo_gpll0_gpll4,
.num_parents = 3,
- .ops = &clk_rcg2_floor_ops,
+ .ops = &clk_rcg2_ops,
},
};
@@ -519,7 +519,7 @@ static struct clk_rcg2 sdcc3_apps_clk_src = {
.name = "sdcc3_apps_clk_src",
.parent_names = gcc_xo_gpll0_gpll4,
.num_parents = 3,
- .ops = &clk_rcg2_floor_ops,
+ .ops = &clk_rcg2_ops,
},
};
@@ -543,7 +543,7 @@ static struct clk_rcg2 sdcc4_apps_clk_src = {
.name = "sdcc4_apps_clk_src",
.parent_names = gcc_xo_gpll0,
.num_parents = 2,
- .ops = &clk_rcg2_floor_ops,
+ .ops = &clk_rcg2_ops,
},
};
diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
index fc73e56eb1e2..cbc5a12af772 100644
--- a/drivers/mmc/host/sdhci-msm.c
+++ b/drivers/mmc/host/sdhci-msm.c
@@ -21,6 +21,7 @@
#include <linux/pm_runtime.h>
#include <linux/slab.h>
#include <linux/iopoll.h>
+#include <linux/pm_opp.h>
#include "sdhci-pltfm.h"
@@ -131,6 +132,7 @@ struct sdhci_msm_host {
struct clk *pclk; /* SDHC peripheral bus clock */
struct clk *bus_clk; /* SDHC bus voter clock */
struct clk *xo_clk; /* TCXO clk needed for FLL feature of cm_dll*/
+ struct opp_table *opp_table;
unsigned long clk_rate;
struct mmc_host *mmc;
bool use_14lpp_dll_reset;
@@ -140,7 +142,7 @@ struct sdhci_msm_host {
bool use_cdclp533;
};
-static unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
+static long unsigned int msm_get_clock_rate_for_bus_mode(struct sdhci_host *host,
unsigned int clock)
{
struct mmc_ios ios = host->mmc->ios;
@@ -165,16 +167,22 @@ static void msm_set_clock_rate_for_bus_mode(struct sdhci_host *host,
struct sdhci_msm_host *msm_host = sdhci_pltfm_priv(pltfm_host);
struct mmc_ios curr_ios = host->mmc->ios;
int rc;
+ struct device *dev = &msm_host->pdev->dev;
+ struct dev_pm_opp *opp;
+ long unsigned int freq;
+
+ freq = msm_get_clock_rate_for_bus_mode(host, clock);
+ opp = dev_pm_opp_find_freq_floor(dev, &freq);
+ if (IS_ERR(opp))
+ pr_err("%s: failed to find OPP for %u at timing %d\n",
+ mmc_hostname(host->mmc), clock, curr_ios.timing);
+
+ rc = dev_pm_opp_set_rate(dev, freq);
+ if (rc)
+ pr_err("%s: error in setting opp\n", __func__);
+
+ msm_host->clk_rate = freq;
- clock = msm_get_clock_rate_for_bus_mode(host, clock);
- rc = clk_set_rate(msm_host->clk, clock);
- if (rc) {
- pr_err("%s: Failed to set clock at rate %u at timing %d\n",
- mmc_hostname(host->mmc), clock,
- curr_ios.timing);
- return;
- }
- msm_host->clk_rate = clock;
pr_debug("%s: Setting clock at rate %lu at timing %d\n",
mmc_hostname(host->mmc), clk_get_rate(msm_host->clk),
curr_ios.timing);
@@ -1268,6 +1276,13 @@ static int sdhci_msm_probe(struct platform_device *pdev)
goto clk_disable;
}
+ /* Set up the OPP table */
+ msm_host->opp_table = dev_pm_opp_set_clkname(&pdev->dev, "core");
+
+ ret = dev_pm_opp_of_add_table(&pdev->dev);
+ if (ret)
+ dev_warn(&pdev->dev, "%s: No OPP table specified\n", __func__);
+
pm_runtime_get_noresume(&pdev->dev);
pm_runtime_set_active(&pdev->dev);
pm_runtime_enable(&pdev->dev);
@@ -1289,6 +1304,8 @@ static int sdhci_msm_probe(struct platform_device *pdev)
pm_runtime_disable(&pdev->dev);
pm_runtime_set_suspended(&pdev->dev);
pm_runtime_put_noidle(&pdev->dev);
+ dev_pm_opp_of_remove_table(&pdev->dev);
+ dev_pm_opp_put_clkname(msm_host->opp_table);
clk_disable:
clk_disable_unprepare(msm_host->clk);
pclk_disable:
@@ -1314,6 +1331,8 @@ static int sdhci_msm_remove(struct platform_device *pdev)
pm_runtime_get_sync(&pdev->dev);
pm_runtime_disable(&pdev->dev);
pm_runtime_put_noidle(&pdev->dev);
+ dev_pm_opp_of_remove_table(&pdev->dev);
+ dev_pm_opp_put_clkname(msm_host->opp_table);
clk_disable_unprepare(msm_host->clk);
clk_disable_unprepare(msm_host->pclk);
--
2.7.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V11 1/7] PM / Domains: Add support to select performance-state of domains
2017-10-11 7:24 ` [PATCH V11 1/7] PM / Domains: Add support to select performance-state of domains Viresh Kumar
@ 2017-10-11 11:27 ` Ulf Hansson
2017-10-12 7:09 ` [PATCH V12 " Viresh Kumar
2017-10-12 9:37 ` [PATCH V13 " Viresh Kumar
2 siblings, 0 replies; 16+ messages in thread
From: Ulf Hansson @ 2017-10-11 11:27 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Kevin Hilman, Stephen Boyd, linux-pm,
Vincent Guittot, Nishanth Menon, Rob Herring, Lina Iyer,
Rajendra Nayak, Sudeep Holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
On 11 October 2017 at 09:24, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Some platforms have the capability to configure the performance state of
> PM domains. This patch enhances the genpd core to support such
> platforms.
>
> The performance levels (within the genpd core) are identified by
> positive integer values, a lower value represents lower performance
> state.
>
> This patch adds a new genpd API, which is called by user drivers (like
> OPP framework):
>
> - int dev_pm_genpd_set_performance_state(struct device *dev,
> unsigned int state);
>
> This updates the performance state constraint of the device on its PM
> domain. On success, the genpd will have its performance state set to a
> value which is >= "state" passed to this routine. The genpd core calls
> the genpd->genpd_set_performance_state() callback, if implemented,
> else -ENODEV is returned to the caller.
>
> The PM domain drivers need to implement the following callback if they
> want to support performance states.
>
> - int (*genpd_set_performance_state)(struct generic_pm_domain *genpd,
> unsigned int state);
>
> This is called internally by the genpd core on several occasions. The
> genpd core passes the genpd pointer and the aggregate of the
> performance states of the devices supported by that genpd to this
> callback. This callback must update the performance state of the genpd
> in a platform dependent way.
>
> The power domains can avoid supplying above callback, if they don't
> support setting performance-states.
>
> A TODO comment is also added to _genpd_reeval_performance_state(). This
> feature will be required once we have hardware that needs to propagate
> the performance state changes to master domains.
Perhaps re-phrase this to being about "a limitation that can be fixed
later if needed" rather than an TODO. That applies also to the comment
in the code.
>
> Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/base/power/domain.c | 179 +++++++++++++++++++++++++++++++++++++++++++-
> include/linux/pm_domain.h | 13 ++++
> 2 files changed, 190 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a6e4c8d7d837..b8360bc6a8eb 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -237,6 +237,169 @@ static void genpd_update_accounting(struct generic_pm_domain *genpd)
> static inline void genpd_update_accounting(struct generic_pm_domain *genpd) {}
> #endif
>
> +/* Returns negative errors or 0 on success */
> +static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
> + int state)
Please rename the function to: genpd_set_performance_state().
> +{
> + int ret;
> +
> + ret = genpd->genpd_set_performance_state(genpd, state);
> + if (!ret)
> + genpd->performance_state = state;
> +
> + return ret;
> +}
> +
> +/*
> + * Re-evaluate performance state of a genpd. Finds the highest requested
> + * performance state by the devices within the genpd and then change genpd's
> + * performance state (if required).
> + *
> + * @genpd: PM domain whose state needs to be reevaluated.
> + * @state: Newly requested performance state of the device for which this
> + * routine is called.
> + *
> + * Returns negative errors or 0 on success.
> + */
This isn't an exported function, so you may slim down this information
quite a bit.
> +static int _genpd_reeval_performance_state(struct generic_pm_domain *genpd,
> + int state)
By looking how this function is used, I suggest you to rename it to
genpd_update_performance_state() and also fold in the code from the
__genpd_dev_update_performance_state(). I think that would simplify
the code a bit.
> +{
> + struct generic_pm_domain_data *pd_data;
> + struct pm_domain_data *pdd;
> +
> + /* New requested state is same as Max requested state */
> + if (state == genpd->performance_state)
> + return 0;
> +
> + /* New requested state is higher than Max requested state */
> + if (state > genpd->performance_state)
> + goto update_state;
> +
> + /* Traverse all devices within the domain */
> + list_for_each_entry(pdd, &genpd->dev_list, list_node) {
> + pd_data = to_gpd_data(pdd);
> +
> + if (pd_data->performance_state > state)
> + state = pd_data->performance_state;
> + }
> +
> + /*
> + * TODO: Traverse all subdomains of the genpd. This will be
> + * required once we have hardware that needs to propagate the
> + * performance state changes.
> + */
As mentioned, please re-phrase this to be about a limitation rather than a TODO.
> +
> +update_state:
> + return _genpd_set_performance_state(genpd, state);
> +}
> +
> +static void __genpd_dev_update_performance_state(struct device *dev, int state)
> +{
> + struct generic_pm_domain_data *gpd_data;
> +
> + spin_lock_irq(&dev->power.lock);
I remember we already discussed this lock, but I did second thought
around this. Apologize for coming back to this again.
As you state in the function header of
dev_pm_genpd_set_performance_state(), users of this function must
guarantee that the device don't get detached from its genpd while
calling it. For that reason, the ->domain_data pointer will always be
valid when __genpd_dev_update_performance_state() is executed. In
other words, there's no reason to use the lock to protect access to
it.
> +
> + if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data) {
> + WARN_ON(1);
> + goto unlock;
> + }
> +
> + gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> + gpd_data->performance_state = state;
> +
> +unlock:
> + spin_unlock_irq(&dev->power.lock);
> +}
> +
> +/**
> + * dev_pm_genpd_set_performance_state- Set performance state of device's power
> + * domain.
> + *
> + * @dev: Device for which the performance-state needs to be adjusted.
> + * @state: Target performance state of the device. This can be set as 0 when the
> + * device doesn't have any performance state constraints left (And so
> + * the device wouldn't participate anymore to find the target
> + * performance state of the genpd).
> + *
> + * It is assumed that the user driver guarantees that the genpd wouldn't be
> + * detached while this routine is getting called.
> + *
> + * Returns 0 on success and negative error values on failures.
> + */
> +int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
> +{
> + struct generic_pm_domain *genpd;
> + int ret;
> +
> + genpd = dev_to_genpd(dev);
> + if (IS_ERR(genpd))
> + return -ENODEV;
> +
> + genpd_lock(genpd);
> +
> + if (!genpd->genpd_set_performance_state) {
Because we expect the ->genpd_set_performance_state() to be assigned
once and at initialization (before pm_genpd_init() is called), you can
move this check outside the genpd lock.
If the reason for the lock, is to avoid the genpd from being removed!?
Then, that is also managed by stating that users of
dev_pm_genpd_set_performance_state() must guarantee the device won't
get detached from its genpd when calling this function.
> + ret = -ENODEV;
> + goto unlock;
> + }
> +
> + if (!genpd_status_on(genpd)) {
> + ret = -EBUSY;
To me, it seems feasible to allow users to request a new performance
state, no matter of whether the PM domain is powered on or off.
In case the genpd is off, we should only compute a new aggregated
performance state value, but not call ->genpd_set_performance_state().
More comments about that below.
> + goto unlock;
> + }
> +
> + ret = _genpd_reeval_performance_state(genpd, state);
> + if (!ret) {
> + /*
> + * Since we are passing "state" as well to
> + * _genpd_reeval_performance_state(), we don't need to call
> + * __genpd_dev_update_performance_state() before updating
> + * genpd's state with the above call. Update it only after the
> + * state of master domain is updated.
> + */
This looks like an old comment from earlier version. I guess you
should remove it!?
> + __genpd_dev_update_performance_state(dev, state);
> + }
> +
> +unlock:
> + genpd_unlock(genpd);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
> +
> +static int _genpd_on_update_performance_state(struct generic_pm_domain *genpd)
> +{
> + int ret, prev = genpd->prev_performance_state;
> +
> + if (likely(!prev))
> + return 0;
> +
> + ret = _genpd_set_performance_state(genpd, prev);
> + if (ret) {
> + pr_err("%s: Failed to restore performance state to %d (%d)\n",
> + genpd->name, prev, ret);
> + } else {
> + genpd->prev_performance_state = 0;
> + }
> +
> + return ret;
> +}
> +
> +static void _genpd_off_update_performance_state(struct generic_pm_domain *genpd)
> +{
> + int ret, state = genpd->performance_state;
> +
> + if (likely(!state))
> + return;
> +
> + ret = _genpd_set_performance_state(genpd, 0);
> + if (ret) {
> + pr_err("%s: Failed to set performance state to 0 (%d)\n",
> + genpd->name, ret);
> + } else {
> + genpd->prev_performance_state = state;
> + }
> +}
> +
> static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> {
> unsigned int state_idx = genpd->state_idx;
> @@ -388,6 +551,8 @@ static int genpd_power_off(struct generic_pm_domain *genpd, bool one_dev_on,
> return ret;
> }
>
> + _genpd_off_update_performance_state(genpd);
> +
This hole thing of keeping track of the previous performance state,
while powering off seems a bit unnecessary complex in my opinion.
Instead I suggest to pick a more simple approach, like this:
*) Allow users to update the performance state even when the genpd is
powered off. As stated above.
**) In _genpd_power_on(), invoke the _genpd_set_performance_state(),
after the ->power_on() callback has been invoked an succeeded. Whether
_genpd_set_performance_state() fails in this case, there is not much
we can do, but just print a warning.
***) During power off, just don't care about the current performance
state, but instead leave that to the genpd client to be managed via
the ->power_off() callback.
In this way, the ->genpd_set_performance_state() is only called when
the genpd is powered on. That should be easier for the genpd clients
to cope with.
Moreover, most of this related code can then be dropped and so can the
"unsigned int prev_performance_state" in the genpd struct.
> genpd->status = GPD_STATE_POWER_OFF;
> genpd_update_accounting(genpd);
>
> @@ -437,15 +602,21 @@ static int genpd_power_on(struct generic_pm_domain *genpd, unsigned int depth)
> }
> }
>
> - ret = _genpd_power_on(genpd, true);
> + ret = _genpd_on_update_performance_state(genpd);
> if (ret)
> goto err;
>
> + ret = _genpd_power_on(genpd, true);
> + if (ret)
> + goto err_power_on;
> +
> genpd->status = GPD_STATE_ACTIVE;
> genpd_update_accounting(genpd);
>
> return 0;
>
> + err_power_on:
> + _genpd_off_update_performance_state(genpd);
> err:
> list_for_each_entry_continue_reverse(link,
> &genpd->slave_links,
> @@ -803,6 +974,8 @@ static void genpd_sync_power_off(struct generic_pm_domain *genpd, bool use_lock,
> if (_genpd_power_off(genpd, false))
> return;
>
> + _genpd_off_update_performance_state(genpd);
> +
> genpd->status = GPD_STATE_POWER_OFF;
>
> list_for_each_entry(link, &genpd->slave_links, slave_node) {
> @@ -848,7 +1021,9 @@ static void genpd_sync_power_on(struct generic_pm_domain *genpd, bool use_lock,
> genpd_unlock(link->master);
> }
>
> - _genpd_power_on(genpd, false);
> + if (!_genpd_on_update_performance_state(genpd))
> + if (_genpd_power_on(genpd, false))
> + _genpd_off_update_performance_state(genpd);
>
> genpd->status = GPD_STATE_ACTIVE;
> }
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 84f423d5633e..81d923f058fd 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -64,8 +64,12 @@ struct generic_pm_domain {
> unsigned int device_count; /* Number of devices */
> unsigned int suspended_count; /* System suspend device counter */
> unsigned int prepared_count; /* Suspend counter of prepared devices */
> + unsigned int performance_state; /* Max requested performance state */
> + unsigned int prev_performance_state; /* Performance state before power off */
> int (*power_off)(struct generic_pm_domain *domain);
> int (*power_on)(struct generic_pm_domain *domain);
> + int (*genpd_set_performance_state)(struct generic_pm_domain *genpd,
> + unsigned int state);
Please rename this callback to "set_performance_state".
> struct gpd_dev_ops dev_ops;
> s64 max_off_time_ns; /* Maximum allowed "suspended" time. */
> bool max_off_time_changed;
> @@ -121,6 +125,7 @@ struct generic_pm_domain_data {
> struct pm_domain_data base;
> struct gpd_timing_data td;
> struct notifier_block nb;
> + unsigned int performance_state;
> void *data;
> };
>
> @@ -148,6 +153,8 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
> extern int pm_genpd_init(struct generic_pm_domain *genpd,
> struct dev_power_governor *gov, bool is_off);
> extern int pm_genpd_remove(struct generic_pm_domain *genpd);
> +extern int dev_pm_genpd_set_performance_state(struct device *dev,
> + unsigned int state);
>
> extern struct dev_power_governor simple_qos_governor;
> extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -188,6 +195,12 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
> return -ENOTSUPP;
> }
>
> +static inline int dev_pm_genpd_set_performance_state(struct device *dev,
> + unsigned int state)
> +{
> + return -ENOTSUPP;
> +}
> +
> #define simple_qos_governor (*(struct dev_power_governor *)(NULL))
> #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
> #endif
> --
> 2.7.4
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V11 0/7] PM / Domains: Performance state support
2017-10-11 7:24 [PATCH V11 0/7] PM / Domains: Performance state support Viresh Kumar
` (6 preceding siblings ...)
2017-10-11 7:24 ` [PATCH V11 7/7] mmc: sdhci-msm: Adapt the driver to use OPPs to set clocks/performance state Viresh Kumar
@ 2017-10-11 11:43 ` Ulf Hansson
7 siblings, 0 replies; 16+ messages in thread
From: Ulf Hansson @ 2017-10-11 11:43 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Kevin Hilman, Stephen Boyd, linux-pm,
Vincent Guittot, Nishanth Menon, Rob Herring, Lina Iyer,
Rajendra Nayak, Sudeep Holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
On 11 October 2017 at 09:24, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hi,
>
> This version contains the changes we discussed during Linaro Connect.
>
> Some platforms have the capability to configure the performance state of
> their power domains. The process of configuring the performance state is
> pretty much platform dependent and we may need to work with a wide range
> of configurables. For some platforms, like Qcom, it can be a positive
> integer value alone, while in other cases it can be voltage levels, etc.
>
> The power-domain framework until now was only designed for the idle
> state management of the device and this needs to change in order to
> reuse the power-domain framework for active state management of the
> devices.
>
> The first patch updates the genpd framework to supply new APIs to
> support active state management and the second patch uses them from the
> OPP core. The third patch adds a new API to the OPP core to get
> performance state corresponding to OPPs (This should rather come via DT
> and would be removed once we have fixed bindings for performance
> states).
>
> Rest of the patches [4-7/7] are included to show how user drivers would
> end up using the new APIs and these patches aren't ready to get merged
> yet and are marked clearly like that. Moreover some of them may go via
> SoC specific trees instead of the PM tree.
>
> This is currently tested by:
> - /me by hacking the kernel a bit with virtual power-domains for the ARM
> 64 hikey platform.
> - Rajendra Nayak, on msm8996 platform (Qcom) with MMC controller.
>
> Thanks Rajendra for helping me testing this out.
>
> I also had a chat with Rajendra and we should be able to get a Qualcomm
> specific power domain driver (which uses these changes) in coming weeks.
>
> I am targeting the first 3 patches for 4.15-rc1, if possible.
I have looked through the series and overall it looks okay to me. I
think my comments on patch1 should be rather simple to address - and
so I agree that aiming for 4.15rc1 seems like a reasonable plan.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V12 1/7] PM / Domains: Add support to select performance-state of domains
2017-10-11 7:24 ` [PATCH V11 1/7] PM / Domains: Add support to select performance-state of domains Viresh Kumar
2017-10-11 11:27 ` Ulf Hansson
@ 2017-10-12 7:09 ` Viresh Kumar
2017-10-12 8:01 ` Ulf Hansson
2017-10-12 9:37 ` [PATCH V13 " Viresh Kumar
2 siblings, 1 reply; 16+ messages in thread
From: Viresh Kumar @ 2017-10-12 7:09 UTC (permalink / raw)
To: Rafael Wysocki, ulf.hansson, Kevin Hilman
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Stephen Boyd,
Nishanth Menon, robh+dt, lina.iyer, rnayak, sudeep.holla,
linux-kernel, Len Brown, Pavel Machek, Andy Gross, David Brown
Some platforms have the capability to configure the performance state of
PM domains. This patch enhances the genpd core to support such
platforms.
The performance levels (within the genpd core) are identified by
positive integer values, a lower value represents lower performance
state.
This patch adds a new genpd API, which is called by user drivers (like
OPP framework):
- int dev_pm_genpd_set_performance_state(struct device *dev,
unsigned int state);
This updates the performance state constraint of the device on its PM
domain. On success, the genpd will have its performance state set to a
value which is >= "state" passed to this routine. The genpd core calls
the genpd->set_performance_state() callback, if implemented,
else -ENODEV is returned to the caller.
The PM domain drivers need to implement the following callback if they
want to support performance states.
- int (*set_performance_state)(struct generic_pm_domain *genpd,
unsigned int state);
This is called internally by the genpd core on several occasions. The
genpd core passes the genpd pointer and the aggregate of the
performance states of the devices supported by that genpd to this
callback. This callback must update the performance state of the genpd
(in a platform dependent way).
The power domains can avoid supplying above callback, if they don't
support setting performance-states.
Currently we aren't propagating performance state changes of a subdomain
to its masters as we don't have hardware that needs it right now. Over
that, the performance states of subdomain and its masters may not have
one-to-one mapping and would require additional information. We can get
back to this once we have hardware that needs it.
Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Only the first patch has got updates (out of the first 3 patches, which
are going to get applied for now) and so I am resending only the first
one here.
V12:
- Always allow setting performance state, even if genpd is off. Don't
call the callback in that case.
- Set performance state only when powering ON the genpd and not during
power OFF. The driver can take care of it.
- Merge several routines and remove unnecessary locking.
- Update comments and log a bit.
drivers/base/power/domain.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_domain.h | 12 ++++++
2 files changed, 101 insertions(+)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a6e4c8d7d837..735850893882 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -237,6 +237,86 @@ static void genpd_update_accounting(struct generic_pm_domain *genpd)
static inline void genpd_update_accounting(struct generic_pm_domain *genpd) {}
#endif
+/**
+ * dev_pm_genpd_set_performance_state- Set performance state of device's power
+ * domain.
+ *
+ * @dev: Device for which the performance-state needs to be set.
+ * @state: Target performance state of the device. This can be set as 0 when the
+ * device doesn't have any performance state constraints left (And so
+ * the device wouldn't participate anymore to find the target
+ * performance state of the genpd).
+ *
+ * It is assumed that the users guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ *
+ * Returns 0 on success and negative error values on failures.
+ */
+int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
+{
+ struct generic_pm_domain *genpd;
+ struct generic_pm_domain_data *pd_data;
+ struct pm_domain_data *pdd;
+ int ret = 0;
+
+ genpd = dev_to_genpd(dev);
+ if (IS_ERR(genpd))
+ return -ENODEV;
+
+ if (!genpd->set_performance_state)
+ return -EINVAL;
+
+ if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data) {
+ WARN_ON(1);
+ return -EINVAL;
+ }
+
+ genpd_lock(genpd);
+
+ /* New requested state is same as Max requested state */
+ if (state == genpd->performance_state)
+ return 0;
+
+ /* New requested state is higher than Max requested state */
+ if (state > genpd->performance_state)
+ goto update_state;
+
+ /* Traverse all devices within the domain */
+ list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+ pd_data = to_gpd_data(pdd);
+
+ if (pd_data->performance_state > state)
+ state = pd_data->performance_state;
+ }
+
+ /*
+ * We aren't propagating performance state changes of a subdomain to its
+ * masters as we don't have hardware that needs it. Over that, the
+ * performance states of subdomain and its masters may not have
+ * one-to-one mapping and would require additional information. We can
+ * get back to this once we have hardware that needs it. For that
+ * reason, we don't have to consider performance state of the subdomains
+ * of genpd here.
+ */
+
+update_state:
+ if (genpd_status_on(genpd)) {
+ ret = genpd->set_performance_state(genpd, state);
+ if (ret)
+ goto unlock;
+ }
+
+ genpd->performance_state = state;
+ pd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+ pd_data->performance_state = state;
+
+unlock:
+ genpd_unlock(genpd);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
+
static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
{
unsigned int state_idx = genpd->state_idx;
@@ -256,6 +336,15 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
return ret;
elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
+
+ if (unlikely(genpd->performance_state)) {
+ ret = genpd->set_performance_state(genpd, genpd->performance_state);
+ if (ret) {
+ pr_warn("%s: Failed to set performance state %d (%d)\n",
+ genpd->name, genpd->performance_state, ret);
+ }
+ }
+
if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
return ret;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 84f423d5633e..3f8de25418a8 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -64,8 +64,11 @@ struct generic_pm_domain {
unsigned int device_count; /* Number of devices */
unsigned int suspended_count; /* System suspend device counter */
unsigned int prepared_count; /* Suspend counter of prepared devices */
+ unsigned int performance_state; /* Max requested performance state */
int (*power_off)(struct generic_pm_domain *domain);
int (*power_on)(struct generic_pm_domain *domain);
+ int (*set_performance_state)(struct generic_pm_domain *genpd,
+ unsigned int state);
struct gpd_dev_ops dev_ops;
s64 max_off_time_ns; /* Maximum allowed "suspended" time. */
bool max_off_time_changed;
@@ -121,6 +124,7 @@ struct generic_pm_domain_data {
struct pm_domain_data base;
struct gpd_timing_data td;
struct notifier_block nb;
+ unsigned int performance_state;
void *data;
};
@@ -148,6 +152,8 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
extern int pm_genpd_init(struct generic_pm_domain *genpd,
struct dev_power_governor *gov, bool is_off);
extern int pm_genpd_remove(struct generic_pm_domain *genpd);
+extern int dev_pm_genpd_set_performance_state(struct device *dev,
+ unsigned int state);
extern struct dev_power_governor simple_qos_governor;
extern struct dev_power_governor pm_domain_always_on_gov;
@@ -188,6 +194,12 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
return -ENOTSUPP;
}
+static inline int dev_pm_genpd_set_performance_state(struct device *dev,
+ unsigned int state)
+{
+ return -ENOTSUPP;
+}
+
#define simple_qos_governor (*(struct dev_power_governor *)(NULL))
#define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
#endif
--
2.15.0.rc1.236.g92ea95045093
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V12 1/7] PM / Domains: Add support to select performance-state of domains
2017-10-12 7:09 ` [PATCH V12 " Viresh Kumar
@ 2017-10-12 8:01 ` Ulf Hansson
0 siblings, 0 replies; 16+ messages in thread
From: Ulf Hansson @ 2017-10-12 8:01 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Kevin Hilman, linux-pm, Vincent Guittot,
Stephen Boyd, Nishanth Menon, Rob Herring, Lina Iyer,
Rajendra Nayak, Sudeep Holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
On 12 October 2017 at 09:09, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Some platforms have the capability to configure the performance state of
> PM domains. This patch enhances the genpd core to support such
> platforms.
>
> The performance levels (within the genpd core) are identified by
> positive integer values, a lower value represents lower performance
> state.
>
> This patch adds a new genpd API, which is called by user drivers (like
> OPP framework):
>
> - int dev_pm_genpd_set_performance_state(struct device *dev,
> unsigned int state);
>
> This updates the performance state constraint of the device on its PM
> domain. On success, the genpd will have its performance state set to a
> value which is >= "state" passed to this routine. The genpd core calls
> the genpd->set_performance_state() callback, if implemented,
> else -ENODEV is returned to the caller.
>
> The PM domain drivers need to implement the following callback if they
> want to support performance states.
>
> - int (*set_performance_state)(struct generic_pm_domain *genpd,
> unsigned int state);
>
> This is called internally by the genpd core on several occasions. The
> genpd core passes the genpd pointer and the aggregate of the
> performance states of the devices supported by that genpd to this
> callback. This callback must update the performance state of the genpd
> (in a platform dependent way).
>
> The power domains can avoid supplying above callback, if they don't
> support setting performance-states.
>
> Currently we aren't propagating performance state changes of a subdomain
> to its masters as we don't have hardware that needs it right now. Over
> that, the performance states of subdomain and its masters may not have
> one-to-one mapping and would require additional information. We can get
> back to this once we have hardware that needs it.
>
> Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Only the first patch has got updates (out of the first 3 patches, which
> are going to get applied for now) and so I am resending only the first
> one here.
>
> V12:
> - Always allow setting performance state, even if genpd is off. Don't
> call the callback in that case.
> - Set performance state only when powering ON the genpd and not during
> power OFF. The driver can take care of it.
> - Merge several routines and remove unnecessary locking.
> - Update comments and log a bit.
>
> drivers/base/power/domain.c | 89 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 12 ++++++
> 2 files changed, 101 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a6e4c8d7d837..735850893882 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -237,6 +237,86 @@ static void genpd_update_accounting(struct generic_pm_domain *genpd)
> static inline void genpd_update_accounting(struct generic_pm_domain *genpd) {}
> #endif
>
> +/**
> + * dev_pm_genpd_set_performance_state- Set performance state of device's power
> + * domain.
> + *
> + * @dev: Device for which the performance-state needs to be set.
> + * @state: Target performance state of the device. This can be set as 0 when the
> + * device doesn't have any performance state constraints left (And so
> + * the device wouldn't participate anymore to find the target
> + * performance state of the genpd).
> + *
> + * It is assumed that the users guarantee that the genpd wouldn't be detached
> + * while this routine is getting called.
> + *
> + * Returns 0 on success and negative error values on failures.
> + */
> +int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
> +{
> + struct generic_pm_domain *genpd;
> + struct generic_pm_domain_data *pd_data;
> + struct pm_domain_data *pdd;
> + int ret = 0;
> +
> + genpd = dev_to_genpd(dev);
> + if (IS_ERR(genpd))
> + return -ENODEV;
> +
> + if (!genpd->set_performance_state)
> + return -EINVAL;
> +
> + if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data) {
> + WARN_ON(1);
> + return -EINVAL;
> + }
> +
> + genpd_lock(genpd);
> +
> + /* New requested state is same as Max requested state */
> + if (state == genpd->performance_state)
> + return 0;
You can't just return 0 here.
1) The lock must be released.
2) You need to update the device's performance_state
(pd_data->performance_state = state)
> +
> + /* New requested state is higher than Max requested state */
> + if (state > genpd->performance_state)
> + goto update_state;
> +
> + /* Traverse all devices within the domain */
> + list_for_each_entry(pdd, &genpd->dev_list, list_node) {
> + pd_data = to_gpd_data(pdd);
> +
> + if (pd_data->performance_state > state)
> + state = pd_data->performance_state;
> + }
> +
> + /*
> + * We aren't propagating performance state changes of a subdomain to its
> + * masters as we don't have hardware that needs it. Over that, the
> + * performance states of subdomain and its masters may not have
> + * one-to-one mapping and would require additional information. We can
> + * get back to this once we have hardware that needs it. For that
> + * reason, we don't have to consider performance state of the subdomains
> + * of genpd here.
> + */
> +
> +update_state:
> + if (genpd_status_on(genpd)) {
> + ret = genpd->set_performance_state(genpd, state);
> + if (ret)
> + goto unlock;
> + }
> +
> + genpd->performance_state = state;
> + pd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> + pd_data->performance_state = state;
> +
> +unlock:
> + genpd_unlock(genpd);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
> +
> static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> {
> unsigned int state_idx = genpd->state_idx;
> @@ -256,6 +336,15 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> return ret;
>
> elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
> +
> + if (unlikely(genpd->performance_state)) {
If the aggregated performance_state has been decreased to 0 while the
genpd was powered off, then I think you need to set the performance
state to zero as well.
Perhaps better to check if (genpd->set_performance_state) and if
assigned, call it with whatever value genpd->performance_state has.
> + ret = genpd->set_performance_state(genpd, genpd->performance_state);
> + if (ret) {
> + pr_warn("%s: Failed to set performance state %d (%d)\n",
> + genpd->name, genpd->performance_state, ret);
> + }
> + }
> +
> if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
> return ret;
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 84f423d5633e..3f8de25418a8 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -64,8 +64,11 @@ struct generic_pm_domain {
> unsigned int device_count; /* Number of devices */
> unsigned int suspended_count; /* System suspend device counter */
> unsigned int prepared_count; /* Suspend counter of prepared devices */
> + unsigned int performance_state; /* Max requested performance state */
> int (*power_off)(struct generic_pm_domain *domain);
> int (*power_on)(struct generic_pm_domain *domain);
> + int (*set_performance_state)(struct generic_pm_domain *genpd,
> + unsigned int state);
> struct gpd_dev_ops dev_ops;
> s64 max_off_time_ns; /* Maximum allowed "suspended" time. */
> bool max_off_time_changed;
> @@ -121,6 +124,7 @@ struct generic_pm_domain_data {
> struct pm_domain_data base;
> struct gpd_timing_data td;
> struct notifier_block nb;
> + unsigned int performance_state;
> void *data;
> };
>
> @@ -148,6 +152,8 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
> extern int pm_genpd_init(struct generic_pm_domain *genpd,
> struct dev_power_governor *gov, bool is_off);
> extern int pm_genpd_remove(struct generic_pm_domain *genpd);
> +extern int dev_pm_genpd_set_performance_state(struct device *dev,
> + unsigned int state);
>
> extern struct dev_power_governor simple_qos_governor;
> extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -188,6 +194,12 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
> return -ENOTSUPP;
> }
>
> +static inline int dev_pm_genpd_set_performance_state(struct device *dev,
> + unsigned int state)
> +{
> + return -ENOTSUPP;
> +}
> +
> #define simple_qos_governor (*(struct dev_power_governor *)(NULL))
> #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
> #endif
> --
> 2.15.0.rc1.236.g92ea95045093
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH V13 1/7] PM / Domains: Add support to select performance-state of domains
2017-10-11 7:24 ` [PATCH V11 1/7] PM / Domains: Add support to select performance-state of domains Viresh Kumar
2017-10-11 11:27 ` Ulf Hansson
2017-10-12 7:09 ` [PATCH V12 " Viresh Kumar
@ 2017-10-12 9:37 ` Viresh Kumar
2017-10-12 9:43 ` Ulf Hansson
2017-10-16 1:59 ` Kevin Hilman
2 siblings, 2 replies; 16+ messages in thread
From: Viresh Kumar @ 2017-10-12 9:37 UTC (permalink / raw)
To: Rafael Wysocki, ulf.hansson, Kevin Hilman
Cc: Viresh Kumar, linux-pm, Vincent Guittot, Stephen Boyd,
Nishanth Menon, robh+dt, lina.iyer, rnayak, sudeep.holla,
linux-kernel, Len Brown, Pavel Machek, Andy Gross, David Brown
Some platforms have the capability to configure the performance state of
PM domains. This patch enhances the genpd core to support such
platforms.
The performance levels (within the genpd core) are identified by
positive integer values, a lower value represents lower performance
state.
This patch adds a new genpd API, which is called by user drivers (like
OPP framework):
- int dev_pm_genpd_set_performance_state(struct device *dev,
unsigned int state);
This updates the performance state constraint of the device on its PM
domain. On success, the genpd will have its performance state set to a
value which is >= "state" passed to this routine. The genpd core calls
the genpd->set_performance_state() callback, if implemented,
else -ENODEV is returned to the caller.
The PM domain drivers need to implement the following callback if they
want to support performance states.
- int (*set_performance_state)(struct generic_pm_domain *genpd,
unsigned int state);
This is called internally by the genpd core on several occasions. The
genpd core passes the genpd pointer and the aggregate of the
performance states of the devices supported by that genpd to this
callback. This callback must update the performance state of the genpd
(in a platform dependent way).
The power domains can avoid supplying above callback, if they don't
support setting performance-states.
Currently we aren't propagating performance state changes of a subdomain
to its masters as we don't have hardware that needs it right now. Over
that, the performance states of subdomain and its masters may not have
one-to-one mapping and would require additional information. We can get
back to this once we have hardware that needs it.
Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V13:
- Don't return directly from a locked section, drop the lock as well.
- Update the performance_state field for the device even in the case
state == genpd->performance_state.
- Always call genpd->set_performance_state() from power-on.
- Update device's performance state before traversing the list of
devices to find highest state, otherwise we will take previous state
of the device into account.
- Check state again after traversing the list of devices to see if state
changed.
drivers/base/power/domain.c | 98 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_domain.h | 12 ++++++
2 files changed, 110 insertions(+)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a6e4c8d7d837..7e01ae364d78 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -237,6 +237,95 @@ static void genpd_update_accounting(struct generic_pm_domain *genpd)
static inline void genpd_update_accounting(struct generic_pm_domain *genpd) {}
#endif
+/**
+ * dev_pm_genpd_set_performance_state- Set performance state of device's power
+ * domain.
+ *
+ * @dev: Device for which the performance-state needs to be set.
+ * @state: Target performance state of the device. This can be set as 0 when the
+ * device doesn't have any performance state constraints left (And so
+ * the device wouldn't participate anymore to find the target
+ * performance state of the genpd).
+ *
+ * It is assumed that the users guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ *
+ * Returns 0 on success and negative error values on failures.
+ */
+int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
+{
+ struct generic_pm_domain *genpd;
+ struct generic_pm_domain_data *gpd_data, *pd_data;
+ struct pm_domain_data *pdd;
+ unsigned int prev;
+ int ret = 0;
+
+ genpd = dev_to_genpd(dev);
+ if (IS_ERR(genpd))
+ return -ENODEV;
+
+ if (unlikely(!genpd->set_performance_state))
+ return -EINVAL;
+
+ if (unlikely(!dev->power.subsys_data ||
+ !dev->power.subsys_data->domain_data)) {
+ WARN_ON(1);
+ return -EINVAL;
+ }
+
+ genpd_lock(genpd);
+
+ gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+ prev = gpd_data->performance_state;
+ gpd_data->performance_state = state;
+
+ /* New requested state is same as Max requested state */
+ if (state == genpd->performance_state)
+ goto unlock;
+
+ /* New requested state is higher than Max requested state */
+ if (state > genpd->performance_state)
+ goto update_state;
+
+ /* Traverse all devices within the domain */
+ list_for_each_entry(pdd, &genpd->dev_list, list_node) {
+ pd_data = to_gpd_data(pdd);
+
+ if (pd_data->performance_state > state)
+ state = pd_data->performance_state;
+ }
+
+ if (state == genpd->performance_state)
+ goto unlock;
+
+ /*
+ * We aren't propagating performance state changes of a subdomain to its
+ * masters as we don't have hardware that needs it. Over that, the
+ * performance states of subdomain and its masters may not have
+ * one-to-one mapping and would require additional information. We can
+ * get back to this once we have hardware that needs it. For that
+ * reason, we don't have to consider performance state of the subdomains
+ * of genpd here.
+ */
+
+update_state:
+ if (genpd_status_on(genpd)) {
+ ret = genpd->set_performance_state(genpd, state);
+ if (ret) {
+ gpd_data->performance_state = prev;
+ goto unlock;
+ }
+ }
+
+ genpd->performance_state = state;
+
+unlock:
+ genpd_unlock(genpd);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
+
static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
{
unsigned int state_idx = genpd->state_idx;
@@ -256,6 +345,15 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
return ret;
elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
+
+ if (unlikely(genpd->set_performance_state)) {
+ ret = genpd->set_performance_state(genpd, genpd->performance_state);
+ if (ret) {
+ pr_warn("%s: Failed to set performance state %d (%d)\n",
+ genpd->name, genpd->performance_state, ret);
+ }
+ }
+
if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
return ret;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 84f423d5633e..9af0356bd69c 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -64,8 +64,11 @@ struct generic_pm_domain {
unsigned int device_count; /* Number of devices */
unsigned int suspended_count; /* System suspend device counter */
unsigned int prepared_count; /* Suspend counter of prepared devices */
+ unsigned int performance_state; /* Aggregated max performance state */
int (*power_off)(struct generic_pm_domain *domain);
int (*power_on)(struct generic_pm_domain *domain);
+ int (*set_performance_state)(struct generic_pm_domain *genpd,
+ unsigned int state);
struct gpd_dev_ops dev_ops;
s64 max_off_time_ns; /* Maximum allowed "suspended" time. */
bool max_off_time_changed;
@@ -121,6 +124,7 @@ struct generic_pm_domain_data {
struct pm_domain_data base;
struct gpd_timing_data td;
struct notifier_block nb;
+ unsigned int performance_state;
void *data;
};
@@ -148,6 +152,8 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
extern int pm_genpd_init(struct generic_pm_domain *genpd,
struct dev_power_governor *gov, bool is_off);
extern int pm_genpd_remove(struct generic_pm_domain *genpd);
+extern int dev_pm_genpd_set_performance_state(struct device *dev,
+ unsigned int state);
extern struct dev_power_governor simple_qos_governor;
extern struct dev_power_governor pm_domain_always_on_gov;
@@ -188,6 +194,12 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
return -ENOTSUPP;
}
+static inline int dev_pm_genpd_set_performance_state(struct device *dev,
+ unsigned int state)
+{
+ return -ENOTSUPP;
+}
+
#define simple_qos_governor (*(struct dev_power_governor *)(NULL))
#define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
#endif
--
2.15.0.rc1.236.g92ea95045093
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH V13 1/7] PM / Domains: Add support to select performance-state of domains
2017-10-12 9:37 ` [PATCH V13 " Viresh Kumar
@ 2017-10-12 9:43 ` Ulf Hansson
2017-10-16 1:59 ` Kevin Hilman
1 sibling, 0 replies; 16+ messages in thread
From: Ulf Hansson @ 2017-10-12 9:43 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, Kevin Hilman, linux-pm, Vincent Guittot,
Stephen Boyd, Nishanth Menon, Rob Herring, Lina Iyer,
Rajendra Nayak, Sudeep Holla, linux-kernel, Len Brown,
Pavel Machek, Andy Gross, David Brown
On 12 October 2017 at 11:37, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Some platforms have the capability to configure the performance state of
> PM domains. This patch enhances the genpd core to support such
> platforms.
>
> The performance levels (within the genpd core) are identified by
> positive integer values, a lower value represents lower performance
> state.
>
> This patch adds a new genpd API, which is called by user drivers (like
> OPP framework):
>
> - int dev_pm_genpd_set_performance_state(struct device *dev,
> unsigned int state);
>
> This updates the performance state constraint of the device on its PM
> domain. On success, the genpd will have its performance state set to a
> value which is >= "state" passed to this routine. The genpd core calls
> the genpd->set_performance_state() callback, if implemented,
> else -ENODEV is returned to the caller.
>
> The PM domain drivers need to implement the following callback if they
> want to support performance states.
>
> - int (*set_performance_state)(struct generic_pm_domain *genpd,
> unsigned int state);
>
> This is called internally by the genpd core on several occasions. The
> genpd core passes the genpd pointer and the aggregate of the
> performance states of the devices supported by that genpd to this
> callback. This callback must update the performance state of the genpd
> (in a platform dependent way).
>
> The power domains can avoid supplying above callback, if they don't
> support setting performance-states.
>
> Currently we aren't propagating performance state changes of a subdomain
> to its masters as we don't have hardware that needs it right now. Over
> that, the performance states of subdomain and its masters may not have
> one-to-one mapping and would require additional information. We can get
> back to this once we have hardware that needs it.
>
> Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Yeah, this looks nice a clean now!
Acked-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> V13:
> - Don't return directly from a locked section, drop the lock as well.
> - Update the performance_state field for the device even in the case
> state == genpd->performance_state.
> - Always call genpd->set_performance_state() from power-on.
> - Update device's performance state before traversing the list of
> devices to find highest state, otherwise we will take previous state
> of the device into account.
> - Check state again after traversing the list of devices to see if state
> changed.
>
> drivers/base/power/domain.c | 98 +++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 12 ++++++
> 2 files changed, 110 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a6e4c8d7d837..7e01ae364d78 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -237,6 +237,95 @@ static void genpd_update_accounting(struct generic_pm_domain *genpd)
> static inline void genpd_update_accounting(struct generic_pm_domain *genpd) {}
> #endif
>
> +/**
> + * dev_pm_genpd_set_performance_state- Set performance state of device's power
> + * domain.
> + *
> + * @dev: Device for which the performance-state needs to be set.
> + * @state: Target performance state of the device. This can be set as 0 when the
> + * device doesn't have any performance state constraints left (And so
> + * the device wouldn't participate anymore to find the target
> + * performance state of the genpd).
> + *
> + * It is assumed that the users guarantee that the genpd wouldn't be detached
> + * while this routine is getting called.
> + *
> + * Returns 0 on success and negative error values on failures.
> + */
> +int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
> +{
> + struct generic_pm_domain *genpd;
> + struct generic_pm_domain_data *gpd_data, *pd_data;
> + struct pm_domain_data *pdd;
> + unsigned int prev;
> + int ret = 0;
> +
> + genpd = dev_to_genpd(dev);
> + if (IS_ERR(genpd))
> + return -ENODEV;
> +
> + if (unlikely(!genpd->set_performance_state))
> + return -EINVAL;
> +
> + if (unlikely(!dev->power.subsys_data ||
> + !dev->power.subsys_data->domain_data)) {
> + WARN_ON(1);
> + return -EINVAL;
> + }
> +
> + genpd_lock(genpd);
> +
> + gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> + prev = gpd_data->performance_state;
> + gpd_data->performance_state = state;
> +
> + /* New requested state is same as Max requested state */
> + if (state == genpd->performance_state)
> + goto unlock;
> +
> + /* New requested state is higher than Max requested state */
> + if (state > genpd->performance_state)
> + goto update_state;
> +
> + /* Traverse all devices within the domain */
> + list_for_each_entry(pdd, &genpd->dev_list, list_node) {
> + pd_data = to_gpd_data(pdd);
> +
> + if (pd_data->performance_state > state)
> + state = pd_data->performance_state;
> + }
> +
> + if (state == genpd->performance_state)
> + goto unlock;
> +
> + /*
> + * We aren't propagating performance state changes of a subdomain to its
> + * masters as we don't have hardware that needs it. Over that, the
> + * performance states of subdomain and its masters may not have
> + * one-to-one mapping and would require additional information. We can
> + * get back to this once we have hardware that needs it. For that
> + * reason, we don't have to consider performance state of the subdomains
> + * of genpd here.
> + */
> +
> +update_state:
> + if (genpd_status_on(genpd)) {
> + ret = genpd->set_performance_state(genpd, state);
> + if (ret) {
> + gpd_data->performance_state = prev;
> + goto unlock;
> + }
> + }
> +
> + genpd->performance_state = state;
> +
> +unlock:
> + genpd_unlock(genpd);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(dev_pm_genpd_set_performance_state);
> +
> static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> {
> unsigned int state_idx = genpd->state_idx;
> @@ -256,6 +345,15 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> return ret;
>
> elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
> +
> + if (unlikely(genpd->set_performance_state)) {
> + ret = genpd->set_performance_state(genpd, genpd->performance_state);
> + if (ret) {
> + pr_warn("%s: Failed to set performance state %d (%d)\n",
> + genpd->name, genpd->performance_state, ret);
> + }
> + }
> +
> if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
> return ret;
>
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 84f423d5633e..9af0356bd69c 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -64,8 +64,11 @@ struct generic_pm_domain {
> unsigned int device_count; /* Number of devices */
> unsigned int suspended_count; /* System suspend device counter */
> unsigned int prepared_count; /* Suspend counter of prepared devices */
> + unsigned int performance_state; /* Aggregated max performance state */
> int (*power_off)(struct generic_pm_domain *domain);
> int (*power_on)(struct generic_pm_domain *domain);
> + int (*set_performance_state)(struct generic_pm_domain *genpd,
> + unsigned int state);
> struct gpd_dev_ops dev_ops;
> s64 max_off_time_ns; /* Maximum allowed "suspended" time. */
> bool max_off_time_changed;
> @@ -121,6 +124,7 @@ struct generic_pm_domain_data {
> struct pm_domain_data base;
> struct gpd_timing_data td;
> struct notifier_block nb;
> + unsigned int performance_state;
> void *data;
> };
>
> @@ -148,6 +152,8 @@ extern int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
> extern int pm_genpd_init(struct generic_pm_domain *genpd,
> struct dev_power_governor *gov, bool is_off);
> extern int pm_genpd_remove(struct generic_pm_domain *genpd);
> +extern int dev_pm_genpd_set_performance_state(struct device *dev,
> + unsigned int state);
>
> extern struct dev_power_governor simple_qos_governor;
> extern struct dev_power_governor pm_domain_always_on_gov;
> @@ -188,6 +194,12 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
> return -ENOTSUPP;
> }
>
> +static inline int dev_pm_genpd_set_performance_state(struct device *dev,
> + unsigned int state)
> +{
> + return -ENOTSUPP;
> +}
> +
> #define simple_qos_governor (*(struct dev_power_governor *)(NULL))
> #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
> #endif
> --
> 2.15.0.rc1.236.g92ea95045093
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V13 1/7] PM / Domains: Add support to select performance-state of domains
2017-10-12 9:37 ` [PATCH V13 " Viresh Kumar
2017-10-12 9:43 ` Ulf Hansson
@ 2017-10-16 1:59 ` Kevin Hilman
2017-10-16 8:56 ` Viresh Kumar
1 sibling, 1 reply; 16+ messages in thread
From: Kevin Hilman @ 2017-10-16 1:59 UTC (permalink / raw)
To: Viresh Kumar
Cc: Rafael Wysocki, ulf.hansson, linux-pm, Vincent Guittot,
Stephen Boyd, Nishanth Menon, robh+dt, lina.iyer, rnayak,
sudeep.holla, linux-kernel, Len Brown, Pavel Machek, Andy Gross,
David Brown
Viresh Kumar <viresh.kumar@linaro.org> writes:
> Some platforms have the capability to configure the performance state of
> PM domains. This patch enhances the genpd core to support such
> platforms.
>
> The performance levels (within the genpd core) are identified by
> positive integer values, a lower value represents lower performance
> state.
>
> This patch adds a new genpd API, which is called by user drivers (like
> OPP framework):
>
> - int dev_pm_genpd_set_performance_state(struct device *dev,
> unsigned int state);
>
> This updates the performance state constraint of the device on its PM
> domain. On success, the genpd will have its performance state set to a
> value which is >= "state" passed to this routine. The genpd core calls
> the genpd->set_performance_state() callback, if implemented,
> else -ENODEV is returned to the caller.
>
> The PM domain drivers need to implement the following callback if they
> want to support performance states.
>
> - int (*set_performance_state)(struct generic_pm_domain *genpd,
> unsigned int state);
>
> This is called internally by the genpd core on several occasions. The
> genpd core passes the genpd pointer and the aggregate of the
> performance states of the devices supported by that genpd to this
> callback. This callback must update the performance state of the genpd
> (in a platform dependent way).
>
> The power domains can avoid supplying above callback, if they don't
> support setting performance-states.
>
> Currently we aren't propagating performance state changes of a subdomain
> to its masters as we don't have hardware that needs it right now. Over
> that, the performance states of subdomain and its masters may not have
> one-to-one mapping and would require additional information. We can get
> back to this once we have hardware that needs it.
>
> Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
OK, I really like the OPP-related changes suggested by Ulf, and you've
removed a lot of the complexity that made it a bit confusing to follow.
It's definitley cleaned up and much easier to follow.
Thanks for your persistence. This is definitely a needed feature.
I have some usecases in mind where the performance state might need to
be selected based on OPP voltage, but that's now a change that can be
added later when that feature is needed.
Reviewed-by: Kevin Hilman <khilman@baylibre.com>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH V13 1/7] PM / Domains: Add support to select performance-state of domains
2017-10-16 1:59 ` Kevin Hilman
@ 2017-10-16 8:56 ` Viresh Kumar
0 siblings, 0 replies; 16+ messages in thread
From: Viresh Kumar @ 2017-10-16 8:56 UTC (permalink / raw)
To: Kevin Hilman
Cc: Rafael Wysocki, ulf.hansson, linux-pm, Vincent Guittot,
Stephen Boyd, Nishanth Menon, robh+dt, lina.iyer, rnayak,
sudeep.holla, linux-kernel, Len Brown, Pavel Machek, Andy Gross,
David Brown
On 16-10-17, 03:59, Kevin Hilman wrote:
> I have some usecases in mind where the performance state might need to
> be selected based on OPP voltage, but that's now a change that can be
> added later when that feature is needed.
I will be more than happy to get that in.
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
Thanks.
Do you think its a good time to get some bindings for this stuff now? The
bindings would be required for only users and not PM domains themselves for now.
Users are:
- OPP core: need per-OPP entry value for performance state.
- Devices: devices directly using the PM domain without OPP core (don't have
DVFS). They can use the same property.
It might look similar to the properties I initially started with, I can just get
them refreshed.
--
viresh
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-10-16 8:56 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-11 7:24 [PATCH V11 0/7] PM / Domains: Performance state support Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 1/7] PM / Domains: Add support to select performance-state of domains Viresh Kumar
2017-10-11 11:27 ` Ulf Hansson
2017-10-12 7:09 ` [PATCH V12 " Viresh Kumar
2017-10-12 8:01 ` Ulf Hansson
2017-10-12 9:37 ` [PATCH V13 " Viresh Kumar
2017-10-12 9:43 ` Ulf Hansson
2017-10-16 1:59 ` Kevin Hilman
2017-10-16 8:56 ` Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 2/7] OPP: Support updating performance state of device's power domain Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 3/7] OPP: Add dev_pm_opp_{un}register_get_pstate_helper() Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 4/7] soc: qcom: rpmpd: Add driver to model cx/mx power domains Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 5/7] soc: qcom: rpmpd: Add support for set performance state Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 6/7] OPP: qcom: Add support to get performance states corresponding to OPPs Viresh Kumar
2017-10-11 7:24 ` [PATCH V11 7/7] mmc: sdhci-msm: Adapt the driver to use OPPs to set clocks/performance state Viresh Kumar
2017-10-11 11:43 ` [PATCH V11 0/7] PM / Domains: Performance state support Ulf Hansson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.