Jean Pihet <jean.pihet@xxxxxxxxxxxxxx> writes:
> On Tue, Mar 8, 2011 at 3:15 AM, Kevin Hilman <khilman@xxxxxx> wrote:
>> Jean Pihet <jean.pihet@xxxxxxxxxxxxxx> writes:
>>
>>> Implement OMAP PM layer omap_pm_set_max_dev_wakeup_lat API by
>>> creating similar APIs at the omap_device and omap_hwmod levels. The
>>> omap_hwmod level call is the layer with access to the powerdomain
>>> core, so it is the place where the powerdomain is queried to set and
>>> release the constraints.
>>>
>>> NOTE: only works for devices which have been converted to use
>>> Â Â Â omap_device/omap_hwmod.
>>>
>>> Longer term, we could possibly remove this API from the OMAP PM layer,
>>> and instead directly use the device level API.
>>>
>>> The power domains get the next power state programmed directly in
>>> the registers via pwrdm_wakeuplat_update_pwrst.
>>>
>>> Note about PM QOS: the MPU and CORE power domains get the next power
>>> state via cpuidle, which get the strongest wake-up latency constraint
>>> by querying PM QOS. The usage of PM QOS is temporary, until a generic
>>> solution is in place.
>>>
>>> Based on Vibhore's original patch, adapted to omap_device, omap_hwmod
>>> and PM QOS frameworks.
>>
>> I haven't got to a detailed review of this code yet, but did do some
>> experiements and have some general comments on the code/design to get
>> started.
>>
>> Also, I had a problem doing a real dumb test until I figured out the
>> problem with the init sequence. ÂI tried setting a constraint in the
>> device init code for UART (mach-omap2/serial.c:omap_serial_init_port()),
>> and then I realized that that runs before
>> mach-omap2/pm34xx.c:pwrdms_setup() which also calls
>> omap_set_pwrdm_state() for each powerdomain to initialize.
>
> Do we need to change the behavior at init?
>
I think so.
At a minimum, it should be documented somewhere that the constraints API
cannot be used before pwrdms_setup(). But since that happens much later
in the boot process than device init code, that might be too strict.
>> Also, for debug purposes, it might be useful to have a per-device sysfs
>> interface to setting this wakeup latency constraint. ÂSomething like
>>
>> Â /sys/devices/platform/omap/.../power/wakeup_latency
> I do not see this as a debug interface but rather an API to the user space.
> If that is the case there are some issues to deal with, cf. below.
By debug, I was thinking under '#ifdef CONFIG_PM_DEBUG'. Even if you
did this as a separate patch that we did not try to get upstream, it
would be extremely valuable for testing this framework.
>> I'm not sure exactly what to set the requesting device to though.
> And also, how to track the requesters? PM QOS is using a /dev node
> that must be kept open as long as the constraints remains valid.
A dummy device could be created for all sysfs requesters since you would
want subsequent writes to override previous ones.
>> As far as implementation goes, you've so far implemented only wakeup
>> latencies, but not througput. ÂWhen you implement throughput you will
>> have to duplicate large parts of this code and data structures for
>> throughput, and if ever add some other constraint (frequency, voltage)
>> it would need to be duplicated again.
>>
>> Maybe now is the time to consider an interface to add a generic
>> per-device constraint, with a type (latency, throughput, etc.), or
>> "class" as it's called in PM QoS. ÂFor now the type/class does not need
>> to be exposed externally, but will make implementing new constraint
>> types much easer.
> Ok that makes sense.
> In the current patch the constraints plist is stored inside the
> powerdomain structure. This does not apply to throughput, frequency
> and voltage constraints.
> Should the list constraint be managed in the OMAP PM layer instead of
> in the power domain code? If so where to store the constraints?
I was thinking at the omap_device layer, where there would be a
constraint plist managed for each class of constraint.
>> Some other comments below...
>>
>>> Signed-off-by: Jean Pihet <j-pihet@xxxxxx>
>>> Cc: Vibhore Vardhan <vvardhan@xxxxxx>
>>> ---
>>> Based on khilman's pm-core branch
>>>
>>> Âarch/arm/mach-omap2/omap_hwmod.c       Â|  62 ++++++++-
>>> Âarch/arm/mach-omap2/powerdomain.c       | Â197 +++++++++++++++++++++++++
>>> Âarch/arm/mach-omap2/powerdomain.h       |  39 +++++-
>>> Âarch/arm/mach-omap2/powerdomains3xxx_data.c  |  60 ++++++++
>>> Âarch/arm/plat-omap/include/plat/omap_device.h | Â Â2 +
>>> Âarch/arm/plat-omap/include/plat/omap_hwmod.h Â| Â Â2 +
>>> Âarch/arm/plat-omap/omap-pm-constraints.c   Â| Â121 +++++++--------
>>> Âarch/arm/plat-omap/omap_device.c       Â|  28 ++++
>>> Â8 files changed, 446 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
>>> index 028efda..bad8248 100644
>>> --- a/arch/arm/mach-omap2/omap_hwmod.c
>>> +++ b/arch/arm/mach-omap2/omap_hwmod.c
>>> @@ -142,6 +142,7 @@
>>> Â#include "powerdomain.h"
>>> Â#include <plat/clock.h>
>>> Â#include <plat/omap_hwmod.h>
>>> +#include <plat/omap_device.h>
>>> Â#include <plat/prcm.h>
>>>
>>> Â#include "cm2xxx_3xxx.h"
>>> @@ -2267,10 +2268,69 @@ ohsps_unlock:
>>> Â}
>>>
>>> Â/**
>>> + * omap_hwmod_set_max_dev_wakeup_lat - set a device wake-up constraint
>>> + * @oh: the device of @oh to set a constraint on.
>>> + * @req_oh: the device of @req_oh is the requester of the constraint.
>>> + * @t: wakeup latency constraint (us). -1 removes the existing constraint.
>>> + *
>>> + * Query the powerdomain of @oh to set/release the wake-up constraint.
>>> + * @oh is used to determine which power domain to set a constraint on.
>>> + * @req_oh is used to record the requester for later update or removal
>>> + * of a constraint.
>>> + *
>>> + * Returns -EINVAL if @oh or @req_oh have no power domain, or the return
>>> + * code from the pwrdm function (pwrdm_wakeuplat_set/release_constraint)
>>> + * of the powerdomain assocated with @oh.
>>> + */
>>> +int omap_hwmod_set_max_dev_wakeup_lat(struct omap_hwmod *req_oh,
>>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct omap_hwmod *oh, long t)
>>> +{
>>> + Â Â struct device *req_dev;
>>> + Â Â struct platform_device *req_pdev;
>>> + Â Â struct powerdomain *pwrdm;
>>> +
>>> + Â Â pwrdm = omap_hwmod_get_pwrdm(oh);
>>> +
>>> + Â Â /* Catch devices with undefined powerdomains */
>>> + Â Â if (!PTR_ERR(pwrdm)) {
>>> + Â Â Â Â Â Â pr_err("omap_hwmod: Error: could not find parent "
>>> + Â Â Â Â Â Â Â Â Â Â "powerdomain for %s\n", oh->name);
>>> + Â Â Â Â Â Â return -EINVAL;
>>> + Â Â }
>>> +
>>> + Â Â req_pdev = &(req_oh->od->pdev);
>>> + Â Â if (!PTR_ERR(req_pdev)) {
>>> + Â Â Â Â Â Â pr_err("omap_hwmod: Error: pdev not found for oh %s\n",
>>> + Â Â Â Â Â Â Â Â Â Âoh->name);
>>> + Â Â Â Â Â Â return -EINVAL;
>>> + Â Â }
>>> +
>>> + Â Â req_dev = &(req_pdev->dev);
>>> + Â Â if (!PTR_ERR(req_dev)) {
>>> + Â Â Â Â Â Â pr_err("omap_hwmod: Error: device not found for oh %s\n",
>>> + Â Â Â Â Â Â Â Â Â Âoh->name);
>>> + Â Â Â Â Â Â return -EINVAL;
>>> + Â Â }
>>> +
>>> + Â Â /* Call set/release_constraint for the given pwrdm */
>>> + Â Â if (t == -1) {
>>> + Â Â Â Â Â Â pr_debug("omap_hwmod: remove max device latency constraint: "
>>> + Â Â Â Â Â Â Â Â Â Â Â"oh %s, pwrdm %s, req by oh %s\n",
>>> + Â Â Â Â Â Â Â Â Â Â Âoh->name, pwrdm->name, req_oh->name);
>>> + Â Â } else {
>>> + Â Â Â Â Â Â pr_debug("omap_hwmod: add max device latency constraint: "
>>> + Â Â Â Â Â Â Â Â Â Â Â"oh %s, t = %ld usec, pwrdm %s, req by oh %s\n",
>>> + Â Â Â Â Â Â Â Â Â Â Âoh->name, t, pwrdm->name, req_oh->name);
>>> + Â Â }
>>> +
>>> + Â Â return pwrdm_wakeuplat_set_constraint(pwrdm, req_dev, t);
>>> +}
>>> +
>>> +/**
>>> Â * omap_hwmod_get_context_loss_count - get lost context count
>>> Â * @oh: struct omap_hwmod *
>>> Â *
>>> - * Query the powerdomain of of @oh to get the context loss
>>> + * Query the powerdomain of @oh to get the context loss
>>> Â * count for this device.
>>> Â *
>>> Â * Returns the context loss count of the powerdomain assocated with @oh
>>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
>>> index eaed0df..6fb4741 100644
>>> --- a/arch/arm/mach-omap2/powerdomain.c
>>> +++ b/arch/arm/mach-omap2/powerdomain.c
>>> @@ -19,6 +19,8 @@
>>> Â#include <linux/list.h>
>>> Â#include <linux/errno.h>
>>> Â#include <linux/string.h>
>>> +#include <linux/slab.h>
>>> +
>>> Â#include "cm2xxx_3xxx.h"
>>> Â#include "prcm44xx.h"
>>> Â#include "cm44xx.h"
>>> @@ -103,6 +105,13 @@ static int _pwrdm_register(struct powerdomain *pwrdm)
>>> Â Â Â pwrdm->state = pwrdm_read_pwrst(pwrdm);
>>> Â Â Â pwrdm->state_counter[pwrdm->state] = 1;
>>>
>>> + Â Â /* Initialize priority ordered list for wakeup latency constraint */
>>> + Â Â spin_lock_init(&pwrdm->wakeuplat_lock);
>>> + Â Â plist_head_init(&pwrdm->wakeuplat_dev_list, &pwrdm->wakeuplat_lock);
>>> +
>>> + Â Â /* res_mutex protects res_list add and del ops */
>>> + Â Â mutex_init(&pwrdm->wakeuplat_mutex);
>>> +
>>> Â Â Â pr_debug("powerdomain: registered %s\n", pwrdm->name);
>>>
>>> Â Â Â return 0;
>>> @@ -176,6 +185,74 @@ 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.
>>> + *
>>> + * Finds the minimum allowed wake-up latency value from all entries
>>> + * in the list and the power domain power state needing the constraint.
>>> + * Programs a new target state if it is different from current power state.
>>> + *
>>> + * Only OMAP3xxx is supported for now
>>> + *
>>> + * Returns 0 upon success.
>>> + */
>>> +static int pwrdm_wakeuplat_update_pwrst(struct powerdomain *pwrdm)
>>> +{
>>> + Â Â struct plist_node *node;
>>> + Â Â int ret = 0, new_state;
>>> + Â Â long min_latency = -1;
>>> +
>>> + Â Â /* Find the strongest constraint from the plist */
>>> + Â Â if (!plist_head_empty(&pwrdm->wakeuplat_dev_list)) {
>>> + Â Â Â Â Â Â node = plist_first(&pwrdm->wakeuplat_dev_list);
>>> + Â Â Â Â Â Â min_latency = node->prio;
>>> + Â Â }
>>> +
>>> + Â Â /* Find power state with wakeup latency < minimum constraint. */
>>> + Â Â for (new_state = 0x0; new_state < PWRDM_MAX_PWRSTS; new_state++) {
>>> + Â Â Â Â Â Â if (min_latency == -1 ||
>>> + Â Â Â Â Â Â Â Â 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:
>>> + Â Â Â Â Â Â if (cpu_is_omap34xx())
>>> + Â Â Â Â Â Â Â Â Â Â pwrdm_set_logic_retst(pwrdm, PWRDM_POWER_OFF);
>>
>> cpu_is_* checks here aren't right.
>>
>> You should use SoC specific function pointers as are done for many of the
>> other powerdomain calls after Rajendra's splitup series.
> The cpu_is_* tests are not related to the SoC specific function
> pointers but rather to the comment in the function description 'Only
> OMAP3xxx is supported for now'.
> In fact the function that are conditionally called
> (pwrdm_set_logic_retst and omap_set_pwrdm_state) are correctly using
> the SoC specific pointers.
>
> Is this test needed? If so is it the correct way to do it?
Right, I think you can just drop the cpu_is_* checks here since as you
pointed out, the called functions already handle SoC specifics.
>>
>>> + Â Â Â Â Â Â new_state = PWRDM_POWER_RET;
>>> + Â Â Â Â Â Â break;
>>> + Â Â case PWRDM_FUNC_PWRST_CSWR:
>>> + Â Â Â Â Â Â if (cpu_is_omap34xx())
>>> + Â Â Â Â Â Â Â Â Â Â pwrdm_set_logic_retst(pwrdm, PWRDM_POWER_RET);
>>> + Â Â Â Â Â Â new_state = PWRDM_POWER_RET;
>>> + Â Â Â Â Â Â 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_pwrst(pwrdm) != new_state) {
>>> + Â Â Â Â Â Â if (cpu_is_omap34xx())
>>> + Â Â Â Â Â Â Â Â Â Â 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;
>>> +}
>>> +
>>
>> [...]
>>
>>> diff --git a/arch/arm/mach-omap2/powerdomains3xxx_data.c b/arch/arm/mach-omap2/powerdomains3xxx_data.c
>>> index e1bec56..4f7e44d 100644
>>> --- a/arch/arm/mach-omap2/powerdomains3xxx_data.c
>>> +++ b/arch/arm/mach-omap2/powerdomains3xxx_data.c
>>> @@ -52,6 +52,12 @@ static struct powerdomain iva2_pwrdm = {
>>> Â Â Â Â Â Â Â [2] = PWRSTS_OFF_ON,
>>> Â Â Â Â Â Â Â [3] = PWRDM_POWER_ON,
>>> Â Â Â },
>>> + Â Â .wakeup_lat = {
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OFF] = 1100,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_CSWR] = 350,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_ON] = 0,
>>> + Â Â },
>>> Â};
>>>
>>> Âstatic struct powerdomain mpu_3xxx_pwrdm = {
>>> @@ -68,6 +74,12 @@ static struct powerdomain mpu_3xxx_pwrdm = {
>>>    .pwrsts_mem_on  Â= {
>>> Â Â Â Â Â Â Â [0] = PWRSTS_OFF_ON,
>>> Â Â Â },
>>> + Â Â .wakeup_lat = {
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OFF] = 95,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_CSWR] = 45,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_ON] = 0,
>>> + Â Â },
>>> Â};
>>>
>>> Â/*
>>> @@ -98,6 +110,12 @@ static struct powerdomain core_3xxx_pre_es3_1_pwrdm = {
>>> Â Â Â Â Â Â Â [0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */
>>> Â Â Â Â Â Â Â [1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */
>>> Â Â Â },
>>> + Â Â .wakeup_lat = {
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OFF] = 100,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_CSWR] = 60,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_ON] = 0,
>>> + Â Â },
>>> Â};
>>>
>>> Âstatic struct powerdomain core_3xxx_es3_1_pwrdm = {
>>> @@ -121,6 +139,12 @@ static struct powerdomain core_3xxx_es3_1_pwrdm = {
>>> Â Â Â Â Â Â Â [0] = PWRSTS_OFF_RET_ON, /* MEM1ONSTATE */
>>> Â Â Â Â Â Â Â [1] = PWRSTS_OFF_RET_ON, /* MEM2ONSTATE */
>>> Â Â Â },
>>> + Â Â .wakeup_lat = {
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OFF] = 100,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_CSWR] = 60,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_ON] = 0,
>>> + Â Â },
>>> Â};
>>>
>>> Âstatic struct powerdomain dss_pwrdm = {
>>> @@ -136,6 +160,12 @@ static struct powerdomain dss_pwrdm = {
>>>    .pwrsts_mem_on  Â= {
>>> Â Â Â Â Â Â Â [0] = PWRDM_POWER_ON, Â/* MEMONSTATE */
>>> Â Â Â },
>>> + Â Â .wakeup_lat = {
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OFF] = 70,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_CSWR] = 20,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_ON] = 0,
>>> + Â Â },
>>> Â};
>>>
>>> Â/*
>>> @@ -157,6 +187,12 @@ static struct powerdomain sgx_pwrdm = {
>>>    .pwrsts_mem_on  Â= {
>>> Â Â Â Â Â Â Â [0] = PWRDM_POWER_ON, Â/* MEMONSTATE */
>>> Â Â Â },
>>> + Â Â .wakeup_lat = {
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OFF] = 1000,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_CSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_ON] = 0,
>>> + Â Â },
>>> Â};
>>>
>>> Âstatic struct powerdomain cam_pwrdm = {
>>> @@ -172,6 +208,12 @@ static struct powerdomain cam_pwrdm = {
>>>    .pwrsts_mem_on  Â= {
>>> Â Â Â Â Â Â Â [0] = PWRDM_POWER_ON, Â/* MEMONSTATE */
>>> Â Â Â },
>>> + Â Â .wakeup_lat = {
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OFF] = 850,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_CSWR] = 35,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_ON] = 0,
>>> + Â Â },
>>> Â};
>>>
>>> Âstatic struct powerdomain per_pwrdm = {
>>> @@ -187,6 +229,12 @@ static struct powerdomain per_pwrdm = {
>>>    .pwrsts_mem_on  Â= {
>>> Â Â Â Â Â Â Â [0] = PWRDM_POWER_ON, Â/* MEMONSTATE */
>>> Â Â Â },
>>> + Â Â .wakeup_lat = {
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OFF] = 200,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_CSWR] = 110,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_ON] = 0,
>>> + Â Â },
>>> Â};
>>>
>>> Âstatic struct powerdomain emu_pwrdm = {
>>> @@ -201,6 +249,12 @@ static struct powerdomain neon_pwrdm = {
>>>    .omap_chip    Â= OMAP_CHIP_INIT(CHIP_IS_OMAP3430),
>>>    .pwrsts      = PWRSTS_OFF_RET_ON,
>>> Â Â Â .pwrsts_logic_ret = PWRDM_POWER_RET,
>>> + Â Â .wakeup_lat = {
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OFF] = 200,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_CSWR] = 35,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_ON] = 0,
>>> + Â Â },
>>> Â};
>>>
>>> Âstatic struct powerdomain usbhost_pwrdm = {
>>> @@ -223,6 +277,12 @@ static struct powerdomain usbhost_pwrdm = {
>>>    .pwrsts_mem_on  Â= {
>>> Â Â Â Â Â Â Â [0] = PWRDM_POWER_ON, Â/* MEMONSTATE */
>>> Â Â Â },
>>> + Â Â .wakeup_lat = {
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OFF] = 800,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_OSWR] = UNSUP_STATE,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_CSWR] = 150,
>>> + Â Â Â Â Â Â [PWRDM_FUNC_PWRST_ON] = 0,
>>> + Â Â },
>>> Â};
>>
>> A summary about where the latency numbers for each powerdomain come from
>> would be useful.
> Ok
>
>>
>> [...]
>>
>>> @@ -87,64 +60,86 @@ int omap_pm_set_min_bus_tput(struct device *dev, u8 agent_id, unsigned long r)
>>> Â Â Â return 0;
>>> Â}
>>>
>>> +/*
>>> + * omap_pm_set_max_dev_wakeup_lat - set/release devices wake-up latency
>>> + * constraints
>>> + */
>>> Âint omap_pm_set_max_dev_wakeup_lat(struct device *req_dev, struct device *dev,
>>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âlong t)
>>> Â{
>>> + Â Â struct platform_device *pdev, *req_pdev;
>>> + Â Â int ret = 0;
>>> +
>>> Â Â Â if (!req_dev || !dev || t < -1) {
>>> Â Â Â Â Â Â Â WARN(1, "OMAP PM: %s: invalid parameter(s)", __func__);
>>> Â Â Â Â Â Â Â return -EINVAL;
>>> + Â Â }
>>> +
>>> + Â Â /* Look for the platform devices */
>>> + Â Â pdev = to_platform_device(dev);
>>> + Â Â req_pdev = to_platform_device(req_dev);
>>> +
>>> + Â Â /* Try to catch non platform devices. */
>>> + Â Â if ((pdev->name == NULL) || (req_pdev->name == NULL)) {
>>> + Â Â Â Â Â Â pr_err("OMAP-PM set_wakeup_lat: Error: platform devices "
>>> + Â Â Â Â Â Â Â Â Â Â"not valid\n");
>>> + Â Â Â Â Â Â return -EINVAL;
>>> + Â Â } else {
>>> + Â Â Â Â Â Â /* Call the omap_device API */
>>> + Â Â Â Â Â Â ret = omap_device_set_max_dev_wakeup_lat(req_pdev, pdev, t);
>>> + Â Â }
>>
>> I don't think a NULL name check is the right sanity check here. ÂWHat
>> you really need to know is whether the target device is an omap_device.
>> The requesting device can be anything (I think.)
>>
>> Here's a simpler check:
>>
>> Â Â Â Âif (pdev->dev->parent == &omap_device_parent)
>> Â Â Â Â Â Â Â Âret = omap_device_set_max_dev_wakeup_lat(req_pdev, pdev, t);
>> Â Â Â Âelse
>> Â Â Â Â Â Â Â Â...
> Ok I will use this test. Note: the dev parameter is already checked
> for NULL above.
>
> Also I think the requester parameter needs to keep the device* type
> across all layers. In any case only this type is used by the low level
> code (in powerdomain.c). This allows for any device (i.e. that does
> not necessarily have an associated omap_device) to request
> constraints.
> What do you think?
I agree that the only the target device needs to be an omap_device, the
requester can be any device.
Kevin
--
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
