Re: [PATCH 08/13] OMAP2+: powerdomain: control power domains next state
Hi Todd,
On Fri, Jul 29, 2011 at 8:00 PM, Todd Poynor wrote:
> On Fri, Jul 29, 2011 at 10:47:43AM +0200, Jean Pihet wrote:
>> On Fri, Jul 29, 2011 at 9:59 AM, Todd Poynor wrote:
> ...
>> > All min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE paths need
>> > free_new_user = 1.
>> free_new_user = 1 is only needed if no existing constraint has been
>> found, i.e. user stays at NULL. This is implemented in the check for
>> an existing constraint (plist_for_each_entry(...)).
>
> Oops, I meant to say it applies in all cases where min_latency ==
> PM_QOS_DEV_LAT_DEFAULT_VALUE, since the new node is only used for
> adding a constraint (and no existing constraint found). I'd suggest
> something like:
>
> if (min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE) {
> new_user = kzalloc(sizeof(struct pwrdm_wkup_constraints_entry),
> GFP_KERNEL);
>
> free_new_user = 1;
> }
>
> and then set free_new_user = 0 only if no existing constraint is found
> for the add case. Because it's easy to miss cases where the
> allocated memory needs to be freed when that's not the default, and you
> might as well skip the allocate on a constraint removal. Pretty minor
> point, though.
This has been addressed in the latest patch set (v4).
Regards,
Jean
>
>
> Todd
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/13] OMAP2+: powerdomain: control power domains next state
On Fri, Jul 29, 2011 at 10:47:43AM +0200, Jean Pihet wrote:
> On Fri, Jul 29, 2011 at 9:59 AM, Todd Poynor wrote:
...
> > All min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE paths need
> > free_new_user = 1.
> free_new_user = 1 is only needed if no existing constraint has been
> found, i.e. user stays at NULL. This is implemented in the check for
> an existing constraint (plist_for_each_entry(...)).
Oops, I meant to say it applies in all cases where min_latency ==
PM_QOS_DEV_LAT_DEFAULT_VALUE, since the new node is only used for
adding a constraint (and no existing constraint found). I'd suggest
something like:
if (min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE) {
new_user = kzalloc(sizeof(struct pwrdm_wkup_constraints_entry),
GFP_KERNEL);
free_new_user = 1;
}
and then set free_new_user = 0 only if no existing constraint is found
for the add case. Because it's easy to miss cases where the
allocated memory needs to be freed when that's not the default, and you
might as well skip the allocate on a constraint removal. Pretty minor
point, though.
Todd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/13] OMAP2+: powerdomain: control power domains next state
Todd,
On Fri, Jul 29, 2011 at 9:59 AM, Todd Poynor wrote:
> On Thu, Jul 28, 2011 at 10:30:15AM +0200, jean.pi...@newoldbits.com wrote:
> ...
>> +int pwrdm_set_wkup_lat_constraint(struct powerdomain *pwrdm, void *cookie,
>> + long min_latency)
>> +{
>> + struct pwrdm_wkup_constraints_entry *user = NULL;
>> + struct pwrdm_wkup_constraints_entry *tmp_user, *new_user;
>> + int ret = 0, free_new_user = 0, free_node = 0;
>> + long value = 0;
>> + unsigned long flags;
>> +
>> + pr_debug("powerdomain: %s: pwrdm %s, cookie=0x%p, min_latency=%ld\n",
>> + __func__, pwrdm->name, cookie, min_latency);
>> +
>> + new_user = kzalloc(sizeof(struct pwrdm_wkup_constraints_entry),
>> + GFP_KERNEL);
>> + if (!new_user) {
>> + pr_err("%s: FATAL ERROR: kzalloc failed\n", __func__);
>> + return -ENOMEM;
>> + }
>> +
>> + spin_lock_irqsave(&pwrdm->wkup_lat_plist_lock, flags);
>> +
>> + /* Check if there already is a constraint for cookie */
>> + plist_for_each_entry(tmp_user, &pwrdm->wkup_lat_plist_head, node) {
>> + if (tmp_user->cookie == cookie) {
>> + user = tmp_user;
>> + free_new_user = 1;
>> + break;
>> + }
>> + }
>> +
>> + if (min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE) {
>> + /* If nothing to update, job done */
>> + if (user && (user->node.prio == min_latency))
>> + goto exit_ok;
>> +
>> + if (!user) {
>> + /* Add new entry to the list */
>> + user = new_user;
>> + user->cookie = cookie;
>> + } else {
>> + /* Update existing entry */
>> + plist_del(&user->node, &pwrdm->wkup_lat_plist_head);
>> + }
>> +
>> + plist_node_init(&user->node, min_latency);
>> + plist_add(&user->node, &pwrdm->wkup_lat_plist_head);
>> + } else {
>> + /* Remove the constraint from the list */
>> + if (!user) {
>> + pr_err("%s: Error: no prior constraint to release\n",
>> + __func__);
>> + ret = -EINVAL;
>> + goto exit_error;
>> + }
>> +
>> + plist_del(&user->node, &pwrdm->wkup_lat_plist_head);
>> + free_node = 1;
>
> All min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE paths need
> free_new_user = 1.
free_new_user = 1 is only needed if no existing constraint has been
found, i.e. user stays at NULL. This is implemented in the check for
an existing constraint (plist_for_each_entry(...)).
>(Or maybe change the logic to check user !=
> new_user and free new_user if so.)
That could be done as well.
>
>> + }
>> +
>> +exit_ok:
>> + /* Find the strongest constraint from the list */
>> + if (!plist_head_empty(&pwrdm->wkup_lat_plist_head))
>> + value = plist_first(&pwrdm->wkup_lat_plist_head)->prio;
>> +
>> + spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags);
>> +
>> + if (free_node)
>> + kfree(user);
>> +
>> + if (free_new_user)
>> + kfree(new_user);
>> +
>> + /* Apply the constraint to the pwrdm */
>> + pr_debug("powerdomain: %s: pwrdm %s, value=%ld\n",
>> + __func__, pwrdm->name, value);
>> + pwrdm_wakeuplat_update_pwrst(pwrdm, value);
>> +
>> + return 0;
>> +
>> +exit_error:
>> + spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags);
>
> Need:
> kfree(new_user);
Correct!
BTW I have a new version (patch here below) that improves the cases
with latency = PM_QOS_DEV_LAT_DEFAULT_VALUE. This will come in v4
after testing.
>
>> + return ret;
>> +}
>
>
> Todd
>
Thanks for reviewing!
Regards,
Jean
---
Patch for cases with latency = PM_QOS_DEV_LAT_DEFAULT_VALUE:
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index c0f48ab..2e9379b 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -206,7 +206,7 @@ static int _pwrdm_post_transition_cb(struct
powerdomain *pwrdm, void *unused)
* pwrdm_wakeuplat_update_pwrst - Update power domain power state if needed
* @pwrdm: struct powerdomain * to which requesting device belongs to.
* @min_latency: the allowed wake-up latency for the given power domain. A
- * value of 0 means 'no constraint' on the pwrdm.
+ * value of PM_QOS_DEV_LAT_DEFAULT_VALUE means 'no constraint' on the pwrdm.
*
* Finds the power domain next power state that fulfills the constraint.
* Programs a new target state if it is different from current power state.
@@ -232,7 +232,7 @@ static int pwrdm_wakeuplat_update_pwrst(struct
powerdomain *pwrdm,
/* Find power state with wakeup latency < minimum constraint */
for (new_state = 0x0; new_state < PWRDM_MAX_PWRSTS; new_
Re: [PATCH 08/13] OMAP2+: powerdomain: control power domains next state
On Thu, Jul 28, 2011 at 10:30:15AM +0200, jean.pi...@newoldbits.com wrote:
...
> +int pwrdm_set_wkup_lat_constraint(struct powerdomain *pwrdm, void *cookie,
> + long min_latency)
> +{
> + struct pwrdm_wkup_constraints_entry *user = NULL;
> + struct pwrdm_wkup_constraints_entry *tmp_user, *new_user;
> + int ret = 0, free_new_user = 0, free_node = 0;
> + long value = 0;
> + unsigned long flags;
> +
> + pr_debug("powerdomain: %s: pwrdm %s, cookie=0x%p, min_latency=%ld\n",
> + __func__, pwrdm->name, cookie, min_latency);
> +
> + new_user = kzalloc(sizeof(struct pwrdm_wkup_constraints_entry),
> +GFP_KERNEL);
> + if (!new_user) {
> + pr_err("%s: FATAL ERROR: kzalloc failed\n", __func__);
> + return -ENOMEM;
> + }
> +
> + spin_lock_irqsave(&pwrdm->wkup_lat_plist_lock, flags);
> +
> + /* Check if there already is a constraint for cookie */
> + plist_for_each_entry(tmp_user, &pwrdm->wkup_lat_plist_head, node) {
> + if (tmp_user->cookie == cookie) {
> + user = tmp_user;
> + free_new_user = 1;
> + break;
> + }
> + }
> +
> + if (min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE) {
> + /* If nothing to update, job done */
> + if (user && (user->node.prio == min_latency))
> + goto exit_ok;
> +
> + if (!user) {
> + /* Add new entry to the list */
> + user = new_user;
> + user->cookie = cookie;
> + } else {
> + /* Update existing entry */
> + plist_del(&user->node, &pwrdm->wkup_lat_plist_head);
> + }
> +
> + plist_node_init(&user->node, min_latency);
> + plist_add(&user->node, &pwrdm->wkup_lat_plist_head);
> + } else {
> + /* Remove the constraint from the list */
> + if (!user) {
> + pr_err("%s: Error: no prior constraint to release\n",
> +__func__);
> + ret = -EINVAL;
> + goto exit_error;
> + }
> +
> + plist_del(&user->node, &pwrdm->wkup_lat_plist_head);
> + free_node = 1;
All min_latency != PM_QOS_DEV_LAT_DEFAULT_VALUE paths need
free_new_user = 1. (Or maybe change the logic to check user !=
new_user and free new_user if so.)
> + }
> +
> +exit_ok:
> + /* Find the strongest constraint from the list */
> + if (!plist_head_empty(&pwrdm->wkup_lat_plist_head))
> + value = plist_first(&pwrdm->wkup_lat_plist_head)->prio;
> +
> + spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags);
> +
> + if (free_node)
> + kfree(user);
> +
> + if (free_new_user)
> + kfree(new_user);
> +
> + /* Apply the constraint to the pwrdm */
> + pr_debug("powerdomain: %s: pwrdm %s, value=%ld\n",
> + __func__, pwrdm->name, value);
> + pwrdm_wakeuplat_update_pwrst(pwrdm, value);
> +
> + return 0;
> +
> +exit_error:
> + spin_unlock_irqrestore(&pwrdm->wkup_lat_plist_lock, flags);
Need:
kfree(new_user);
> + return ret;
> +}
Todd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/13] OMAP2+: powerdomain: control power domains next state
From: Jean Pihet
When a PM QoS device latency constraint is requested or removed the
PM QoS layer notifies the underlying layer with the updated aggregated
constraint value. The constraint is stored in the powerdomain constraints
list and then applied to the corresponding power domain.
The power domains get the next power state programmed directly in the
registers via pwrdm_wakeuplat_update_pwrst.
Tested on OMAP3 Beagleboard and OMAP4 Pandaboard in RET/OFF using
wake-up latency constraints on MPU, CORE and PER.
Signed-off-by: Jean Pihet
---
arch/arm/mach-omap2/powerdomain.c | 187 +
arch/arm/mach-omap2/powerdomain.h | 33 ++-
2 files changed, 218 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-omap2/powerdomain.c
b/arch/arm/mach-omap2/powerdomain.c
index 63c3e7a..c0f48ab 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -17,8 +17,10 @@
#include
#include
#include
+#include
#include
#include
+#include
#include
#include "cm2xxx_3xxx.h"
@@ -104,6 +106,12 @@ static int _pwrdm_register(struct powerdomain *pwrdm)
for (i = 0; i < pwrdm->banks; i++)
pwrdm->ret_mem_off_counter[i] = 0;
+ /* Initialize the per-od wake-up constraints list and spinlock */
+ spin_lock_init(&pwrdm->wkup_lat_plist_lock);
+ plist_head_init(&pwrdm->wkup_lat_plist_head,
+ &pwrdm->wkup_lat_plist_lock);
+
+ /* Initialize the pwrdm state */
pwrdm_wait_transition(pwrdm);
pwrdm->state = pwrdm_read_pwrst(pwrdm);
pwrdm->state_counter[pwrdm->state] = 1;
@@ -194,6 +202,77 @@ static int _pwrdm_post_transition_cb(struct powerdomain
*pwrdm, void *unused)
return 0;
}
+/**
+ * pwrdm_wakeuplat_update_pwrst - Update power domain power state if needed
+ * @pwrdm: struct powerdomain * to which requesting device belongs to.
+ * @min_latency: the allowed wake-up latency for the given power domain. A
+ * value of 0 means 'no constraint' on the pwrdm.
+ *
+ * Finds the power domain next power state that fulfills the constraint.
+ * Programs a new target state if it is different from current power state.
+ * The power domains get the next power state programmed directly in the
+ * registers.
+ *
+ * Returns 0 upon success.
+ */
+static int pwrdm_wakeuplat_update_pwrst(struct powerdomain *pwrdm,
+ long min_latency)
+{
+ int ret = 0, new_state = 0;
+
+ if (!pwrdm) {
+ WARN(1, "powerdomain: %s: invalid parameter(s)", __func__);
+ return -EINVAL;
+ }
+
+ /*
+* Apply constraints to power domains by programming
+* the pwrdm next power state.
+*/
+
+ /* Find power state with wakeup latency < minimum constraint */
+ for (new_state = 0x0; new_state < PWRDM_MAX_PWRSTS; new_state++) {
+ if (min_latency == 0 ||
+ pwrdm->wakeup_lat[new_state] <= min_latency)
+ break;
+ }
+
+ switch (new_state) {
+ case PWRDM_FUNC_PWRST_OFF:
+ new_state = PWRDM_POWER_OFF;
+ break;
+ case PWRDM_FUNC_PWRST_OSWR:
+ pwrdm_set_logic_retst(pwrdm, PWRDM_POWER_OFF);
+ new_state = PWRDM_POWER_RET;
+ break;
+ case PWRDM_FUNC_PWRST_CSWR:
+ pwrdm_set_logic_retst(pwrdm, PWRDM_POWER_RET);
+ new_state = PWRDM_POWER_RET;
+ break;
+ case PWRDM_FUNC_PWRST_INACTIVE:
+ new_state = PWRDM_POWER_INACTIVE;
+ break;
+ case PWRDM_FUNC_PWRST_ON:
+ new_state = PWRDM_POWER_ON;
+ break;
+ default:
+ pr_warn("powerdomain: requested latency constraint not "
+ "supported %s set to ON state\n", pwrdm->name);
+ new_state = PWRDM_POWER_ON;
+ break;
+ }
+
+ if (pwrdm_read_next_pwrst(pwrdm) != new_state)
+ ret = omap_set_pwrdm_state(pwrdm, new_state);
+
+ pr_debug("powerdomain: %s pwrst: curr=%d, prev=%d next=%d "
+"min_latency=%ld, set_state=%d\n", pwrdm->name,
+pwrdm_read_pwrst(pwrdm), pwrdm_read_prev_pwrst(pwrdm),
+pwrdm_read_next_pwrst(pwrdm), min_latency, new_state);
+
+ return ret;
+}
+
/* Public functions */
/**
@@ -933,6 +1012,114 @@ int pwrdm_post_transition(void)
return 0;
}
+/*
+ * pwrdm_set_wkup_lat_constraint - Set/update/remove a powerdomain wakeup
+ * latency constraint and apply it
+ * @pwrdm: struct powerdomain * which the constraint applies to
+ * @cookie: constraint identifier, used for tracking.
+ * @min_latency: minimum wakeup latency constraint (in microseconds) for
+ * the given pwrdm. The value of PM_QOS_DEV_WAKEUP_LAT_DEFAULT_VALUE
+ * removes the constraint.
+ *
+ * Tracks the constraints by @cookie.
+ * Constraint set/update: Adds
