Paul,
On Mon, May 7, 2012 at 11:28 AM, Paul Walmsley <paul@xxxxxxxxx> wrote:
> Hi
>
> On Wed, 18 Apr 2012, jean.pihet@xxxxxxxxxxxxxx wrote:
>
>> From: Jean Pihet <j-pihet@xxxxxx>
>>
>> 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@xxxxxx>
>
> 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."
>
>> +
>> +/*
>> + * Internal (i.e. registers) to functional (i.e. logical) values
>> + * for the power domains states
>> + */
>
> Please use kerneldoc-style function documentation.
Ok
>
>> +int omap2_pwrdm_pwrst_to_func(struct powerdomain *pwrdm, u8 pwrst)
>
> Probably best to use "func_pwrst" or "fpwrst" rather than simply "func"
> here.
Ok. I am for 'omap2_pwrdm_pwrst_to_fpwrst(...)'.
>
>> +{
>> + int ret;
>> +
>> + switch (pwrst) {
>> + case PWRDM_POWER_ON:
>> + ret = PWRDM_FUNC_PWRST_ON;
>> + break;
>> + case PWRDM_POWER_INACTIVE:
>> + ret = PWRDM_FUNC_PWRST_INACTIVE;
>> + break;
>> + case PWRDM_POWER_RET:
>> + /*
>> + * XXX warning: return OSWR in case of pd in RET and
>> + * logic in OFF
>> + */
>> + ret = PWRDM_FUNC_PWRST_CSWR;
>> + break;
>> + case PWRDM_POWER_OFF:
>> + ret = PWRDM_FUNC_PWRST_OFF;
>> + break;
>> + default:
>> + ret = -1;
>> + }
>> +
>> + return ret;
>> +}
>
> This function also needs to check the bank power states and logic power
> states to determine what the actual functional powerstate is.
Same remark as above + there is a comment about it in the code ("XXX warning").
>> +
>> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
>> index 319b277..718fa43 100644
>> --- a/arch/arm/mach-omap2/powerdomain.c
>> +++ b/arch/arm/mach-omap2/powerdomain.c
>> @@ -466,6 +466,136 @@ int pwrdm_get_mem_bank_count(struct powerdomain *pwrdm)
>> }
>>
>> /**
>> + * pwrdm_func_to_pwrst - get the internal (i.e. registers) value mapped
>> + * to the functional state
>
> Probably best to use "func_pwrst" or "fpwrst" rather than simply "func"
> here.
Ok
>
>> + * @pwrdm: struct powerdomain * to query
>> + * @func_pwrst: functional power state
>> + *
>> + * Convert the functional power state to the platform specific power
>> + * domain state value.
>> + * Returns the internal power domain state value or -EINVAL in case
>> + * of invalid parameters passed in.
>> + */
>> +int pwrdm_func_to_pwrst(struct powerdomain *pwrdm, u8 func_pwrst)
>> +{
>> + int ret = func_pwrst;
>> +
>> + if ((!pwrdm)|| (func_pwrst >= PWRDM_MAX_FUNC_PWRSTS))
>> + return -EINVAL;
>> +
>> + pr_debug("powerdomain: convert %0x func to pwrst for %s\n",
>> + func_pwrst, pwrdm->name);
>> +
>> + if (arch_pwrdm && arch_pwrdm->pwrdm_func_to_pwrst) {
>> + ret = arch_pwrdm->pwrdm_func_to_pwrst(pwrdm, func_pwrst);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/**
>> + * pwrdm_pwrst_to_func - get the functional (i.e. logical) value mapped
>> + * to the internal state
>> + * @pwrdm: struct powerdomain * to query
>> + * @pwrst: internal power state
>> + *
>> + * Convert the internal power state to the power domain functional value.
>> + * Returns the functional power domain state value or -EINVAL in case
>> + * of invalid parameters passed in.
>> + */
>> +int pwrdm_pwrst_to_func(struct powerdomain *pwrdm, u8 pwrst)
>> +{
>> + int ret = pwrst;
>> +
>> + if ((!pwrdm)|| (pwrst >= PWRDM_MAX_PWRSTS))
>> + return -EINVAL;
>> +
>> + pr_debug("powerdomain: convert %0x pwrst to func for %s\n",
>> + pwrst, pwrdm->name);
>> +
>> + if (arch_pwrdm && arch_pwrdm->pwrdm_pwrst_to_func) {
>> + ret = arch_pwrdm->pwrdm_pwrst_to_func(pwrdm, pwrst);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +/* Types of sleep_switch used in omap_set_pwrdm_state */
>> +#define FORCEWAKEUP_SWITCH 0
>> +#define LOWPOWERSTATE_SWITCH 1
>> +
>> +/**
>> + * omap_set_pwrdm_state - program next powerdomain power state
>> + * @pwrdm: struct powerdomain * to set
>> + * @func_pwrst: power domain functional state, one of the PWRDM_FUNC_* macros
>> + *
>> + * This programs the pwrdm next functional state, sets the dependencies
>> + * and waits for the state to be applied.
>> + */
>> +int omap_set_pwrdm_state(struct powerdomain *pwrdm, u32 func_pwrst)
>
> This should presumably get renamed to match the other function names in
> this file, to be something like pwrdm_set_func_pwrst() or something
> similar.
Ok that makes sense.
...
>
>
> - Paul
Thanks,
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
