[PATCH 2/6] ARM: OMAP2+: PM: introduce power domains functional states

[Jean Pihet]

Hi Santosh,

On Thu, May 17, 2012 at 12:04 PM, Santosh Shilimkar
<santosh.shilimkar at ti.com> wrote:
> Jean,
> On Tuesday 08 May 2012 02:10 PM, Jean Pihet wrote:
>> Paul,
>>
>> On Mon, May 7, 2012 at 11:28 AM, Paul Walmsley <paul at pwsan.com> wrote:
>>> Hi
>>>
>>> On Wed, 18 Apr 2012, jean.pihet at newoldbits.com wrote:
>>>
>>>> From: Jean Pihet <j-pihet at ti.com>
>>>>
>>>> Introduce functional (or logical) states for power domains and the
>>>> API functions to read the power domains settings and to convert
>>>> between the functional (i.e. logical) and the internal (or registers)
>>>> values.
>>>>
>>>> OMAP2, OMAP3 and OMAP4 platforms are defining a conversion routine.
>>>>
>>>> In the new API the function omap_set_pwrdm_state takes the functional
>>>> states as parameter; while at it the function is moved to the power
>>>> domains code.
>>>>
>>>> The memory and logic states are not using the functional states, this
>>>> comes as a subsequent patch.
>>>>
>>>> Signed-off-by: Jean Pihet <j-pihet at ti.com>
>>>
>>> This patch results in several checkpatch warnings; please resolve them.
>> Oops. Will check and update.
>>
>>>
>>>> ---
>>>>  arch/arm/mach-omap2/pm.c                   |   66 -----------
>>>>  arch/arm/mach-omap2/powerdomain-common.c   |   61 ++++++++++
>>>>  arch/arm/mach-omap2/powerdomain.c          |  175 ++++++++++++++++++++++++++++
>>>>  arch/arm/mach-omap2/powerdomain.h          |   21 ++++
>>>>  arch/arm/mach-omap2/powerdomain2xxx_3xxx.c |    5 +
>>>>  arch/arm/mach-omap2/powerdomain44xx.c      |    2 +
>>>>  6 files changed, 264 insertions(+), 66 deletions(-)
>>>>
>> ...
>>
>>>> +/*
>>>> + * Functional (i.e. logical) to internal (i.e. registers)
>>>> + * values for the power domains states
>>>> + */
>>>
>>> Please use kerneldoc-style function comments.
>> Ok
>>
>>>
>>>> +int omap2_pwrdm_func_to_pwrst(struct powerdomain *pwrdm, u8 func_pwrst)
>>>> +{
>>>> +     int ret;
>>>> +
>>>> +     switch (func_pwrst) {
>>>> +     case PWRDM_FUNC_PWRST_ON:
>>>> +             ret = PWRDM_POWER_ON;
>>>> +             break;
>>>> +     case PWRDM_FUNC_PWRST_INACTIVE:
>>>> +             ret = PWRDM_POWER_INACTIVE;
>>>> +             break;
>>>> +     case PWRDM_FUNC_PWRST_CSWR:
>>>> +     case PWRDM_FUNC_PWRST_OSWR:
>>>> +             ret = PWRDM_POWER_RET;
>>>> +             break;
>>>> +     case PWRDM_FUNC_PWRST_OFF:
>>>> +             ret = PWRDM_POWER_OFF;
>>>> +             break;
>>>> +     default:
>>>> +             ret = -1;
>>>> +     }
>>>> +
>>>> +     return ret;
>>>> +}
>>>
>>> At a quick glance, I don't think that this function is appropriate for all
>>> OMAP2+ chips.  For example, off-mode is not supported in our OMAP2xxx
>>> kernels.  And OMAP2xxx/3xxx do not support the INACTIVE powerstate.  So
>>> probably this function should differ by chip, and be located in the
>>> powerdomain[2-4]*xx*.c files.
>> I hope to make this function as generic as possible, hence the
>> location (powerdomain-common.c). Some states are not programmed by the
>> pwrdm_* functions since there forbidden by the contents of the
>> pwrdm->pwrst field.
>> Now if the need arises some platform specific function can be defined
>> for conversion functions since the pwrdm->ops function pointers are
>> used. I do not think it is needed now but it can easily be changed.
>>
>>> Also, what about the logic and memory bank power states?  Shouldn't this
>>> function pass those back as well?
>> Cf. in the description "The memory and logic states are not using the
>> functional states, this comes as a subsequent patch."
>>
> I don't see much difference between the functional power states to
> actual power states with some cases here and there.
>
> +     case PWRDM_FUNC_PWRST_CSWR:
> +     case PWRDM_FUNC_PWRST_OSWR:
> +             ret = PWRDM_POWER_RET;
The whole patch series takes the logic state into account in the
functional power state.
Cf. in the description "The memory and logic states are not using the
functional states, this comes as a subsequent patch."

> This is not true and even if you add logic/memmort etc support,
> I still think, we are just duplicating stuff between functional
> vs actual power state.
The idea is to provide a single function to program the power domains
state, including the logic state.
There is no duplication of code, just a simplification in the API. IMO
there are too much calls to set and get the power domain states (power
state, logical etc.) and most of the calls do not take the logical
state into account.
If you look the the code in pmxxxx.c and cpuidlexxxx.c you will notice
that the code is simplified.

> If the idea is to have one single interface to program the power
> domain states, so that Generic frameworks, or PM code movement to
> driver/* directories is easier, we may want think about alternative
> which is scalable.
>
> Just a wild thought...
>
> May be we can abstract all the necessary/used power domain programming
> entities inside a structure and export that. That should contain, power
> state, logic state, mem state, special quirk flags like SAR etc.
> being structure is always expandable and an API taking structure as
> a parameter need not change.
That is the goal. This patch series uses the date from the platform
specific power domain structs and abstracts those in the API.

So in summary I would say that the power domain code callers do
benefit from the API simplification and the abstraction of thr power
domains parameters:
- cpuidle: the logic state field is removed from the structs; the
power domain state checks are simplified,
- per-device PM QoS: the latency parameters are specified for every
functional power state,
- pmxxxx.c: the suspend/resume code is simplified; the power domain
state checks are simplified.

>
> Regards
> Santosh
>

What do you think?

Thanks,
Jean