Re: [PATCHv6 9/9] OMAP3: PM: Added support for suspending to INACTIVE state

[Kevin Hilman <khilman@deeprootsystems.com>]

<Tero.Kristo@nokia.com> writes:

[...]

> True, ancient info there. OFF for example has been supported for ages already.
>
>>
>>
>>> +	if (state != PWRDM_POWER_INACTIVE)
>>> +		while (!(pwrdm->pwrsts & (1 << state))) {
>>> +			if (state == PWRDM_POWER_OFF)
>>> +				return ret;
>>> +			state--;
>>> +		}
>>
>>I think all powerdomains can be inactive right?
>
> Yes.
>
>>I think it would be cleaner to just have all the pwrdm->pwrsts fields
>>include intactive as a valid option.
>>
>>Something like the patch below.  IIRC, you did something like this in
>>one of the earlier versions of the patch.
>
> Yeah, something like this was done previously, however Paul did not
> like the idea of changing the generic powerdomain code too much so I
> dropped it completely. It is now done only via the support functions
> in patch #1, and only done for the powerdomains that actually need
> it for the cpuidle (mpu/core/neon.) It would be possible to add
> support for the rest of the powerdomains also, but I decided to drop
> this in favor of getting the patch set in.

I'm not proposing changing any of the other powerdomain code.  Just
changing the PWRSTS_* defines, essentially so that INACTIVE is
a valid state.

That will eliminate the need for a special check for inactive in this
patch.

Kevin

>>
>>diff --git a/arch/arm/plat-omap/include/plat/powerdomain.h 
>>b/arch/arm/plat-omap/include/plat/powerdomain.h
>>index a1ecd47..c692472 100644
>>--- a/arch/arm/plat-omap/include/plat/powerdomain.h
>>+++ b/arch/arm/plat-omap/include/plat/powerdomain.h
>>@@ -31,17 +31,17 @@
>> #define PWRDM_MAX_PWRSTS	4
>> 
>> /* Powerdomain allowable state bitfields */
>>-#define PWRSTS_OFF_ON		((1 << PWRDM_POWER_OFF) | \
>>+#define PWRSTS_ON		((1 << PWRDM_POWER_INACTIVE) | \
>> 				 (1 << PWRDM_POWER_ON))
>> 
>>+#define PWRSTS_OFF_ON		((1 << PWRDM_POWER_OFF) | PWRSTS_ON)
>>+
>> #define PWRSTS_OFF_RET		((1 << PWRDM_POWER_OFF) | \
>> 				 (1 << PWRDM_POWER_RET))
>> 
>>-#define PWRSTS_RET_ON		((1 << PWRDM_POWER_RET) | \
>>-				 (1 << PWRDM_POWER_ON))
>>-
>>-#define PWRSTS_OFF_RET_ON	(PWRSTS_OFF_RET | (1 << PWRDM_POWER_ON))
>>+#define PWRSTS_RET_ON		((1 << PWRDM_POWER_RET) | PWRSTS_ON)
>> 
>>+#define PWRSTS_OFF_RET_ON	(PWRSTS_OFF_RET | PWRSTS_ON)
>> 
>> /* Powerdomain flags */
>> #define PWRDM_HAS_HDWR_SAR	(1 << 0) /* hardware 
>>save-and-restore support */
>>