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

[Shilimkar, Santosh]

Jean,

On Mon, May 21, 2012 at 7:23 PM, Jean Pihet <jean.pihet at newoldbits.com> wrote:
> 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."
>
Yeah. i understood that from comments.

>> 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.
>
I noticed that but thought that simplification is not substantial.

>> 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.
>
Good to know that we are on same page.

> 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:
I agree. I am just worried about this 1 API because we have many used
programmable nobs. For example the API assumes all the power domain
same and while programming the powerdomain deeper state, does
clockdomain wakeup and sleep transition sequence. Such a process
is not needed for domain like CPU/MPU PD's because they are
powerdown them self.

> - 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.
>
QOS does need functional power state especially since CSWR and OSWR
has such a huge difference in terms of latency.

>>
>> Regards
>> Santosh
>>
>
> What do you think?
>
I like the idea as mentioned. I just think the series should already
take care of memory state, logic state and quirk flags to see how
it looks overall. Will have one more fresh look at the series but if
you have subsequent WIP patches which can handle the other
parameters, please send that link.

Regards
Santosh