Kevin,
Thanks for reviewing!
All comments below fixed in the code. The updated series is coming asap.
More comments inlined below.
On Thu, Mar 17, 2011 at 9:36 PM, Kevin Hilman <khilman@xxxxxx> wrote:
> Jean Pihet <jean.pihet@xxxxxxxxxxxxxx> writes:
>
>> The code at omap device level manages the constraints: storage,
>> tracking of requesters and dispatching to the low level
>> code (e.g. powerdomain for the wake-up latency constraints).
>>
>> Tested on OMAP3 Beagleboard in RET/OFF using wake-up latency constraints
>> on MPU, CORE and PER.
>>
>> Signed-off-by: Jean Pihet <j-pihet@xxxxxx>
>
> Subject prefix should be: OMAP2+: omap_device: ...
>
>> ---
>> arch/arm/plat-omap/include/plat/omap_device.h | 14 ++
>> arch/arm/plat-omap/omap_device.c | 202 +++++++++++++++++++++++++
>> 2 files changed, 216 insertions(+), 0 deletions(-)
>>
>> diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
>> index e4c349f..d4766c4 100644
>> --- a/arch/arm/plat-omap/include/plat/omap_device.h
>> +++ b/arch/arm/plat-omap/include/plat/omap_device.h
>> @@ -32,9 +32,11 @@
>> #define __ARCH_ARM_PLAT_OMAP_INCLUDE_MACH_OMAP_DEVICE_H
>>
>> #include <linux/kernel.h>
>> +#include <linux/plist.h>
>> #include <linux/platform_device.h>
>>
>> #include <plat/omap_hwmod.h>
>> +#include <plat/omap-pm.h>
>>
>> extern struct device omap_device_parent;
>>
>> @@ -73,6 +75,15 @@ struct omap_device {
>> s8 pm_lat_level;
>> u8 hwmods_cnt;
>> u8 _state;
>> +
>> +};
>> +
>> +/* Linked list for the devices constraints entries */
>> +struct omap_device_constraints_entry {
>> + struct device *req_dev;
>> + void *target;
>> + unsigned long constraint_value;
>
> constratint_ prefix not needed
>
> Also, should this be 'long' (instead of unsigned long' to match the
> value from the API below?
>
>> + struct plist_node node;
>> };
>>
>> /* Device driver interface (call via platform_data fn ptrs) */
>> @@ -107,6 +118,9 @@ void __iomem *omap_device_get_rt_va(struct omap_device *od);
>> int omap_device_align_pm_lat(struct platform_device *pdev,
>> u32 new_wakeup_lat_limit);
>> struct powerdomain *omap_device_get_pwrdm(struct omap_device *od);
>> +int omap_device_set_dev_constraint(enum omap_pm_constraint_class class,
>> + struct device *req_dev,
>> + struct device *dev, long t);
>> u32 omap_device_get_context_loss_count(struct platform_device *pdev);
>>
>> /* Other */
>> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
>> index 9bbda9a..9199d3e 100644
>> --- a/arch/arm/plat-omap/omap_device.c
>> +++ b/arch/arm/plat-omap/omap_device.c
>> @@ -292,10 +292,205 @@ static void _add_optional_clock_clkdev(struct omap_device *od,
>> }
>> }
>>
>> +/* plist that stores the devices wake-up latency constraints */
>> +static struct plist_head _wkup_lat_constraints_list;
>> +/* Spinlock that protects the constraints lists */
>> +static spinlock_t _constraints_lock;
>> +/* Mutex that protects the constraints lists ops */
>> +static struct mutex _constraints_mutex;
>> +
>> +/*
>> + * _store_constraint: add/update/remove a constraint from a plist
>> + *
>> + * @constraints_list: plist to use
>> + * @req_dev: constraint requester, used to track the requests
>> + * @target: target which the constraint applies to (e.g. power domain ID or
>> + * ptr for wake-up latency constraints)
>> + * @value: constraint value. The plist is sorted by the value. -1 remove the
>> + * constraint from the list
>> + * @ascending: return the lowest constraint value if set to 1, return the
>> + * highest value if not.
>> + *
>> + * Returns the strongest constraint value for the given target, 0 in the
>> + * case there is no constraint on the given target or a negative value in
>> + * case of error.
>> + * The caller must check the validity of the parameters.
>> + */
>> +static long _store_constraint(struct plist_head *constraints_list,
>> + struct device *req_dev, void *target,
>> + long value, int ascending)
>> +{
>> + struct omap_device_constraints_entry *user;
>> + int found = 0, ret = 0;
>> +
>> + mutex_lock(&_constraints_mutex);
>> +
>> + /* Check if the constraint requester is already in the list */
>> + plist_for_each_entry(user, constraints_list, node) {
>> + if (user->req_dev == req_dev) {
>> + found = 1;
>> + break;
>> + }
>> + }
>
> You should probably release the mutex here so it's not held over the
> kzalloc (which can sleep.)
The mutex is removed and only the spinlock is used to protect the list ops.
>
> Minor: I'm not a fan of this common 'found = 1' technique. Rather, use
> a temporary list iterator, e.g:
>
> struct omap_device_constraints_entry *user = NULL, *tmp_user;
>
> plist_for_each_entry(tmp_user, constraints_list, node) {
> if (user->req_dev == req_dev) {
> user = tmp_user;
> break;
> }
> }
>
> Then, rather than checking 'found' below, check 'user'.
That is indeed cleaner.
>
>> + if (value >= 0) {
>> + /* Add new entry to the list or update existing request */
>> + if (found &&
>> + user->constraint_value == value &&
>> + user->target == target) {
>> + goto exit_ok;
>> + } else if (!found) {
>> + user = kzalloc(
>> + sizeof(struct omap_device_constraints_entry),
>> + GFP_KERNEL);
>> + if (!user) {
>> + pr_err("%s: FATAL ERROR: kzalloc failed\n",
>> + __func__);
>> + ret = -ENOMEM;
>> + goto exit_error;
>> + }
>> + user->req_dev = req_dev;
>> + } else {
>> + plist_del(&user->node, constraints_list);
>> + }
>> +
>> + plist_node_init(&user->node, value);
>> + plist_add(&user->node, constraints_list);
>> + user->node.prio = user->constraint_value = value;
>> + user->target = target;
>> + } else {
>> + /* Remove the constraint from the list */
>> + if (!found) {
>> + pr_err("%s: Error: no prior constraint to release\n",
>> + __func__);
>> + ret = -EINVAL;
>> + goto exit_error;
>> + }
>> +
>> + plist_del(&user->node, constraints_list);
>> + kfree(user);
>> + }
>> +
>> +exit_ok:
>> + /* Find the strongest constraint for the given target */
>> + ret = 0;
>> + if (ascending) {
>> + list_for_each_entry(user, &(constraints_list)->node_list,
>> + node.plist.node_list) {
>> + /* Find the lowest (i.e. first) value for the target */
>> + if (user->target == target) {
>> + ret = user->constraint_value;
>> + break;
>> + }
>> + }
>> + } else {
>> + list_for_each_entry_reverse(user,
>> + &(constraints_list)->node_list,
>> + node.plist.node_list) {
>> + /* Find the highest (i.e. last) value for the target */
>> + if (user->target == target) {
>> + ret = user->constraint_value;
>> + break;
>> + }
>> + }
>> + }
>
> Hmm, why can't you use plist_first() and plist_last() here?
Because the plist is sorted by the 'prio' field (which is 'value' in
this code) and this function searches for the strongest constraint for
a given target. So it is needed to iterate through the list in order
to find the first (or last) constraint with the right target.
>
>> +exit_error:
>> + mutex_unlock(&_constraints_mutex);
>> +
>> + return ret;
>> +}
>>
>> /* Public functions for use by core code */
>>
>> /**
>> + * omap_device_set_dev_constraint - set/release a device constraint
>> + * @class: constraint class
>> + * @req_dev: constraint requester, used for tracking the constraints
>> + * @dev: device to apply the constraint to. Must have an associated omap_device
>> + * @t: constraint value. A value of -1 removes the constraint.
>> + *
>> + * Using the primary hwmod, set/release a device constraint for the dev
>> + * device, requested by the req_dev device. Depending of the constraint class
>> + * this code calls the appropriate low level code, e.g. power domain for
>> + * the wake-up latency constraints.
>> + *
>> + * If any hwmods exist for the omap_device assoiated with @dev,
>> + * set/release the constraint for the corresponding hwmods, otherwise return
>> + * -EINVAL.
>> + */
>> +int omap_device_set_dev_constraint(enum omap_pm_constraint_class class,
>> + struct device *req_dev,
>> + struct device *dev, long t)
>> +{
>> + struct omap_device *od;
>> + struct omap_hwmod *oh;
>> + struct platform_device *pdev;
>> + struct powerdomain *pwrdm = NULL;
>> + u32 ret = -EINVAL;
>> +
>> + /* Look for the platform device for dev */
>> + pdev = to_platform_device(dev);
>> +
>> + /* Try to catch non platform devices. */
>> + if (pdev->name == NULL) {
>
> This should check for a valid omap_device, not platform_device.
This check remains but the check below is changed, see below.
>
>> + pr_err("%s: Error: platform device for device %s not valid\n",
>> + __func__, dev_name(dev));
>> + return -EINVAL;
>> + }
>> +
>> + /* Find the associated omap_device for dev */
>> + od = _find_by_pdev(pdev);
>> + if (!(od->hwmods_cnt)) {
>> + pr_err("%s: Error: No hwmod for device %s\n",
>> + __func__, dev_name(dev));
>> + return -EINVAL;
>> + }
Changed to:
+ if (!od || (od->hwmods_cnt != 1)) {
+ pr_err("%s: Error: No unique hwmod for device %s\n",
+ __func__, dev_name(dev));
+ return -EINVAL;
+ }
>> +
>> + /* Find the associated omap_hwmod for dev */
>> + oh = od->hwmods[0];
>> +
>> + switch (class) {
>> + case OMAP_PM_CONSTRAINT_WKUP_LAT:
>> + /* Find the pwrdm associated to dev */
>
> comment doesn't add anything to code
>
>> + pwrdm = omap_device_get_pwrdm(od);
>> + if (!pwrdm) {
>> + pr_err("%s: Error: No pwrdm for device %s\n",
>> + __func__, dev_name(dev));
>> + ret = -EINVAL;
>> + break;
>> + }
>> +
>> + /*
>> + * Store the constraint in the appropriate list and find the
>> + * strongest constraint for the given pwrdm
>> + */
>> + ret = _store_constraint(&_wkup_lat_constraints_list,
>> + req_dev, (void *) pwrdm, t, 1);
>> +
>> + /* Apply the constraint to the corresponding pwrdm */
>> + if (ret >= 0) {
>> + ret = omap_hwmod_update_power_state(oh, ret);
>
> This function doesn't exist yet, so the series up to here will not
> compile.
>
>> + } else {
>> + pr_err("%s: Error storing the constraint for device "
>> + "%s\n", __func__, dev_name(dev));
>> + }
>> +
>> + break;
>> + case OMAP_PM_CONSTRAINT_THROUGHPUT:
>> + WARN(1, "OMAP PM: %s: Bus throughput constraint class \
>> + not implemented\n", __func__);
>> + ret = -EINVAL;
>> + break;
>> + default:
>> + WARN(1, "OMAP PM: %s: invalid constraint class %d",
>> + __func__, class);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> * omap_device_get_context_loss_count - get lost context count
>> * @od: struct omap_device *
>> *
>> @@ -824,6 +1019,13 @@ struct device omap_device_parent = {
>>
>> static int __init omap_device_init(void)
>> {
>> + /* Initialize priority ordered list for wakeup latency constraint */
>> + spin_lock_init(&_constraints_lock);
>> + plist_head_init(&_wkup_lat_constraints_list, &_constraints_lock);
>> +
>> + /* res_mutex protects res_list add and del ops */
>> + mutex_init(&_constraints_mutex);
>> +
>> return device_register(&omap_device_parent);
>> }
>> core_initcall(omap_device_init);
>
> Kevin
>
Jean
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
