* [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-06-21 7:10 [PATCH V8 0/6] PM / Domains: Power domain performance states Viresh Kumar
@ 2017-06-21 7:10 ` Viresh Kumar
2017-07-17 12:38 ` Ulf Hansson
2017-06-21 7:10 ` [PATCH V8 2/6] PM / OPP: Support updating performance state of device's power domains Viresh Kumar
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Viresh Kumar @ 2017-06-21 7:10 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
their Power Domains. The performance levels are identified by positive
integer values, a lower value represents lower performance state.
This patch adds a new genpd API: pm_genpd_update_performance_state().
The caller passes the affected device and the frequency representing its
next DVFS state.
The power domains get two new callbacks:
- get_performance_state(): This is called by the genpd core to retrieve
the performance state (integer value) corresponding to a target
frequency for the device. This state is used by the genpd core to find
the highest requested state by all the devices powered by a domain.
- set_performance_state(): The highest state retrieved from above
interface is then passed to this callback to finally program the
performance state of the power domain.
The power domains can avoid supplying these callbacks, if they don't
support setting performance-states.
A power domain may have only get_performance_state() callback, if it
doesn't have the capability of changing the performance state itself but
someone in its parent hierarchy has.
A power domain may have only set_performance_state(), if it doesn't have
any direct devices below it but subdomains. And so the
get_performance_state() will never get called from the core.
The more common case would be to have both the callbacks set.
Another API, pm_genpd_has_performance_state(), is also added to let
other parts of the kernel check if the power domain of a device supports
performance-states or not. This could have been done from
pm_genpd_update_performance_state() as well, but that routine gets
called every time we do DVFS for the device and it wouldn't be optimal
in that case.
Note that, the performance level as returned by
->get_performance_state() for the parent domain of a device is used for
all domains in parent hierarchy.
Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/base/power/domain.c | 223 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/pm_domain.h | 22 +++++
2 files changed, 245 insertions(+)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 71c95ad808d5..d506be9ff1f7 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -466,6 +466,229 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
return NOTIFY_DONE;
}
+/*
+ * Returns true if anyone in genpd's parent hierarchy has
+ * set_performance_state() set.
+ */
+static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
+{
+ struct gpd_link *link;
+
+ if (genpd->set_performance_state)
+ return true;
+
+ list_for_each_entry(link, &genpd->slave_links, slave_node) {
+ if (genpd_has_set_performance_state(link->master))
+ return true;
+ }
+
+ return false;
+}
+
+/**
+ * pm_genpd_has_performance_state - Checks if power domain does performance
+ * state management.
+ *
+ * @dev: Device whose power domain is getting inquired.
+ *
+ * This must be called by the user drivers, before they start calling
+ * pm_genpd_update_performance_state(), to guarantee that all dependencies are
+ * met and the device's genpd supports performance states.
+ *
+ * It is assumed that the user driver guarantees that the genpd wouldn't be
+ * detached while this routine is getting called.
+ *
+ * Returns "true" if device's genpd supports performance states, "false"
+ * otherwise.
+ */
+bool pm_genpd_has_performance_state(struct device *dev)
+{
+ struct generic_pm_domain *genpd = genpd_lookup_dev(dev);
+
+ /* The parent domain must have set get_performance_state() */
+ if (!IS_ERR(genpd) && genpd->get_performance_state) {
+ if (genpd_has_set_performance_state(genpd))
+ return true;
+
+ /*
+ * A genpd with ->get_performance_state() callback must also
+ * allow setting performance state.
+ */
+ dev_err(dev, "genpd doesn't support setting performance state\n");
+ }
+
+ return false;
+}
+EXPORT_SYMBOL_GPL(pm_genpd_has_performance_state);
+
+/*
+ * Re-evaluate performance state of a power domain. Finds the highest requested
+ * performance state by the devices and subdomains within the power domain and
+ * then tries to change its performance state. If the power domain doesn't have
+ * a set_performance_state() callback, then we move the request to its parent
+ * power domain.
+ *
+ * Locking: Access (or update) to device's "pd_data->performance_state" field
+ * happens only with parent domain's lock held. Subdomains have their
+ * "genpd->performance_state" protected with their own lock (and they are the
+ * only user of this field) and their per-parent-domain
+ * "link->performance_state" field is protected with individual parent power
+ * domain's lock and is only accessed/updated with that lock held.
+ */
+static int genpd_update_performance_state(struct generic_pm_domain *genpd,
+ int depth)
+{
+ struct generic_pm_domain_data *pd_data;
+ struct generic_pm_domain *master;
+ struct pm_domain_data *pdd;
+ unsigned int state = 0, prev;
+ struct gpd_link *link;
+ int ret;
+
+ /* 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;
+ }
+
+ /* Traverse all subdomains within the domain */
+ list_for_each_entry(link, &genpd->master_links, master_node) {
+ if (link->performance_state > state)
+ state = link->performance_state;
+ }
+
+ if (genpd->performance_state == state)
+ return 0;
+
+ if (genpd->set_performance_state) {
+ ret = genpd->set_performance_state(genpd, state);
+ if (!ret)
+ genpd->performance_state = state;
+
+ return ret;
+ }
+
+ /*
+ * Not all domains support updating performance state. Move on to their
+ * parent domains in that case.
+ */
+ prev = genpd->performance_state;
+
+ list_for_each_entry(link, &genpd->slave_links, slave_node) {
+ master = link->master;
+
+ genpd_lock_nested(master, depth + 1);
+
+ link->performance_state = state;
+ ret = genpd_update_performance_state(master, depth + 1);
+ if (ret)
+ link->performance_state = prev;
+
+ genpd_unlock(master);
+
+ if (ret)
+ goto err;
+ }
+
+ /*
+ * The parent domains are updated now, lets get genpd performance_state
+ * in sync with those.
+ */
+ genpd->performance_state = state;
+ return 0;
+
+err:
+ list_for_each_entry_continue_reverse(link, &genpd->slave_links,
+ slave_node) {
+ master = link->master;
+
+ genpd_lock_nested(master, depth + 1);
+ link->performance_state = prev;
+ if (genpd_update_performance_state(master, depth + 1))
+ pr_err("%s: Failed to roll back to %d performance state\n",
+ genpd->name, prev);
+ genpd_unlock(master);
+ }
+
+ return ret;
+}
+
+static int __dev_update_performance_state(struct device *dev, int state)
+{
+ struct generic_pm_domain_data *gpd_data;
+ int ret;
+
+ spin_lock_irq(&dev->power.lock);
+
+ if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data) {
+ ret = -ENODEV;
+ goto unlock;
+ }
+
+ gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
+
+ ret = gpd_data->performance_state;
+ gpd_data->performance_state = state;
+
+unlock:
+ spin_unlock_irq(&dev->power.lock);
+
+ return ret;
+}
+
+/**
+ * pm_genpd_update_performance_state - Update performance state of device's
+ * parent power domain for the target frequency for the device.
+ *
+ * @dev: Device for which the performance-state needs to be adjusted.
+ * @rate: Device's next frequency. 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).
+ *
+ * This must be called by the user drivers (as many times as they want) only
+ * after pm_genpd_has_performance_state() is called (only once) and that
+ * returned "true".
+ *
+ * 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 pm_genpd_update_performance_state(struct device *dev, unsigned long rate)
+{
+ struct generic_pm_domain *genpd = dev_to_genpd(dev);
+ int ret, state;
+
+ if (IS_ERR(genpd))
+ return -ENODEV;
+
+ genpd_lock(genpd);
+
+ state = genpd->get_performance_state(dev, rate);
+ if (state < 0) {
+ ret = state;
+ goto unlock;
+ }
+
+ state = __dev_update_performance_state(dev, state);
+ if (state < 0) {
+ ret = state;
+ goto unlock;
+ }
+
+ ret = genpd_update_performance_state(genpd, 0);
+ if (ret)
+ __dev_update_performance_state(dev, state);
+
+unlock:
+ genpd_unlock(genpd);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(pm_genpd_update_performance_state);
+
/**
* genpd_power_off_work_fn - Power off PM domain whose subdomain count is 0.
* @work: Work structure used for scheduling the execution of this function.
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index b7803a251044..bf90177208a2 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -63,8 +63,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 */
int (*power_off)(struct generic_pm_domain *domain);
int (*power_on)(struct generic_pm_domain *domain);
+ int (*get_performance_state)(struct device *dev, unsigned long rate);
+ int (*set_performance_state)(struct generic_pm_domain *domain,
+ unsigned int state);
struct gpd_dev_ops dev_ops;
s64 max_off_time_ns; /* Maximum allowed "suspended" time. */
bool max_off_time_changed;
@@ -99,6 +103,9 @@ struct gpd_link {
struct list_head master_node;
struct generic_pm_domain *slave;
struct list_head slave_node;
+
+ /* Sub-domain's per-parent domain performance state */
+ unsigned int performance_state;
};
struct gpd_timing_data {
@@ -118,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 +156,9 @@ extern int pm_genpd_remove(struct generic_pm_domain *genpd);
extern struct dev_power_governor simple_qos_governor;
extern struct dev_power_governor pm_domain_always_on_gov;
+extern bool pm_genpd_has_performance_state(struct device *dev);
+extern int pm_genpd_update_performance_state(struct device *dev,
+ unsigned long rate);
#else
static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
@@ -185,6 +196,17 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
return -ENOTSUPP;
}
+static inline bool pm_genpd_has_performance_state(struct device *dev)
+{
+ return false;
+}
+
+static inline int pm_genpd_update_performance_state(struct device *dev,
+ unsigned long rate)
+{
+ return -ENOTSUPP;
+}
+
#define simple_qos_governor (*(struct dev_power_governor *)(NULL))
#define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
#endif
--
2.13.0.71.gd7076ec9c9cb
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-06-21 7:10 ` [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains Viresh Kumar
@ 2017-07-17 12:38 ` Ulf Hansson
2017-07-19 12:37 ` Viresh Kumar
0 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2017-07-17 12:38 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 21 June 2017 at 09:10, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Some platforms have the capability to configure the performance state of
> their Power Domains. The performance levels are identified by positive
> integer values, a lower value represents lower performance state.
>
> This patch adds a new genpd API: pm_genpd_update_performance_state().
> The caller passes the affected device and the frequency representing its
> next DVFS state.
>
> The power domains get two new callbacks:
>
> - get_performance_state(): This is called by the genpd core to retrieve
> the performance state (integer value) corresponding to a target
> frequency for the device. This state is used by the genpd core to find
> the highest requested state by all the devices powered by a domain.
Please clarify this a bit more.
I guess what you want to say is that genpd aggregates all requested
performance state of all its devices and its subdomains, to be able to
set a correct (highest requested) performance state.
Moreover, could you perhaps explain a bit on *when* this callback
becomes invoked.
>
> - set_performance_state(): The highest state retrieved from above
> interface is then passed to this callback to finally program the
> performance state of the power domain.
When will this callback be invoked?
What happens when a power domain gets powered off and then on. Is the
performance state restored? Please elaborate a bit on this.
>
> The power domains can avoid supplying these callbacks, if they don't
> support setting performance-states.
>
> A power domain may have only get_performance_state() callback, if it
> doesn't have the capability of changing the performance state itself but
> someone in its parent hierarchy has.
>
> A power domain may have only set_performance_state(), if it doesn't have
> any direct devices below it but subdomains. And so the
> get_performance_state() will never get called from the core.
>
It seems like the ->get_perfomance_state() is a device specific
callback, while the ->set_performance_state() is a genpd domain
callback.
I am wondering whether we should try to improve the names of the
callbacks to reflect this.
> The more common case would be to have both the callbacks set.
>
> Another API, pm_genpd_has_performance_state(), is also added to let
> other parts of the kernel check if the power domain of a device supports
> performance-states or not. This could have been done from
I think a better name of this function is:
dev_pm_genpd_has_performance_state(). What do you think?
We might even want to decide to explicitly stay using the terminology
"DVFS" instead. In such case, perhaps converting the names of the
callbacks/API to use "dvfs" instead. For the API added here, maybe
dev_pm_genpd_can_dvfs().
> pm_genpd_update_performance_state() as well, but that routine gets
> called every time we do DVFS for the device and it wouldn't be optimal
> in that case.
So pm_genpd_update_performance_state() is also a new API added in
$subject patch. But there is no information about it in the changelog,
besides the above. Please add that.
Moreover, perhaps we should rename the function to dev_pm_genpd_set_dvfs()
>
> Note that, the performance level as returned by
> ->get_performance_state() for the parent domain of a device is used for
> all domains in parent hierarchy.
Please clarify a bit on this. What exactly does this mean?
>
> Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> drivers/base/power/domain.c | 223 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/pm_domain.h | 22 +++++
> 2 files changed, 245 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 71c95ad808d5..d506be9ff1f7 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -466,6 +466,229 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
> return NOTIFY_DONE;
> }
>
> +/*
> + * Returns true if anyone in genpd's parent hierarchy has
> + * set_performance_state() set.
> + */
> +static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
> +{
So this function will be become in-directly called by generic drivers
that supports DVFS of the genpd for their devices.
I think the data you validate here would be better to be pre-validated
at pm_genpd_init() and at pm_genpd_add|remove_subdomain() and the
result stored in a variable in the genpd struct. Especially when a
subdomain is added, that is a point when you can verify the
*_performance_state() callbacks, and thus make sure it's a correct
setup from the topology point of view.
> + struct gpd_link *link;
> +
> + if (genpd->set_performance_state)
> + return true;
> +
> + list_for_each_entry(link, &genpd->slave_links, slave_node) {
> + if (genpd_has_set_performance_state(link->master))
> + return true;
> + }
> +
> + return false;
> +}
> +
> +/**
> + * pm_genpd_has_performance_state - Checks if power domain does performance
> + * state management.
> + *
> + * @dev: Device whose power domain is getting inquired.
> + *
> + * This must be called by the user drivers, before they start calling
> + * pm_genpd_update_performance_state(), to guarantee that all dependencies are
> + * met and the device's genpd supports performance states.
> + *
> + * It is assumed that the user driver guarantees that the genpd wouldn't be
> + * detached while this routine is getting called.
> + *
> + * Returns "true" if device's genpd supports performance states, "false"
> + * otherwise.
> + */
> +bool pm_genpd_has_performance_state(struct device *dev)
> +{
> + struct generic_pm_domain *genpd = genpd_lookup_dev(dev);
> +
> + /* The parent domain must have set get_performance_state() */
This comment is wrong. This is not about *parent* domains.
Instead I think it should say: "The genpd must have the
->get_performance_state() assigned." ...
> + if (!IS_ERR(genpd) && genpd->get_performance_state) {
> + if (genpd_has_set_performance_state(genpd))
> + return true;
... while this is about verifying that some of the domains (genpds) in
domain topology has a ->set_performance_state() callback.
In other words, either the genpd or some of its masters must have a
->set_performance_state() callback.
That makes me wonder: what will happen if there are more than one
master having a ->set_perfomance_state() callback assigned? I guess
that is non-allowed configuration?
> +
> + /*
> + * A genpd with ->get_performance_state() callback must also
> + * allow setting performance state.
> + */
> + dev_err(dev, "genpd doesn't support setting performance state\n");
> + }
> +
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(pm_genpd_has_performance_state);
> +
> +/*
> + * Re-evaluate performance state of a power domain. Finds the highest requested
> + * performance state by the devices and subdomains within the power domain and
> + * then tries to change its performance state. If the power domain doesn't have
> + * a set_performance_state() callback, then we move the request to its parent
> + * power domain.
> + *
> + * Locking: Access (or update) to device's "pd_data->performance_state" field
> + * happens only with parent domain's lock held. Subdomains have their
What is a *parent* domain here?
In genpd we try to use the terminology of master- and sub-domains.
Could you re-phrase this to get some clarity on what you try to
explain from the above?
> + * "genpd->performance_state" protected with their own lock (and they are the
> + * only user of this field) and their per-parent-domain
> + * "link->performance_state" field is protected with individual parent power
> + * domain's lock and is only accessed/updated with that lock held.
> + */
I recall we discussed this off-list, but I am not sure this was the
conclusion on how to solve the locking. :-) More comments below.
> +static int genpd_update_performance_state(struct generic_pm_domain *genpd,
> + int depth)
> +{
> + struct generic_pm_domain_data *pd_data;
> + struct generic_pm_domain *master;
> + struct pm_domain_data *pdd;
> + unsigned int state = 0, prev;
> + struct gpd_link *link;
> + int ret;
> +
> + /* 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;
> + }
I don't think walking the list of devices for each master domain is
necessary, unless the aggregated performance state from the subdomain
was increased.
> +
> + /* Traverse all subdomains within the domain */
> + list_for_each_entry(link, &genpd->master_links, master_node) {
> + if (link->performance_state > state)
> + state = link->performance_state;
> + }
> +
>From a locking point of view we always traverse the topology from
bottom an up. In other words, we walk the genpd's ->slave_links, and
lock the masters in the order the are defined via the slave_links
list. The order is important to avoid deadlocks. I don't think you
should walk the master_links as being done above, especially not
without using locks.
> + if (genpd->performance_state == state)
> + return 0;
> +
> + if (genpd->set_performance_state) {
> + ret = genpd->set_performance_state(genpd, state);
> + if (!ret)
> + genpd->performance_state = state;
> +
> + return ret;
This looks wrong.
I think you should continue to walk upwards in the domain topology, as
there may be some other master than needs to get its performance state
updated.
> + }
> +
> + /*
> + * Not all domains support updating performance state. Move on to their
> + * parent domains in that case.
/s/parent/master
> + */
> + prev = genpd->performance_state;
> +
> + list_for_each_entry(link, &genpd->slave_links, slave_node) {
> + master = link->master;
> +
> + genpd_lock_nested(master, depth + 1);
> +
> + link->performance_state = state;
> + ret = genpd_update_performance_state(master, depth + 1);
> + if (ret)
> + link->performance_state = prev;
> +
> + genpd_unlock(master);
> +
> + if (ret)
> + goto err;
> + }
> +
A general comment is that I think you should look more closely in the
code of genpd_power_off|on(). And also how it calls the
->power_on|off() callbacks.
Depending whether you want to update the performance state of the
master domain before the subdomain or the opposite, you will find one
of them being suited for this case as well.
> + /*
> + * The parent domains are updated now, lets get genpd performance_state
> + * in sync with those.
> + */
> + genpd->performance_state = state;
> + return 0;
> +
> +err:
> + list_for_each_entry_continue_reverse(link, &genpd->slave_links,
> + slave_node) {
> + master = link->master;
> +
> + genpd_lock_nested(master, depth + 1);
> + link->performance_state = prev;
> + if (genpd_update_performance_state(master, depth + 1))
> + pr_err("%s: Failed to roll back to %d performance state\n",
> + genpd->name, prev);
> + genpd_unlock(master);
> + }
> +
> + return ret;
> +}
> +
> +static int __dev_update_performance_state(struct device *dev, int state)
Please use the prefix genpd, _genpd_ or __genpd for static functions.
> +{
> + struct generic_pm_domain_data *gpd_data;
> + int ret;
> +
> + spin_lock_irq(&dev->power.lock);
Actually there is no need to use this lock.
Because you hold the genpd lock here, then the device can't be removed
from its genpd and thus there is always a valid gpd_data.
> +
> + if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data) {
> + ret = -ENODEV;
> + goto unlock;
> + }
> +
> + gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> +
> + ret = gpd_data->performance_state;
> + gpd_data->performance_state = state;
> +
> +unlock:
> + spin_unlock_irq(&dev->power.lock);
> +
> + return ret;
> +}
> +
> +/**
> + * pm_genpd_update_performance_state - Update performance state of device's
> + * parent power domain for the target frequency for the device.
> + *
> + * @dev: Device for which the performance-state needs to be adjusted.
> + * @rate: Device's next frequency. 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).
> + *
> + * This must be called by the user drivers (as many times as they want) only
> + * after pm_genpd_has_performance_state() is called (only once) and that
> + * returned "true".
> + *
> + * 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 pm_genpd_update_performance_state(struct device *dev, unsigned long rate)
> +{
> + struct generic_pm_domain *genpd = dev_to_genpd(dev);
> + int ret, state;
> +
> + if (IS_ERR(genpd))
> + return -ENODEV;
> +
> + genpd_lock(genpd);
> +
> + state = genpd->get_performance_state(dev, rate);
> + if (state < 0) {
> + ret = state;
> + goto unlock;
> + }
> +
> + state = __dev_update_performance_state(dev, state);
> + if (state < 0) {
> + ret = state;
> + goto unlock;
> + }
> +
> + ret = genpd_update_performance_state(genpd, 0);
> + if (ret)
> + __dev_update_performance_state(dev, state);
> +
> +unlock:
> + genpd_unlock(genpd);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(pm_genpd_update_performance_state);
> +
> /**
> * genpd_power_off_work_fn - Power off PM domain whose subdomain count is 0.
> * @work: Work structure used for scheduling the execution of this function.
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index b7803a251044..bf90177208a2 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -63,8 +63,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 */
> int (*power_off)(struct generic_pm_domain *domain);
> int (*power_on)(struct generic_pm_domain *domain);
> + int (*get_performance_state)(struct device *dev, unsigned long rate);
> + int (*set_performance_state)(struct generic_pm_domain *domain,
> + unsigned int state);
> struct gpd_dev_ops dev_ops;
> s64 max_off_time_ns; /* Maximum allowed "suspended" time. */
> bool max_off_time_changed;
> @@ -99,6 +103,9 @@ struct gpd_link {
> struct list_head master_node;
> struct generic_pm_domain *slave;
> struct list_head slave_node;
> +
> + /* Sub-domain's per-parent domain performance state */
> + unsigned int performance_state;
How about aggregate_dvfs_state, and move it to the struct generic_pm_domain.
Because I think you only need to keep track one aggregated state per
domain and per each subdomain, right?
> };
>
> struct gpd_timing_data {
> @@ -118,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 +156,9 @@ extern int pm_genpd_remove(struct generic_pm_domain *genpd);
>
> extern struct dev_power_governor simple_qos_governor;
> extern struct dev_power_governor pm_domain_always_on_gov;
> +extern bool pm_genpd_has_performance_state(struct device *dev);
> +extern int pm_genpd_update_performance_state(struct device *dev,
> + unsigned long rate);
Please move these above governor defines, as to be consistent with
below changes.
> #else
>
> static inline struct generic_pm_domain_data *dev_gpd_data(struct device *dev)
> @@ -185,6 +196,17 @@ static inline int pm_genpd_remove(struct generic_pm_domain *genpd)
> return -ENOTSUPP;
> }
>
> +static inline bool pm_genpd_has_performance_state(struct device *dev)
> +{
> + return false;
> +}
> +
> +static inline int pm_genpd_update_performance_state(struct device *dev,
> + unsigned long rate)
> +{
> + return -ENOTSUPP;
> +}
> +
> #define simple_qos_governor (*(struct dev_power_governor *)(NULL))
> #define pm_domain_always_on_gov (*(struct dev_power_governor *)(NULL))
> #endif
> --
> 2.13.0.71.gd7076ec9c9cb
>
Huh, I hope my comments make some sense. It's a bit of tricky code,
when walking the domain topology and I think there are couple
optimization we can do while doing that. Hopefully you can understand
most of my comments at least. :-)
Feel free to ping me on IRC, if further explanations is needed.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-07-17 12:38 ` Ulf Hansson
@ 2017-07-19 12:37 ` Viresh Kumar
2017-07-21 8:35 ` Ulf Hansson
0 siblings, 1 reply; 17+ messages in thread
From: Viresh Kumar @ 2017-07-19 12:37 UTC (permalink / raw)
To: Ulf Hansson
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 17-07-17, 14:38, Ulf Hansson wrote:
> On 21 June 2017 at 09:10, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > Some platforms have the capability to configure the performance state of
> > their Power Domains. The performance levels are identified by positive
> > integer values, a lower value represents lower performance state.
> >
> > This patch adds a new genpd API: pm_genpd_update_performance_state().
> > The caller passes the affected device and the frequency representing its
> > next DVFS state.
> >
> > The power domains get two new callbacks:
> >
> > - get_performance_state(): This is called by the genpd core to retrieve
> > the performance state (integer value) corresponding to a target
> > frequency for the device. This state is used by the genpd core to find
> > the highest requested state by all the devices powered by a domain.
>
> Please clarify this a bit more.
>
> I guess what you want to say is that genpd aggregates all requested
> performance state of all its devices and its subdomains, to be able to
> set a correct (highest requested) performance state.
Right.
> Moreover, could you perhaps explain a bit on *when* this callback
> becomes invoked.
Sure, its done when pm_genpd_update_performance_state() is called for
a device. On such an event, the genpd core first calls
get_performance_state() and gets the device's target state. It then
aggregates the states of all devices/subdomains of the parent domain
of this device and finally calls set_performance_state() for the genpd.
> >
> > - set_performance_state(): The highest state retrieved from above
> > interface is then passed to this callback to finally program the
> > performance state of the power domain.
>
> When will this callback be invoked?
See above.
> What happens when a power domain gets powered off and then on. Is the
> performance state restored? Please elaborate a bit on this.
Can this happen while the genpd is still in use? If not then we
wouldn't have a problem here as the users of it would have revoked
their constraints by now.
> > The power domains can avoid supplying these callbacks, if they don't
> > support setting performance-states.
> >
> > A power domain may have only get_performance_state() callback, if it
> > doesn't have the capability of changing the performance state itself but
> > someone in its parent hierarchy has.
> >
> > A power domain may have only set_performance_state(), if it doesn't have
> > any direct devices below it but subdomains. And so the
> > get_performance_state() will never get called from the core.
> >
>
> It seems like the ->get_perfomance_state() is a device specific
> callback, while the ->set_performance_state() is a genpd domain
> callback.
Yes.
> I am wondering whether we should try to improve the names of the
> callbacks to reflect this.
What about dev_get_performance_state() and
genpd_set_performance_state() ?
> > The more common case would be to have both the callbacks set.
> >
> > Another API, pm_genpd_has_performance_state(), is also added to let
> > other parts of the kernel check if the power domain of a device supports
> > performance-states or not. This could have been done from
>
> I think a better name of this function is:
> dev_pm_genpd_has_performance_state(). What do you think?
Sure.
> We might even want to decide to explicitly stay using the terminology
> "DVFS" instead. In such case, perhaps converting the names of the
> callbacks/API to use "dvfs" instead. For the API added here, maybe
> dev_pm_genpd_can_dvfs().
I am not sure about that really. Because in most of the cases genpd
wouldn't do any freq switching, but only voltage.
> > pm_genpd_update_performance_state() as well, but that routine gets
> > called every time we do DVFS for the device and it wouldn't be optimal
> > in that case.
>
> So pm_genpd_update_performance_state() is also a new API added in
> $subject patch. But there is no information about it in the changelog,
> besides the above. Please add that.
Yeah, I missed that and will include like what I said in the beginning
of the reply on how this leads to call of other callbacks.
> Moreover, perhaps we should rename the function to dev_pm_genpd_set_dvfs()
Not sure as earlier said.
> > Note that, the performance level as returned by
> > ->get_performance_state() for the parent domain of a device is used for
> > all domains in parent hierarchy.
>
> Please clarify a bit on this. What exactly does this mean?
For a hierarchy like this:
PPdomain 0 PPdomain 1
| |
--------------------------
|
Pdomain
|
device
->dev_get_performance_state(dev) would be called for the device and it
will return a single value (X) representing the performance index of
its parent ("Pdomain"). But the direct parent domain may not support
setting of performance index and so we need to propagate the call to
parents of Pdomain. And that would be PPdomain 0 and 1.
Now the paragraph in the commit says that the same performance index
value X will be used for both these PPdomains, as we don't want to
make things more complex to begin with.
> > Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > drivers/base/power/domain.c | 223 ++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/pm_domain.h | 22 +++++
> > 2 files changed, 245 insertions(+)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 71c95ad808d5..d506be9ff1f7 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -466,6 +466,229 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
> > return NOTIFY_DONE;
> > }
> >
> > +/*
> > + * Returns true if anyone in genpd's parent hierarchy has
> > + * set_performance_state() set.
> > + */
> > +static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
> > +{
>
> So this function will be become in-directly called by generic drivers
> that supports DVFS of the genpd for their devices.
>
> I think the data you validate here would be better to be pre-validated
> at pm_genpd_init() and at pm_genpd_add|remove_subdomain() and the
> result stored in a variable in the genpd struct. Especially when a
> subdomain is added, that is a point when you can verify the
> *_performance_state() callbacks, and thus make sure it's a correct
> setup from the topology point of view.
Something like this ?
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4a898e095a1d..182c1911ea9c 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -466,25 +466,6 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
return NOTIFY_DONE;
}
-/*
- * Returns true if anyone in genpd's parent hierarchy has
- * set_performance_state() set.
- */
-static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
-{
- struct gpd_link *link;
-
- if (genpd->set_performance_state)
- return true;
-
- list_for_each_entry(link, &genpd->slave_links, slave_node) {
- if (genpd_has_set_performance_state(link->master))
- return true;
- }
-
- return false;
-}
-
/**
* pm_genpd_has_performance_state - Checks if power domain does performance
* state management.
@@ -507,7 +488,7 @@ bool pm_genpd_has_performance_state(struct device *dev)
/* The parent domain must have set get_performance_state() */
if (!IS_ERR(genpd) && genpd->get_performance_state) {
- if (genpd_has_set_performance_state(genpd))
+ if (genpd->can_set_performance_state)
return true;
/*
@@ -1594,6 +1575,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
if (genpd_status_on(subdomain))
genpd_sd_counter_inc(genpd);
+ subdomain->can_set_performance_state += genpd->can_set_performance_state;
+
out:
genpd_unlock(genpd);
genpd_unlock(subdomain);
@@ -1654,6 +1637,8 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
if (genpd_status_on(subdomain))
genpd_sd_counter_dec(genpd);
+ subdomain->can_set_performance_state -= genpd->can_set_performance_state;
+
ret = 0;
break;
}
@@ -1721,6 +1706,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
genpd->max_off_time_changed = true;
genpd->provider = NULL;
genpd->has_provider = false;
+ genpd->can_set_performance_state = !!genpd->set_performance_state;
genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
genpd->domain.ops.runtime_resume = genpd_runtime_resume;
genpd->domain.ops.prepare = pm_genpd_prepare;
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index bf90177208a2..995d0cb1bc14 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -64,6 +64,7 @@ struct generic_pm_domain {
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 can_set_performance_state; /* Number of parent domains supporting set state */
int (*power_off)(struct generic_pm_domain *domain);
int (*power_on)(struct generic_pm_domain *domain);
int (*get_performance_state)(struct device *dev, unsigned long rate);
> > + struct gpd_link *link;
> > +
> > + if (genpd->set_performance_state)
> > + return true;
> > +
> > + list_for_each_entry(link, &genpd->slave_links, slave_node) {
> > + if (genpd_has_set_performance_state(link->master))
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > +
> > +/**
> > + * pm_genpd_has_performance_state - Checks if power domain does performance
> > + * state management.
> > + *
> > + * @dev: Device whose power domain is getting inquired.
> > + *
> > + * This must be called by the user drivers, before they start calling
> > + * pm_genpd_update_performance_state(), to guarantee that all dependencies are
> > + * met and the device's genpd supports performance states.
> > + *
> > + * It is assumed that the user driver guarantees that the genpd wouldn't be
> > + * detached while this routine is getting called.
> > + *
> > + * Returns "true" if device's genpd supports performance states, "false"
> > + * otherwise.
> > + */
> > +bool pm_genpd_has_performance_state(struct device *dev)
> > +{
> > + struct generic_pm_domain *genpd = genpd_lookup_dev(dev);
> > +
> > + /* The parent domain must have set get_performance_state() */
>
> This comment is wrong. This is not about *parent* domains.
>
> Instead I think it should say: "The genpd must have the
> ->get_performance_state() assigned." ...
I was (wrongly) calling power domain of a device as its parent power
domain and so yeah I agree with your feedback.
> > + if (!IS_ERR(genpd) && genpd->get_performance_state) {
> > + if (genpd_has_set_performance_state(genpd))
> > + return true;
>
> ... while this is about verifying that some of the domains (genpds) in
> domain topology has a ->set_performance_state() callback.
>
> In other words, either the genpd or some of its masters must have a
> ->set_performance_state() callback.
Right.
> That makes me wonder: what will happen if there are more than one
> master having a ->set_perfomance_state() callback assigned? I guess
> that is non-allowed configuration?
This patch supports them at least. Device's domain can have multiple
masters which require this configuration, but the same performance
index will be used for all of them (for simplicity unless we have a
real example to serve).
> > +
> > + /*
> > + * A genpd with ->get_performance_state() callback must also
> > + * allow setting performance state.
> > + */
> > + dev_err(dev, "genpd doesn't support setting performance state\n");
> > + }
> > +
> > + return false;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_genpd_has_performance_state);
> > +
> > +/*
> > + * Re-evaluate performance state of a power domain. Finds the highest requested
> > + * performance state by the devices and subdomains within the power domain and
> > + * then tries to change its performance state. If the power domain doesn't have
> > + * a set_performance_state() callback, then we move the request to its parent
> > + * power domain.
> > + *
> > + * Locking: Access (or update) to device's "pd_data->performance_state" field
> > + * happens only with parent domain's lock held. Subdomains have their
>
> What is a *parent* domain here?
Same crappy wording I have been using. Its about device's genpd.
> In genpd we try to use the terminology of master- and sub-domains.
> Could you re-phrase this to get some clarity on what you try to
> explain from the above?
Yeah, sure.
So do we call a device's power domain as its master domain? I thought
that master is only used in context of sub-domains, right ?
> > + * "genpd->performance_state" protected with their own lock (and they are the
> > + * only user of this field) and their per-parent-domain
> > + * "link->performance_state" field is protected with individual parent power
> > + * domain's lock and is only accessed/updated with that lock held.
> > + */
>
> I recall we discussed this off-list, but I am not sure this was the
> conclusion on how to solve the locking. :-) More comments below.
There was no conclusion there :)
> > +static int genpd_update_performance_state(struct generic_pm_domain *genpd,
> > + int depth)
> > +{
> > + struct generic_pm_domain_data *pd_data;
> > + struct generic_pm_domain *master;
> > + struct pm_domain_data *pdd;
> > + unsigned int state = 0, prev;
> > + struct gpd_link *link;
> > + int ret;
> > +
> > + /* 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;
> > + }
>
> I don't think walking the list of devices for each master domain is
> necessary, unless the aggregated performance state from the subdomain
> was increased.
Actually we can skip both ^^ and below loop in few cases. Here is the
diff:
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 599b731fcffc..55bbfdabab53 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -509,6 +509,9 @@ EXPORT_SYMBOL_GPL(pm_genpd_has_performance_state);
* a set_performance_state() callback, then we move the request to its parent
* power domain.
*
+ * The state parameter is the newly requested performance state of the device or
+ * subdomain for which this routine is called.
+ *
* Locking: Access (or update) to device's "pd_data->performance_state" field
* happens only with parent domain's lock held. Subdomains have their
* "genpd->performance_state" protected with their own lock (and they are the
@@ -517,15 +520,23 @@ EXPORT_SYMBOL_GPL(pm_genpd_has_performance_state);
* domain's lock and is only accessed/updated with that lock held.
*/
static int genpd_update_performance_state(struct generic_pm_domain *genpd,
- int depth)
+ int state, int depth)
{
struct generic_pm_domain_data *pd_data;
struct generic_pm_domain *master;
struct pm_domain_data *pdd;
- unsigned int state = 0, prev;
+ unsigned int prev;
struct gpd_link *link;
int ret;
+ /* 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);
@@ -540,9 +551,7 @@ static int genpd_update_performance_state(struct generic_pm_domain *genpd,
state = link->performance_state;
}
- if (genpd->performance_state == state)
- return 0;
-
+update_state:
if (genpd->set_performance_state) {
ret = genpd->set_performance_state(genpd, state);
if (!ret)
@@ -563,7 +572,7 @@ static int genpd_update_performance_state(struct generic_pm_domain *genpd,
genpd_lock_nested(master, depth + 1);
link->performance_state = state;
- ret = genpd_update_performance_state(master, depth + 1);
+ ret = genpd_update_performance_state(master, state, depth + 1);
if (ret)
link->performance_state = prev;
@@ -587,7 +596,7 @@ static int genpd_update_performance_state(struct generic_pm_domain *genpd,
genpd_lock_nested(master, depth + 1);
link->performance_state = prev;
- if (genpd_update_performance_state(master, depth + 1))
+ if (genpd_update_performance_state(master, prev, depth + 1))
pr_err("%s: Failed to roll back to %d performance state\n",
genpd->name, prev);
genpd_unlock(master);
@@ -659,7 +668,7 @@ int pm_genpd_update_performance_state(struct device *dev, unsigned long rate)
goto unlock;
}
- ret = genpd_update_performance_state(genpd, 0);
+ ret = genpd_update_performance_state(genpd, state, 0);
if (ret)
__dev_update_performance_state(dev, state);
> > +
> > + /* Traverse all subdomains within the domain */
> > + list_for_each_entry(link, &genpd->master_links, master_node) {
> > + if (link->performance_state > state)
> > + state = link->performance_state;
> > + }
> > +
>
> From a locking point of view we always traverse the topology from
> bottom an up. In other words, we walk the genpd's ->slave_links, and
> lock the masters in the order the are defined via the slave_links
> list. The order is important to avoid deadlocks. I don't think you
> should walk the master_links as being done above, especially not
> without using locks.
So we need to look at the performance states of the subdomains of a
master. The way it is done in this patch with help of
link->performance_state, we don't need that locking while traversing
the master_links list. Here is how:
- Master's (genpd) master_links list is only updated under master's
lock, which we have already taken here. So master_links list can't
get updated concurrently.
- The link->performance_state field of a subdomain (or slave) is only
updated from within the master's lock. And we are reading it here
from the same lock.
AFAIU, there shouldn't be any deadlocks or locking issues here. Can
you describe some case that may blow up ?
> > + if (genpd->performance_state == state)
> > + return 0;
> > +
> > + if (genpd->set_performance_state) {
> > + ret = genpd->set_performance_state(genpd, state);
> > + if (!ret)
> > + genpd->performance_state = state;
> > +
> > + return ret;
>
> This looks wrong.
>
> I think you should continue to walk upwards in the domain topology, as
> there may be some other master than needs to get its performance state
> updated.
I can do that.
> > + }
> > +
> > + /*
> > + * Not all domains support updating performance state. Move on to their
> > + * parent domains in that case.
>
> /s/parent/master
>
> > + */
> > + prev = genpd->performance_state;
> > +
> > + list_for_each_entry(link, &genpd->slave_links, slave_node) {
> > + master = link->master;
> > +
> > + genpd_lock_nested(master, depth + 1);
> > +
> > + link->performance_state = state;
> > + ret = genpd_update_performance_state(master, depth + 1);
> > + if (ret)
> > + link->performance_state = prev;
> > +
> > + genpd_unlock(master);
> > +
> > + if (ret)
> > + goto err;
> > + }
> > +
>
> A general comment is that I think you should look more closely in the
> code of genpd_power_off|on(). And also how it calls the
> ->power_on|off() callbacks.
>
> Depending whether you want to update the performance state of the
> master domain before the subdomain or the opposite, you will find one
> of them being suited for this case as well.
Isn't it very much similar to that already ? The only major difference
is link->performance_state and I already explained why is it required
to be done that way to avoid deadlocks.
> > + /*
> > + * The parent domains are updated now, lets get genpd performance_state
> > + * in sync with those.
> > + */
> > + genpd->performance_state = state;
> > + return 0;
> > +
> > +err:
> > + list_for_each_entry_continue_reverse(link, &genpd->slave_links,
> > + slave_node) {
> > + master = link->master;
> > +
> > + genpd_lock_nested(master, depth + 1);
> > + link->performance_state = prev;
> > + if (genpd_update_performance_state(master, depth + 1))
> > + pr_err("%s: Failed to roll back to %d performance state\n",
> > + genpd->name, prev);
> > + genpd_unlock(master);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +static int __dev_update_performance_state(struct device *dev, int state)
>
> Please use the prefix genpd, _genpd_ or __genpd for static functions.
>
> > +{
> > + struct generic_pm_domain_data *gpd_data;
> > + int ret;
> > +
> > + spin_lock_irq(&dev->power.lock);
>
> Actually there is no need to use this lock.
>
> Because you hold the genpd lock here, then the device can't be removed
> from its genpd and thus there is always a valid gpd_data.
I am afraid we still need this lock.
genpd_free_dev_data() is called from genpd_remove_device() without
genpd lock and so it is possible that we reach here after that lock is
dropped from genpd_remove_device() but before genpd_free_dev_data() is
called.
Right ?
> > +
> > + if (!dev->power.subsys_data || !dev->power.subsys_data->domain_data) {
> > + ret = -ENODEV;
> > + goto unlock;
> > + }
> > +
> > + gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
> > +
> > + ret = gpd_data->performance_state;
> > + gpd_data->performance_state = state;
> > +
> > +unlock:
> > + spin_unlock_irq(&dev->power.lock);
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * pm_genpd_update_performance_state - Update performance state of device's
> > + * parent power domain for the target frequency for the device.
> > + *
> > + * @dev: Device for which the performance-state needs to be adjusted.
> > + * @rate: Device's next frequency. 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).
> > + *
> > + * This must be called by the user drivers (as many times as they want) only
> > + * after pm_genpd_has_performance_state() is called (only once) and that
> > + * returned "true".
> > + *
> > + * 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 pm_genpd_update_performance_state(struct device *dev, unsigned long rate)
> > +{
> > + struct generic_pm_domain *genpd = dev_to_genpd(dev);
> > + int ret, state;
> > +
> > + if (IS_ERR(genpd))
> > + return -ENODEV;
> > +
> > + genpd_lock(genpd);
> > +
> > + state = genpd->get_performance_state(dev, rate);
> > + if (state < 0) {
> > + ret = state;
> > + goto unlock;
> > + }
> > +
> > + state = __dev_update_performance_state(dev, state);
> > + if (state < 0) {
> > + ret = state;
> > + goto unlock;
> > + }
> > +
> > + ret = genpd_update_performance_state(genpd, 0);
> > + if (ret)
> > + __dev_update_performance_state(dev, state);
> > +
> > +unlock:
> > + genpd_unlock(genpd);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(pm_genpd_update_performance_state);
> > +
> > /**
> > * genpd_power_off_work_fn - Power off PM domain whose subdomain count is 0.
> > * @work: Work structure used for scheduling the execution of this function.
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index b7803a251044..bf90177208a2 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -63,8 +63,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 */
> > int (*power_off)(struct generic_pm_domain *domain);
> > int (*power_on)(struct generic_pm_domain *domain);
> > + int (*get_performance_state)(struct device *dev, unsigned long rate);
> > + int (*set_performance_state)(struct generic_pm_domain *domain,
> > + unsigned int state);
> > struct gpd_dev_ops dev_ops;
> > s64 max_off_time_ns; /* Maximum allowed "suspended" time. */
> > bool max_off_time_changed;
> > @@ -99,6 +103,9 @@ struct gpd_link {
> > struct list_head master_node;
> > struct generic_pm_domain *slave;
> > struct list_head slave_node;
> > +
> > + /* Sub-domain's per-parent domain performance state */
> > + unsigned int performance_state;
>
> How about aggregate_dvfs_state, and move it to the struct generic_pm_domain.
>
> Because I think you only need to keep track one aggregated state per
> domain and per each subdomain, right?
This is required to solve the deadlock you showed in the previous
version of this patch. As I explained earlier.
> Huh, I hope my comments make some sense.
Sure, and thanks for such a detailed review :)
> It's a bit of tricky code,
> when walking the domain topology and I think there are couple
> optimization we can do while doing that. Hopefully you can understand
> most of my comments at least. :-)
>
> Feel free to ping me on IRC, if further explanations is needed.
I would let you go through this reply and then we can talk tomorrow on
IRC as we don't agree on some of the cases here for sure :)
--
viresh
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-07-19 12:37 ` Viresh Kumar
@ 2017-07-21 8:35 ` Ulf Hansson
2017-07-21 9:05 ` Viresh Kumar
2017-07-28 11:00 ` Viresh Kumar
0 siblings, 2 replies; 17+ messages in thread
From: Ulf Hansson @ 2017-07-21 8:35 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
[...]
>
>> What happens when a power domain gets powered off and then on. Is the
>> performance state restored? Please elaborate a bit on this.
>
> Can this happen while the genpd is still in use? If not then we
> wouldn't have a problem here as the users of it would have revoked
> their constraints by now.
This depends on how drivers are dealing with runtime PM in conjunction
with the new pm_genpd_update_performance_state().
In case you don't want to manage some of this in genpd, then each
driver will have to drop their constraints every time they are about
to runtime suspend its device. And restore them at runtime resume.
To me, that's seems like a bad idea. Then it's better to make genpd
deal with this - somehow.
[...]
>>
>> I think a better name of this function is:
>> dev_pm_genpd_has_performance_state(). What do you think?
>
> Sure.
>
>> We might even want to decide to explicitly stay using the terminology
>> "DVFS" instead. In such case, perhaps converting the names of the
>> callbacks/API to use "dvfs" instead. For the API added here, maybe
>> dev_pm_genpd_can_dvfs().
>
> I am not sure about that really. Because in most of the cases genpd
> wouldn't do any freq switching, but only voltage.
Fair enough, let's stick with "performance" then. However, then please
make sure to not mention DVFS in the changelog/comments as it could be
confusing.
>
>> > Note that, the performance level as returned by
>> > ->get_performance_state() for the parent domain of a device is used for
>> > all domains in parent hierarchy.
>>
>> Please clarify a bit on this. What exactly does this mean?
>
> For a hierarchy like this:
>
> PPdomain 0 PPdomain 1
> | |
> --------------------------
> |
> Pdomain
> |
> device
>
> ->dev_get_performance_state(dev) would be called for the device and it
> will return a single value (X) representing the performance index of
> its parent ("Pdomain"). But the direct parent domain may not support
This is not parent or parent domain, but just "domain" or perhaps "PM
domain" to make it clear.
> setting of performance index and so we need to propagate the call to
> parents of Pdomain. And that would be PPdomain 0 and 1.
Use "master domains" instead.
>
> Now the paragraph in the commit says that the same performance index
> value X will be used for both these PPdomains, as we don't want to
> make things more complex to begin with.
Alright, I get it, thanks!
>
>> > Tested-by: Rajendra Nayak <rnayak@codeaurora.org>
>> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> > ---
>> > drivers/base/power/domain.c | 223 ++++++++++++++++++++++++++++++++++++++++++++
>> > include/linux/pm_domain.h | 22 +++++
>> > 2 files changed, 245 insertions(+)
>> >
>> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> > index 71c95ad808d5..d506be9ff1f7 100644
>> > --- a/drivers/base/power/domain.c
>> > +++ b/drivers/base/power/domain.c
>> > @@ -466,6 +466,229 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
>> > return NOTIFY_DONE;
>> > }
>> >
>> > +/*
>> > + * Returns true if anyone in genpd's parent hierarchy has
>> > + * set_performance_state() set.
>> > + */
>> > +static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
>> > +{
>>
>> So this function will be become in-directly called by generic drivers
>> that supports DVFS of the genpd for their devices.
>>
>> I think the data you validate here would be better to be pre-validated
>> at pm_genpd_init() and at pm_genpd_add|remove_subdomain() and the
>> result stored in a variable in the genpd struct. Especially when a
>> subdomain is added, that is a point when you can verify the
>> *_performance_state() callbacks, and thus make sure it's a correct
>> setup from the topology point of view.
>
> Something like this ?
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 4a898e095a1d..182c1911ea9c 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -466,25 +466,6 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
> return NOTIFY_DONE;
> }
>
> -/*
> - * Returns true if anyone in genpd's parent hierarchy has
> - * set_performance_state() set.
> - */
> -static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
> -{
> - struct gpd_link *link;
> -
> - if (genpd->set_performance_state)
> - return true;
> -
> - list_for_each_entry(link, &genpd->slave_links, slave_node) {
> - if (genpd_has_set_performance_state(link->master))
> - return true;
> - }
> -
> - return false;
> -}
> -
> /**
> * pm_genpd_has_performance_state - Checks if power domain does performance
> * state management.
> @@ -507,7 +488,7 @@ bool pm_genpd_has_performance_state(struct device *dev)
>
> /* The parent domain must have set get_performance_state() */
> if (!IS_ERR(genpd) && genpd->get_performance_state) {
> - if (genpd_has_set_performance_state(genpd))
> + if (genpd->can_set_performance_state)
> return true;
>
> /*
> @@ -1594,6 +1575,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
> if (genpd_status_on(subdomain))
> genpd_sd_counter_inc(genpd);
>
> + subdomain->can_set_performance_state += genpd->can_set_performance_state;
> +
> out:
> genpd_unlock(genpd);
> genpd_unlock(subdomain);
> @@ -1654,6 +1637,8 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
> if (genpd_status_on(subdomain))
> genpd_sd_counter_dec(genpd);
>
> + subdomain->can_set_performance_state -= genpd->can_set_performance_state;
> +
> ret = 0;
> break;
> }
> @@ -1721,6 +1706,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> genpd->max_off_time_changed = true;
> genpd->provider = NULL;
> genpd->has_provider = false;
> + genpd->can_set_performance_state = !!genpd->set_performance_state;
> genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
> genpd->domain.ops.runtime_resume = genpd_runtime_resume;
> genpd->domain.ops.prepare = pm_genpd_prepare;
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index bf90177208a2..995d0cb1bc14 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -64,6 +64,7 @@ struct generic_pm_domain {
> 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 can_set_performance_state; /* Number of parent domains supporting set state */
> int (*power_off)(struct generic_pm_domain *domain);
> int (*power_on)(struct generic_pm_domain *domain);
> int (*get_performance_state)(struct device *dev, unsigned long rate);
>
Yes!
On top of that change, you could also add some validation if the
get/set callbacks is there are any constraints on how they must be
assigned.
[...]
>> That makes me wonder: what will happen if there are more than one
>> master having a ->set_perfomance_state() callback assigned? I guess
>> that is non-allowed configuration?
>
> This patch supports them at least. Device's domain can have multiple
> masters which require this configuration, but the same performance
> index will be used for all of them (for simplicity unless we have a
> real example to serve).
Okay!
[...]
>> What is a *parent* domain here?
>
> Same crappy wording I have been using. Its about device's genpd.
>
>> In genpd we try to use the terminology of master- and sub-domains.
>> Could you re-phrase this to get some clarity on what you try to
>> explain from the above?
>
> Yeah, sure.
>
> So do we call a device's power domain as its master domain? I thought
> that master is only used in context of sub-domains, right ?
Correct. A master domain should be used in context of sub-domain. In
most cases we also use "PM domain" instead of "power domain" is it has
a slightly different meaning.
>>
>> From a locking point of view we always traverse the topology from
>> bottom an up. In other words, we walk the genpd's ->slave_links, and
>> lock the masters in the order the are defined via the slave_links
>> list. The order is important to avoid deadlocks. I don't think you
>> should walk the master_links as being done above, especially not
>> without using locks.
>
> So we need to look at the performance states of the subdomains of a
> master. The way it is done in this patch with help of
> link->performance_state, we don't need that locking while traversing
> the master_links list. Here is how:
>
> - Master's (genpd) master_links list is only updated under master's
> lock, which we have already taken here. So master_links list can't
> get updated concurrently.
>
> - The link->performance_state field of a subdomain (or slave) is only
> updated from within the master's lock. And we are reading it here
> from the same lock.
>
> AFAIU, there shouldn't be any deadlocks or locking issues here. Can
> you describe some case that may blow up ?
My main concern is the order of how you take the looks. We never take
a masters lock before the current domain lock.
And when walking the topology, we use the slave links and locks the
first master from that list. Continues with that tree, then get back
to slave list and pick the next master.
If you change that order, we could end getting deadlocks.
[...]
>> A general comment is that I think you should look more closely in the
>> code of genpd_power_off|on(). And also how it calls the
>> ->power_on|off() callbacks.
>>
>> Depending whether you want to update the performance state of the
>> master domain before the subdomain or the opposite, you will find one
>> of them being suited for this case as well.
>
> Isn't it very much similar to that already ? The only major difference
> is link->performance_state and I already explained why is it required
> to be done that way to avoid deadlocks.
No, because you walk the master lists. Thus getting a different order or locks.
I did some drawing of this, using the slave links, and I don't see any
issues why you can't use that instead.
[...]
>> > +{
>> > + struct generic_pm_domain_data *gpd_data;
>> > + int ret;
>> > +
>> > + spin_lock_irq(&dev->power.lock);
>>
>> Actually there is no need to use this lock.
>>
>> Because you hold the genpd lock here, then the device can't be removed
>> from its genpd and thus there is always a valid gpd_data.
>
> I am afraid we still need this lock.
>
> genpd_free_dev_data() is called from genpd_remove_device() without
> genpd lock and so it is possible that we reach here after that lock is
> dropped from genpd_remove_device() but before genpd_free_dev_data() is
> called.
>
> Right ?
I must had something else in mind, because you are absolutely correct.
Sorry for the noise.
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-07-21 8:35 ` Ulf Hansson
@ 2017-07-21 9:05 ` Viresh Kumar
2017-07-23 7:20 ` Ulf Hansson
2017-07-28 11:00 ` Viresh Kumar
1 sibling, 1 reply; 17+ messages in thread
From: Viresh Kumar @ 2017-07-21 9:05 UTC (permalink / raw)
To: Ulf Hansson
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 21-07-17, 10:35, Ulf Hansson wrote:
> This depends on how drivers are dealing with runtime PM in conjunction
> with the new pm_genpd_update_performance_state().
>
> In case you don't want to manage some of this in genpd, then each
> driver will have to drop their constraints every time they are about
> to runtime suspend its device. And restore them at runtime resume.
>
> To me, that's seems like a bad idea. Then it's better to make genpd
> deal with this - somehow.
Right. So we should call the ->set_performance_state() from off/on as
well. Will do that.
> Yes!
>
> On top of that change, you could also add some validation if the
> get/set callbacks is there are any constraints on how they must be
> assigned.
I am not sure if I understood that, sorry. What other constraints are
you talking about ?
> >> From a locking point of view we always traverse the topology from
> >> bottom an up. In other words, we walk the genpd's ->slave_links, and
> >> lock the masters in the order the are defined via the slave_links
> >> list. The order is important to avoid deadlocks. I don't think you
> >> should walk the master_links as being done above, especially not
> >> without using locks.
> >
> > So we need to look at the performance states of the subdomains of a
> > master. The way it is done in this patch with help of
> > link->performance_state, we don't need that locking while traversing
> > the master_links list. Here is how:
> >
> > - Master's (genpd) master_links list is only updated under master's
> > lock, which we have already taken here. So master_links list can't
> > get updated concurrently.
> >
> > - The link->performance_state field of a subdomain (or slave) is only
> > updated from within the master's lock. And we are reading it here
> > from the same lock.
> >
> > AFAIU, there shouldn't be any deadlocks or locking issues here. Can
> > you describe some case that may blow up ?
>
> My main concern is the order of how you take the looks. We never take
> a masters lock before the current domain lock.
Right and this patch doesn't break that.
> And when walking the topology, we use the slave links and locks the
> first master from that list. Continues with that tree, then get back
> to slave list and pick the next master.
Again, that's how this patch does it.
> If you change that order, we could end getting deadlocks.
And because that order isn't changed at all, we shouldn't have
deadlocks.
> >> A general comment is that I think you should look more closely in the
> >> code of genpd_power_off|on(). And also how it calls the
> >> ->power_on|off() callbacks.
> >>
> >> Depending whether you want to update the performance state of the
> >> master domain before the subdomain or the opposite, you will find one
> >> of them being suited for this case as well.
> >
> > Isn't it very much similar to that already ? The only major difference
> > is link->performance_state and I already explained why is it required
> > to be done that way to avoid deadlocks.
>
> No, because you walk the master lists. Thus getting a different order or locks.
>
> I did some drawing of this, using the slave links, and I don't see any
> issues why you can't use that instead.
Damn, I am confused on which part are you talking about. Let me paste
the code here once again and clarify how this is supposed to work just fine :)
>> +static int genpd_update_performance_state(struct generic_pm_domain *genpd,
>> + int depth)
>> +{
genpd is already locked.
>> + struct generic_pm_domain_data *pd_data;
>> + struct generic_pm_domain *master;
>> + struct pm_domain_data *pdd;
>> + unsigned int state = 0, prev;
>> + struct gpd_link *link;
>> + int ret;
>> +
>> + /* 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;
>> + }
>> +
>> + /* Traverse all subdomains within the domain */
>> + list_for_each_entry(link, &genpd->master_links, master_node) {
This is the only place where we look at all the sub-domains, but we
don't need locking here at all as link->performance_state is only
accessed while "genpd" is locked. It doesn't need sub-domain's lock.
>> + if (link->performance_state > state)
>> + state = link->performance_state;
>> + }
>> +
>> + if (genpd->performance_state == state)
>> + return 0;
>> +
>> + if (genpd->set_performance_state) {
>> + ret = genpd->set_performance_state(genpd, state);
>> + if (!ret)
>> + genpd->performance_state = state;
>> +
>> + return ret;
>> + }
>> +
>> + /*
>> + * Not all domains support updating performance state. Move on to their
>> + * parent domains in that case.
>> + */
>> + prev = genpd->performance_state;
>> +
The below part is what I assumed you were commenting on and this is
very similar to how on/off are implemented today.
>> + list_for_each_entry(link, &genpd->slave_links, slave_node) {
i.e. we traverse the list of masters from slave_links list.
>> + master = link->master;
>> +
>> + genpd_lock_nested(master, depth + 1);
Take master's lock (and "genpd" is already locked when this function
is called.)
>> +
>> + link->performance_state = state;
>> + ret = genpd_update_performance_state(master, depth + 1);
Do recursive calling and so the master tree will finish first before
moving to next master.
>> + if (ret)
>> + link->performance_state = prev;
>> +
The above can actually be done outside of this lock as we are only
concerned about "genpd" lock here.
Where do you think the order of locking is screwed up ?
>> + genpd_unlock(master);
>> +
>> + if (ret)
>> + goto err;
>> + }
>> +
>> + /*
>> + * The parent domains are updated now, lets get genpd performance_state
>> + * in sync with those.
>> + */
>> + genpd->performance_state = state;
>> + return 0;
>> +
>> +err:
>> + list_for_each_entry_continue_reverse(link, &genpd->slave_links,
>> + slave_node) {
>> + master = link->master;
>> +
>> + genpd_lock_nested(master, depth + 1);
>> + link->performance_state = prev;
>> + if (genpd_update_performance_state(master, depth + 1))
>> + pr_err("%s: Failed to roll back to %d performance state\n",
>> + genpd->name, prev);
>> + genpd_unlock(master);
>> + }
>> +
>> + return ret;
>> +}
>> +
--
viresh
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-07-21 9:05 ` Viresh Kumar
@ 2017-07-23 7:20 ` Ulf Hansson
2017-07-24 10:32 ` Viresh Kumar
0 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2017-07-23 7:20 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 21 July 2017 at 11:05, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 21-07-17, 10:35, Ulf Hansson wrote:
>> This depends on how drivers are dealing with runtime PM in conjunction
>> with the new pm_genpd_update_performance_state().
>>
>> In case you don't want to manage some of this in genpd, then each
>> driver will have to drop their constraints every time they are about
>> to runtime suspend its device. And restore them at runtime resume.
>>
>> To me, that's seems like a bad idea. Then it's better to make genpd
>> deal with this - somehow.
>
> Right. So we should call the ->set_performance_state() from off/on as
> well. Will do that.
>
>> Yes!
>>
>> On top of that change, you could also add some validation if the
>> get/set callbacks is there are any constraints on how they must be
>> assigned.
>
> I am not sure if I understood that, sorry. What other constraints are
> you talking about ?
Just thinking that if a genpd is about to be added as a subdomain, and
it has ->get_performance_state(), but not ->set_performance_state(),
perhaps we should require its master to have
->set_performance_state().
Anyway, I let you do the thinking of what is and what is not needed here.
[...]
>>
>> My main concern is the order of how you take the looks. We never take
>> a masters lock before the current domain lock.
>
> Right and this patch doesn't break that.
>
>> And when walking the topology, we use the slave links and locks the
>> first master from that list. Continues with that tree, then get back
>> to slave list and pick the next master.
>
> Again, that's how this patch does it.
>
>> If you change that order, we could end getting deadlocks.
>
> And because that order isn't changed at all, we shouldn't have
> deadlocks.
True. Trying to clarify more below...
>
>> >> A general comment is that I think you should look more closely in the
>> >> code of genpd_power_off|on(). And also how it calls the
>> >> ->power_on|off() callbacks.
>> >>
>> >> Depending whether you want to update the performance state of the
>> >> master domain before the subdomain or the opposite, you will find one
>> >> of them being suited for this case as well.
>> >
>> > Isn't it very much similar to that already ? The only major difference
>> > is link->performance_state and I already explained why is it required
>> > to be done that way to avoid deadlocks.
>>
>> No, because you walk the master lists. Thus getting a different order or locks.
>>
>> I did some drawing of this, using the slave links, and I don't see any
>> issues why you can't use that instead.
>
> Damn, I am confused on which part are you talking about. Let me paste
> the code here once again and clarify how this is supposed to work just fine :)
I should have been more clear. Walking the master list, then checking
each link without using locks - why is that safe?
Then even if you think it's safe, then please explain in detail why its needed.
Walking the slave list as being done for power off/on should work
perfectly okay for your case as well. No?
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-07-23 7:20 ` Ulf Hansson
@ 2017-07-24 10:32 ` Viresh Kumar
0 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2017-07-24 10:32 UTC (permalink / raw)
To: Ulf Hansson
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 23-07-17, 09:20, Ulf Hansson wrote:
> I should have been more clear. Walking the master list, then checking
> each link without using locks - why is that safe?
>
> Then even if you think it's safe, then please explain in detail why its needed.
>
> Walking the slave list as being done for power off/on should work
> perfectly okay for your case as well. No?
I got it. I will try to explain why it is done this way with the help
of two versions of genpd_update_performance_state() routine. The first
one is from a previous version and second one is from the current
series.
Just as a note, the problem is not with traversing slave_links list
but the master_links list.
1.) Previous Version (Have deadlock issues, as you reported then).
>> +static int genpd_update_performance_state(struct generic_pm_domain *genpd,
>> + int depth)
>> +{
The genpd is already locked here..
>> + struct generic_pm_domain_data *pd_data;
>> + struct generic_pm_domain *subdomain;
>> + struct pm_domain_data *pdd;
>> + unsigned int state = 0, prev;
>> + struct gpd_link *link;
>> + int ret;
>> +
>> + /* 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;
>> + }
Above is fine as we traversed list of devices that are powered by the
PM domain. No additional locks are required.
>> + /* Traverse all subdomains within the domain */
>> + list_for_each_entry(link, &genpd->master_links, master_node) {
>> + subdomain = link->slave;
>> +
>> + if (subdomain->performance_state > state)
>> + state = subdomain->performance_state;
>> + }
But this is not fine at all. subdomain->performance_state might get
updated from another thread for another genpd. And so we need locking
here, but we can't do that as we need to take locks starting from
slaves to masters. This is what you correctly pointed out in earlier
versions.
>> + if (genpd->performance_state == state)
>> + return 0;
>> +
>> + /*
>> + * Not all domains support updating performance state. Move on to their
>> + * parent domains in that case.
>> + */
>> + if (genpd->set_performance_state) {
>> + ret = genpd->set_performance_state(genpd, state);
>> + if (!ret)
>> + genpd->performance_state = state;
>> +
>> + return ret;
>> + }
>> +
>> + prev = genpd->performance_state;
>> + genpd->performance_state = state;
This is racy as well (because of the earlier traversal of
master-list), as genpd->performance_state might be getting read for
one of its masters currently (from another instance of this same
routine).
>> +
>> + list_for_each_entry(link, &genpd->slave_links, slave_node) {
>> + struct generic_pm_domain *master = link->master;
>> +
>> + genpd_lock_nested(master, depth + 1);
>> + ret = genpd_update_performance_state(master, depth + 1);
>> + genpd_unlock(master);
>>
>> ... Handle errors here.
>> + }
>> +
>> + return 0;
>> +}
So the conclusion was that we surely can't lock the subdomains while
running genpd_update_performance_state() for a master genpd.
And that's what the below latest code tried to address.
2.) New code, which shouldn't have any of those deadlock issues.
>> +static int genpd_update_performance_state(struct generic_pm_domain *genpd,
genpd is still locked from its caller.
>> + int depth)
>> +{
>> + struct generic_pm_domain_data *pd_data;
>> + struct generic_pm_domain *master;
>> + struct pm_domain_data *pdd;
>> + unsigned int state = 0, prev;
>> + struct gpd_link *link;
>> + int ret;
>> +
>> + /* 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;
>> + }
>> +
Above remains the same and shouldn't have any issues.
>> + /* Traverse all subdomains within the domain */
>> + list_for_each_entry(link, &genpd->master_links, master_node) {
>> + if (link->performance_state > state)
>> + state = link->performance_state;
>> + }
>> +
Instead of a single performance_state field for the entire subdomain
structure, we store a performance_state value for each
master <-> subdomain pair. And this field is protected by the master
lock, always.
Since the genpd was already locked, the link->performance_state field
of all its subdomains can be accessed without further locking.
>> + if (genpd->performance_state == state)
>> + return 0;
>> +
>> + if (genpd->set_performance_state) {
>> + ret = genpd->set_performance_state(genpd, state);
>> + if (!ret)
>> + genpd->performance_state = state;
>> +
>> + return ret;
>> + }
>> +
>> + /*
>> + * Not all domains support updating performance state. Move on to their
>> + * parent domains in that case.
>> + */
>> + prev = genpd->performance_state;
>> +
Lets look at the below one now.
>> + list_for_each_entry(link, &genpd->slave_links, slave_node) {
>> + master = link->master;
>> +
>> + genpd_lock_nested(master, depth + 1);
>> +
>> + link->performance_state = state;
(I incorrectly mentioned in my last reply to you that this can be
present outside of the lock, this can't be.)
So here we have taken the master's lock and updated the link shared
between master <-> subdomain (genpd here).
>> + ret = genpd_update_performance_state(master, depth + 1);
So when this recursive call is made for the "master" domain, it will
read link->performance_state of all its subdomains (from the earlier
for-loop) and the current "genpd" domain will be part of that
master-list. And that's why we updated link->performance_state before
calling the genpd_update_performance_state() routine above, so that
its new value can be available.
IOW, we can't have a race now where link->performance_state for any of
the subdomains of genpd is read/updated in parallel.
>> + if (ret)
>> + link->performance_state = prev;
>> +
>> + genpd_unlock(master);
>> +
>> + if (ret)
>> + goto err;
>> + }
>> +
>> + /*
>> + * The parent domains are updated now, lets get genpd performance_state
>> + * in sync with those.
>> + */
And once we are done with all the master domains, we can update the
genpd->performance_state safely.
>> + genpd->performance_state = state;
>> + return 0;
>> +
>> +err:
>> ...
>> +}
>> +
Was I able to clarify your doubts ?
--
viresh
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-07-21 8:35 ` Ulf Hansson
2017-07-21 9:05 ` Viresh Kumar
@ 2017-07-28 11:00 ` Viresh Kumar
2017-07-29 8:24 ` Ulf Hansson
1 sibling, 1 reply; 17+ messages in thread
From: Viresh Kumar @ 2017-07-28 11:00 UTC (permalink / raw)
To: Ulf Hansson
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 21-07-17, 10:35, Ulf Hansson wrote:
> >> > +/*
> >> > + * Returns true if anyone in genpd's parent hierarchy has
> >> > + * set_performance_state() set.
> >> > + */
> >> > +static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
> >> > +{
> >>
> >> So this function will be become in-directly called by generic drivers
> >> that supports DVFS of the genpd for their devices.
> >>
> >> I think the data you validate here would be better to be pre-validated
> >> at pm_genpd_init() and at pm_genpd_add|remove_subdomain() and the
> >> result stored in a variable in the genpd struct. Especially when a
> >> subdomain is added, that is a point when you can verify the
> >> *_performance_state() callbacks, and thus make sure it's a correct
> >> setup from the topology point of view.
Looks like I have to keep this routine as is and your solution may not
work well. :(
> > Something like this ?
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 4a898e095a1d..182c1911ea9c 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -466,25 +466,6 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
> > return NOTIFY_DONE;
> > }
> >
> > -/*
> > - * Returns true if anyone in genpd's parent hierarchy has
> > - * set_performance_state() set.
> > - */
> > -static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
> > -{
> > - struct gpd_link *link;
> > -
> > - if (genpd->set_performance_state)
> > - return true;
> > -
> > - list_for_each_entry(link, &genpd->slave_links, slave_node) {
> > - if (genpd_has_set_performance_state(link->master))
> > - return true;
> > - }
> > -
> > - return false;
> > -}
> > -
> > /**
> > * pm_genpd_has_performance_state - Checks if power domain does performance
> > * state management.
> > @@ -507,7 +488,7 @@ bool pm_genpd_has_performance_state(struct device *dev)
> >
> > /* The parent domain must have set get_performance_state() */
> > if (!IS_ERR(genpd) && genpd->get_performance_state) {
> > - if (genpd_has_set_performance_state(genpd))
> > + if (genpd->can_set_performance_state)
> > return true;
> >
> > /*
> > @@ -1594,6 +1575,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
> > if (genpd_status_on(subdomain))
> > genpd_sd_counter_inc(genpd);
> >
> > + subdomain->can_set_performance_state += genpd->can_set_performance_state;
> > +
> > out:
> > genpd_unlock(genpd);
> > genpd_unlock(subdomain);
> > @@ -1654,6 +1637,8 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
> > if (genpd_status_on(subdomain))
> > genpd_sd_counter_dec(genpd);
> >
> > + subdomain->can_set_performance_state -= genpd->can_set_performance_state;
> > +
> > ret = 0;
> > break;
> > }
> > @@ -1721,6 +1706,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> > genpd->max_off_time_changed = true;
> > genpd->provider = NULL;
> > genpd->has_provider = false;
> > + genpd->can_set_performance_state = !!genpd->set_performance_state;
> > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
> > genpd->domain.ops.runtime_resume = genpd_runtime_resume;
> > genpd->domain.ops.prepare = pm_genpd_prepare;
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index bf90177208a2..995d0cb1bc14 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -64,6 +64,7 @@ struct generic_pm_domain {
> > 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 can_set_performance_state; /* Number of parent domains supporting set state */
> > int (*power_off)(struct generic_pm_domain *domain);
> > int (*power_on)(struct generic_pm_domain *domain);
> > int (*get_performance_state)(struct device *dev, unsigned long rate);
> >
>
> Yes!
The above diff will work fine only for the case where the master
domain has all its masters set properly before genpd_add_subdomain()
is called for the subdomain, as the genpd->can_set_performance_state
count wouldn't change after that. But if the masters of the
master are linked to the master after genpd_add_subdomain() is called
for the subdomain, then we wouldn't be update the
subdomain->can_set_performance_state field later.
For example, consider this scenario:
Domain A (has set_performance_state())
Domain B Domain C (both don't have set_performance_state())
Domain D Domain E (both don't have set_performance_state(), but have get_performance_state())
and here is the call sequence:
genpd_add_subdomain(B, D); can_set_performance_state of B and D = 0;
genpd_add_subdomain(C, E); ... C and E = 0;
genpd_add_subdomain(A, B); ... A = 1, B = 1;
genpd_add_subdomain(A, C); ... A = 1, C = 1;
While the count is set properly for A, B and C, it isn't propagated to
C and E. :(
Though everything would have worked fine if we had this sequence:
genpd_add_subdomain(A, B); ... A = 1, B = 1;
genpd_add_subdomain(A, C); ... A = 1, C = 1;
genpd_add_subdomain(B, D); ... D = 1 ;
genpd_add_subdomain(C, E); ... E = 1;
How to fix it? I tried solving that by propagating the count to all
the subdomains of the subdomain getting added here. But that requires
locking and we can't do that in the reverse direction :(
Anyway, genpd_has_set_performance_state() is supposed to be called
only ONCE by the drivers and so its fine if we have to traverse the
list of subdomains there.
I will keep the original code unless you suggest a good way of getting
around that.
--
viresh
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-07-28 11:00 ` Viresh Kumar
@ 2017-07-29 8:24 ` Ulf Hansson
2017-07-31 4:14 ` Viresh Kumar
0 siblings, 1 reply; 17+ messages in thread
From: Ulf Hansson @ 2017-07-29 8:24 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 28 July 2017 at 13:00, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 21-07-17, 10:35, Ulf Hansson wrote:
>> >> > +/*
>> >> > + * Returns true if anyone in genpd's parent hierarchy has
>> >> > + * set_performance_state() set.
>> >> > + */
>> >> > +static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
>> >> > +{
>> >>
>> >> So this function will be become in-directly called by generic drivers
>> >> that supports DVFS of the genpd for their devices.
>> >>
>> >> I think the data you validate here would be better to be pre-validated
>> >> at pm_genpd_init() and at pm_genpd_add|remove_subdomain() and the
>> >> result stored in a variable in the genpd struct. Especially when a
>> >> subdomain is added, that is a point when you can verify the
>> >> *_performance_state() callbacks, and thus make sure it's a correct
>> >> setup from the topology point of view.
>
> Looks like I have to keep this routine as is and your solution may not
> work well. :(
>
>> > Something like this ?
>> >
>> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> > index 4a898e095a1d..182c1911ea9c 100644
>> > --- a/drivers/base/power/domain.c
>> > +++ b/drivers/base/power/domain.c
>> > @@ -466,25 +466,6 @@ static int genpd_dev_pm_qos_notifier(struct notifier_block *nb,
>> > return NOTIFY_DONE;
>> > }
>> >
>> > -/*
>> > - * Returns true if anyone in genpd's parent hierarchy has
>> > - * set_performance_state() set.
>> > - */
>> > -static bool genpd_has_set_performance_state(struct generic_pm_domain *genpd)
>> > -{
>> > - struct gpd_link *link;
>> > -
>> > - if (genpd->set_performance_state)
>> > - return true;
>> > -
>> > - list_for_each_entry(link, &genpd->slave_links, slave_node) {
>> > - if (genpd_has_set_performance_state(link->master))
>> > - return true;
>> > - }
>> > -
>> > - return false;
>> > -}
>> > -
>> > /**
>> > * pm_genpd_has_performance_state - Checks if power domain does performance
>> > * state management.
>> > @@ -507,7 +488,7 @@ bool pm_genpd_has_performance_state(struct device *dev)
>> >
>> > /* The parent domain must have set get_performance_state() */
>> > if (!IS_ERR(genpd) && genpd->get_performance_state) {
>> > - if (genpd_has_set_performance_state(genpd))
>> > + if (genpd->can_set_performance_state)
>> > return true;
>> >
>> > /*
>> > @@ -1594,6 +1575,8 @@ static int genpd_add_subdomain(struct generic_pm_domain *genpd,
>> > if (genpd_status_on(subdomain))
>> > genpd_sd_counter_inc(genpd);
>> >
>> > + subdomain->can_set_performance_state += genpd->can_set_performance_state;
>> > +
>> > out:
>> > genpd_unlock(genpd);
>> > genpd_unlock(subdomain);
>> > @@ -1654,6 +1637,8 @@ int pm_genpd_remove_subdomain(struct generic_pm_domain *genpd,
>> > if (genpd_status_on(subdomain))
>> > genpd_sd_counter_dec(genpd);
>> >
>> > + subdomain->can_set_performance_state -= genpd->can_set_performance_state;
>> > +
>> > ret = 0;
>> > break;
>> > }
>> > @@ -1721,6 +1706,7 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>> > genpd->max_off_time_changed = true;
>> > genpd->provider = NULL;
>> > genpd->has_provider = false;
>> > + genpd->can_set_performance_state = !!genpd->set_performance_state;
>> > genpd->domain.ops.runtime_suspend = genpd_runtime_suspend;
>> > genpd->domain.ops.runtime_resume = genpd_runtime_resume;
>> > genpd->domain.ops.prepare = pm_genpd_prepare;
>> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
>> > index bf90177208a2..995d0cb1bc14 100644
>> > --- a/include/linux/pm_domain.h
>> > +++ b/include/linux/pm_domain.h
>> > @@ -64,6 +64,7 @@ struct generic_pm_domain {
>> > 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 can_set_performance_state; /* Number of parent domains supporting set state */
>> > int (*power_off)(struct generic_pm_domain *domain);
>> > int (*power_on)(struct generic_pm_domain *domain);
>> > int (*get_performance_state)(struct device *dev, unsigned long rate);
>> >
>>
>> Yes!
>
> The above diff will work fine only for the case where the master
> domain has all its masters set properly before genpd_add_subdomain()
> is called for the subdomain, as the genpd->can_set_performance_state
> count wouldn't change after that. But if the masters of the
> master are linked to the master after genpd_add_subdomain() is called
> for the subdomain, then we wouldn't be update the
> subdomain->can_set_performance_state field later.
>
> For example, consider this scenario:
>
> Domain A (has set_performance_state())
>
> Domain B Domain C (both don't have set_performance_state())
>
> Domain D Domain E (both don't have set_performance_state(), but have get_performance_state())
>
>
> and here is the call sequence:
>
> genpd_add_subdomain(B, D); can_set_performance_state of B and D = 0;
> genpd_add_subdomain(C, E); ... C and E = 0;
> genpd_add_subdomain(A, B); ... A = 1, B = 1;
> genpd_add_subdomain(A, C); ... A = 1, C = 1;
>
> While the count is set properly for A, B and C, it isn't propagated to
> C and E. :(
>
> Though everything would have worked fine if we had this sequence:
>
> genpd_add_subdomain(A, B); ... A = 1, B = 1;
> genpd_add_subdomain(A, C); ... A = 1, C = 1;
> genpd_add_subdomain(B, D); ... D = 1 ;
> genpd_add_subdomain(C, E); ... E = 1;
>
> How to fix it? I tried solving that by propagating the count to all
> the subdomains of the subdomain getting added here. But that requires
> locking and we can't do that in the reverse direction :(
Yeah, you are right!
>
> Anyway, genpd_has_set_performance_state() is supposed to be called
> only ONCE by the drivers and so its fine if we have to traverse the
> list of subdomains there.
>
> I will keep the original code unless you suggest a good way of getting
> around that.
Let's invent a new genpd flag, GENPD_FLAG_PERF_STATE!
The creator of the genpd then needs to set this before calling
pm_genpd_init(). Similar as we are dealing with GENPD_FLAG_PM_CLK.
The requirement for GENPD_FLAG_PERF_STATES, is to have the
->get_performance_state() assigned. This shall be verified during
pm_genpd_init().
The pm_genpd_has_performance_state() then only need to return true, in
cases the device's genpd has GENPD_FLAG_PERF_STATE set - else false.
Regarding ->set_performance_state(), let's just make it optional - and
when trying to set a new performance state, just walk the genpd
hierarchy, from bottom to up, then invoke the callback when it's
assigned.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-07-29 8:24 ` Ulf Hansson
@ 2017-07-31 4:14 ` Viresh Kumar
2017-08-02 8:21 ` Viresh Kumar
0 siblings, 1 reply; 17+ messages in thread
From: Viresh Kumar @ 2017-07-31 4:14 UTC (permalink / raw)
To: Ulf Hansson
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 29-07-17, 10:24, Ulf Hansson wrote:
> Let's invent a new genpd flag, GENPD_FLAG_PERF_STATE!
>
> The creator of the genpd then needs to set this before calling
> pm_genpd_init(). Similar as we are dealing with GENPD_FLAG_PM_CLK.
>
> The requirement for GENPD_FLAG_PERF_STATES, is to have the
> ->get_performance_state() assigned. This shall be verified during
> pm_genpd_init().
>
> The pm_genpd_has_performance_state() then only need to return true, in
> cases the device's genpd has GENPD_FLAG_PERF_STATE set - else false.
>
> Regarding ->set_performance_state(), let's just make it optional - and
> when trying to set a new performance state, just walk the genpd
> hierarchy, from bottom to up, then invoke the callback when it's
> assigned.
Sounds good.
--
viresh
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains
2017-07-31 4:14 ` Viresh Kumar
@ 2017-08-02 8:21 ` Viresh Kumar
0 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2017-08-02 8:21 UTC (permalink / raw)
To: Ulf Hansson
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 31-07-17, 09:44, Viresh Kumar wrote:
> On 29-07-17, 10:24, Ulf Hansson wrote:
> > Let's invent a new genpd flag, GENPD_FLAG_PERF_STATE!
> >
> > The creator of the genpd then needs to set this before calling
> > pm_genpd_init(). Similar as we are dealing with GENPD_FLAG_PM_CLK.
> >
> > The requirement for GENPD_FLAG_PERF_STATES, is to have the
> > ->get_performance_state() assigned. This shall be verified during
> > pm_genpd_init().
> >
> > The pm_genpd_has_performance_state() then only need to return true, in
> > cases the device's genpd has GENPD_FLAG_PERF_STATE set - else false.
> >
> > Regarding ->set_performance_state(), let's just make it optional - and
> > when trying to set a new performance state, just walk the genpd
> > hierarchy, from bottom to up, then invoke the callback when it's
> > assigned.
>
> Sounds good.
Actually, I don't think we need this flag at all. The presence of the
get_performance_state() callback itself can be used as a flag here instead of
defining a new one.
As with both, your above solution and my solution, we pretty much don't check
presence of set_performance_state() callbacks in the master hierarchy. If its
present, we call it, else nothing happens.
--
viresh
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH V8 2/6] PM / OPP: Support updating performance state of device's power domains
2017-06-21 7:10 [PATCH V8 0/6] PM / Domains: Power domain performance states Viresh Kumar
2017-06-21 7:10 ` [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains Viresh Kumar
@ 2017-06-21 7:10 ` Viresh Kumar
2017-06-21 7:10 ` [NOT-FOR-MERGE V8 3/6] soc: qcom: rpmpd: Add a powerdomain driver to model cx/mx powerdomains Viresh Kumar
` (3 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2017-06-21 7:10 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
The genpd framework now provides an API to request device's power
domain to update its performance state based on a particular target
frequency for the device.
Use that interface from the OPP core for devices whose power domains
support performance states.
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/base/power/opp/core.c | 48 ++++++++++++++++++++++++++++++++++++++++++-
drivers/base/power/opp/opp.h | 2 ++
2 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c
index a8cc14fd8ae4..ef623afcc5fd 100644
--- a/drivers/base/power/opp/core.c
+++ b/drivers/base/power/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,42 @@ _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)
+{
+ int ret;
+
+ /* Scaling up? Scale domain performance state before frequency */
+ if (freq > old_freq) {
+ ret = pm_genpd_update_performance_state(dev, freq);
+ 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 = pm_genpd_update_performance_state(dev, freq);
+ 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:
+ pm_genpd_update_performance_state(dev, old_freq);
+
+ return ret;
+}
+
static int _generic_set_opp_regulator(const struct opp_table *opp_table,
struct device *dev,
unsigned long old_freq,
@@ -653,7 +690,14 @@ 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);
+ 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,
@@ -755,6 +799,8 @@ static struct opp_table *_allocate_opp_table(struct device *dev)
ret);
}
+ opp_table->genpd_performance_state = pm_genpd_has_performance_state(dev);
+
BLOCKING_INIT_NOTIFIER_HEAD(&opp_table->head);
INIT_LIST_HEAD(&opp_table->opp_list);
mutex_init(&opp_table->lock);
diff --git a/drivers/base/power/opp/opp.h b/drivers/base/power/opp/opp.h
index 166eef990599..1efa253e1934 100644
--- a/drivers/base/power/opp/opp.h
+++ b/drivers/base/power/opp/opp.h
@@ -135,6 +135,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 +171,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.13.0.71.gd7076ec9c9cb
^ permalink raw reply related [flat|nested] 17+ messages in thread* [NOT-FOR-MERGE V8 3/6] soc: qcom: rpmpd: Add a powerdomain driver to model cx/mx powerdomains
2017-06-21 7:10 [PATCH V8 0/6] PM / Domains: Power domain performance states Viresh Kumar
2017-06-21 7:10 ` [PATCH V8 1/6] PM / Domains: Add support to select performance-state of domains Viresh Kumar
2017-06-21 7:10 ` [PATCH V8 2/6] PM / OPP: Support updating performance state of device's power domains Viresh Kumar
@ 2017-06-21 7:10 ` Viresh Kumar
2017-06-21 7:10 ` [NOT-FOR-MERGE V8 4/6] soc: qcom: rpmpd: Add support for get/set performance state Viresh Kumar
` (2 subsequent siblings)
5 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2017-06-21 7:10 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
From: Rajendra Nayak <rnayak@codeaurora.org>
THIS IS TEST CODE, SHOULDN'T BE MERGED.
The cx/mx powerdomains 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 powerdomains on msm8996.
NOT-signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
NOT-signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
.../devicetree/bindings/power/qcom,rpmpd.txt | 10 +
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, 332 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..8b48ce57a563
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/qcom,rpmpd.txt
@@ -0,0 +1,10 @@
+Qualcomm RPM Powerdomains
+
+* For RPM powerdomains, 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 9bc9c857a000..1fec06285f38 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -412,6 +412,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 9fca977ef18d..3892148be3c3 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -24,6 +24,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 Powerdomain driver"
+ depends on MFD_QCOM_RPM && QCOM_SMD_RPM
+ help
+ QCOM RPM powerdomain driver to support powerdomain 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 414f0de274fa..f7c3edfa6de8 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -7,3 +7,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..d34d9c363815
--- /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/module.h>
+#include <linux/mutex.h>
+#include <linux/pm_domain.h>
+#include <linux/mfd/qcom_rpm.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.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.13.0.71.gd7076ec9c9cb
^ permalink raw reply related [flat|nested] 17+ messages in thread* [NOT-FOR-MERGE V8 4/6] soc: qcom: rpmpd: Add support for get/set performance state
2017-06-21 7:10 [PATCH V8 0/6] PM / Domains: Power domain performance states Viresh Kumar
` (2 preceding siblings ...)
2017-06-21 7:10 ` [NOT-FOR-MERGE V8 3/6] soc: qcom: rpmpd: Add a powerdomain driver to model cx/mx powerdomains Viresh Kumar
@ 2017-06-21 7:10 ` Viresh Kumar
2017-06-21 7:10 ` [NOT-FOR-MERGE V8 5/6] mmc: sdhci-msm: Adapt the driver to use OPPs to set clocks/performance state Viresh Kumar
2017-06-21 7:10 ` [NOT-FOR-MERGE V8 6/6] remoteproc: qcom: q6v5: Vote for proxy powerdomain performance state Viresh Kumar
5 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2017-06-21 7:10 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
From: Rajendra Nayak <rnayak@codeaurora.org>
THIS IS TEST CODE, SHOULDN'T BE MERGED.
With genpd now expecting powerdomain drivers supporting performance
state to support get/set performance state callbacks, add support for it
in the rpmpd driver.
NOT-signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
NOT-signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/soc/qcom/rpmpd.c | 100 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 100 insertions(+)
diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
index d34d9c363815..7ef81429c5c5 100644
--- a/drivers/soc/qcom/rpmpd.c
+++ b/drivers/soc/qcom/rpmpd.c
@@ -101,6 +101,20 @@ struct rpmpd_desc {
size_t num_pds;
};
+enum rpmpd_levels {
+ NONE,
+ LOWER, /* SVS2 */
+ LOW, /* SVS */
+ NOMINAL, /* NOMINAL */
+ HIGH, /* Turbo */
+ MAX_LEVEL,
+};
+
+struct rpmpd_freq_map {
+ struct rpmpd *pd;
+ unsigned long freq[MAX_LEVEL];
+};
+
static DEFINE_MUTEX(rpmpd_lock);
/* msm8996 RPM powerdomains */
@@ -126,6 +140,47 @@ static const struct rpmpd_desc msm8996_desc = {
.num_pds = ARRAY_SIZE(msm8996_rpmpds),
};
+enum msm8996_devices {
+ SDHCI,
+ UFS,
+ PCIE,
+ USB3,
+};
+
+static struct rpmpd_freq_map msm8996_rpmpd_freq_map[] = {
+ [SDHCI] = {
+ .pd = &msm8996_vddcx,
+ .freq[LOWER] = 19200000,
+ .freq[LOW] = 200000000,
+ .freq[NOMINAL] = 400000000,
+ },
+ [UFS] = {
+ .pd = &msm8996_vddcx,
+ .freq[LOWER] = 19200000,
+ .freq[LOW] = 100000000,
+ .freq[NOMINAL] = 200000000,
+ .freq[HIGH] = 240000000,
+ },
+ [PCIE] = {
+ .pd = &msm8996_vddcx,
+ .freq[LOWER] = 1011000,
+ },
+ [USB3] = {
+ .pd = &msm8996_vddcx,
+ .freq[LOWER] = 60000000,
+ .freq[LOW] = 120000000,
+ .freq[NOMINAL] = 150000000,
+ },
+};
+
+static const struct of_device_id rpmpd_performance_table[] = {
+ { .compatible = "qcom,sdhci-msm-v4", .data = (void *)SDHCI },
+ { .compatible = "qcom,ufshc", .data = (void *)UFS },
+ { .compatible = "qcom,pcie-msm8996", .data = (void *)PCIE },
+ { .compatible = "qcom,dwc3", .data = (void *)USB3 },
+ { }
+};
+
static const struct of_device_id rpmpd_match_table[] = {
{ .compatible = "qcom,rpmpd-msm8996", .data = &msm8996_desc },
{ }
@@ -230,6 +285,49 @@ 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_get_performance(struct device *dev, unsigned long rate)
+{
+ int i;
+ unsigned long index;
+ const struct of_device_id *id;
+
+ if (!rate)
+ return 0;
+
+ id = of_match_device(rpmpd_performance_table, dev);
+ if (!id)
+ return -EINVAL;
+
+ index = (unsigned long)id->data;
+ for (i = 0; i < MAX_LEVEL; i++)
+ if (msm8996_rpmpd_freq_map[index].freq[i] >= rate)
+ return i;
+
+ return MAX_LEVEL;
+}
+
static int rpmpd_probe(struct platform_device *pdev)
{
int i;
@@ -267,6 +365,8 @@ 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.set_performance_state = rpmpd_set_performance;
+ rpmpds[i]->pd.get_performance_state = rpmpd_get_performance;
pm_genpd_init(&rpmpds[i]->pd, NULL, true);
data->domains[i] = &rpmpds[i]->pd;
--
2.13.0.71.gd7076ec9c9cb
^ permalink raw reply related [flat|nested] 17+ messages in thread* [NOT-FOR-MERGE V8 5/6] mmc: sdhci-msm: Adapt the driver to use OPPs to set clocks/performance state
2017-06-21 7:10 [PATCH V8 0/6] PM / Domains: Power domain performance states Viresh Kumar
` (3 preceding siblings ...)
2017-06-21 7:10 ` [NOT-FOR-MERGE V8 4/6] soc: qcom: rpmpd: Add support for get/set performance state Viresh Kumar
@ 2017-06-21 7:10 ` Viresh Kumar
2017-06-21 7:10 ` [NOT-FOR-MERGE V8 6/6] remoteproc: qcom: q6v5: Vote for proxy powerdomain performance state Viresh Kumar
5 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2017-06-21 7:10 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
From: Rajendra Nayak <rnayak@codeaurora.org>
THIS IS TEST CODE, SHOULDN'T BE MERGED.
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.
NOT-signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
NOT-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 1fec06285f38..2cabdba93e0e 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -431,8 +431,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 8abc200d4fd3..9eb23063e78f 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 9d601dc0d646..2dfa4d58e113 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);
@@ -1267,6 +1275,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);
@@ -1288,6 +1303,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:
@@ -1313,6 +1330,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.13.0.71.gd7076ec9c9cb
^ permalink raw reply related [flat|nested] 17+ messages in thread* [NOT-FOR-MERGE V8 6/6] remoteproc: qcom: q6v5: Vote for proxy powerdomain performance state
2017-06-21 7:10 [PATCH V8 0/6] PM / Domains: Power domain performance states Viresh Kumar
` (4 preceding siblings ...)
2017-06-21 7:10 ` [NOT-FOR-MERGE V8 5/6] mmc: sdhci-msm: Adapt the driver to use OPPs to set clocks/performance state Viresh Kumar
@ 2017-06-21 7:10 ` Viresh Kumar
5 siblings, 0 replies; 17+ messages in thread
From: Viresh Kumar @ 2017-06-21 7:10 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
From: Rajendra Nayak <rnayak@codeaurora.org>
THIS IS TEST CODE, SHOULDN'T BE MERGED.
This patch just demonstrates the usage of pm_genpd_update_performance_state()
api in cases where users need to set performance state of a powerdomain without
having to do it via the OPP framework.
q6v5 remoteproc driver needs to proxy vote for performance states of multiple
powerdomains (but we currently only demonstate how it can be done for
one powerdomain, as there is no way to associate multiple powerdomains
to a device at this time) while it loads the firmware, and then releases
the vote, once the firmware is up and can vote for itself.
This is not a functional patch since rpmpd driver only supports msm8996
and there is no msm8996 support in the q6v5 remoteproc driver at this
point in mainline.
This patch is not tested as well.
NOT-signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
NOT-signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
drivers/remoteproc/qcom_q6v5_pil.c | 20 +++++++++++++-------
drivers/soc/qcom/rpmpd.c | 5 +++++
2 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c
index 8fd697a3cf8f..47c9dad98ee8 100644
--- a/drivers/remoteproc/qcom_q6v5_pil.c
+++ b/drivers/remoteproc/qcom_q6v5_pil.c
@@ -18,6 +18,7 @@
#include <linux/clk.h>
#include <linux/delay.h>
#include <linux/dma-mapping.h>
+#include <linux/pm_domain.h>
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/mfd/syscon.h>
@@ -152,6 +153,8 @@ struct q6v5 {
void *mpss_region;
size_t mpss_size;
+ bool has_perf_state;
+
struct qcom_rproc_subdev smd_subdev;
};
@@ -603,11 +606,12 @@ static int q6v5_start(struct rproc *rproc)
struct q6v5 *qproc = (struct q6v5 *)rproc->priv;
int ret;
- ret = q6v5_regulator_enable(qproc, qproc->proxy_regs,
- qproc->proxy_reg_count);
- if (ret) {
- dev_err(qproc->dev, "failed to enable proxy supplies\n");
- return ret;
+ if (qproc->has_perf_state) {
+ ret = pm_genpd_update_performance_state(qproc->dev, INT_MAX);
+ if (ret) {
+ dev_err(qproc->dev, "Failed to set performance state.\n");
+ return ret;
+ }
}
ret = q6v5_clk_enable(qproc->dev, qproc->proxy_clks,
@@ -671,8 +675,8 @@ static int q6v5_start(struct rproc *rproc)
q6v5_clk_disable(qproc->dev, qproc->proxy_clks,
qproc->proxy_clk_count);
- q6v5_regulator_disable(qproc, qproc->proxy_regs,
- qproc->proxy_reg_count);
+ if (qproc->has_perf_state)
+ pm_genpd_update_performance_state(qproc->dev, 0);
return 0;
@@ -1043,6 +1047,8 @@ static int q6v5_probe(struct platform_device *pdev)
if (ret)
goto free_rproc;
+ qproc->has_perf_state = pm_genpd_has_performance_state(&qproc->dev);
+
return 0;
free_rproc:
diff --git a/drivers/soc/qcom/rpmpd.c b/drivers/soc/qcom/rpmpd.c
index 7ef81429c5c5..b1af1442ebe9 100644
--- a/drivers/soc/qcom/rpmpd.c
+++ b/drivers/soc/qcom/rpmpd.c
@@ -145,6 +145,7 @@ enum msm8996_devices {
UFS,
PCIE,
USB3,
+ Q6V5_PIL,
};
static struct rpmpd_freq_map msm8996_rpmpd_freq_map[] = {
@@ -171,6 +172,10 @@ static struct rpmpd_freq_map msm8996_rpmpd_freq_map[] = {
.freq[LOW] = 120000000,
.freq[NOMINAL] = 150000000,
},
+ [Q6V5_PIL] = {
+ .pd = &msm8996_vddcx,
+ .freq[HIGH] = INT_MAX,
+ },
};
static const struct of_device_id rpmpd_performance_table[] = {
--
2.13.0.71.gd7076ec9c9cb
^ permalink raw reply related [flat|nested] 17+ messages in thread