[PATCH 0/5] ARM: OMAP2+: PM: implement a caching mechanism on the power domains state registers

[PATCH 0/5] ARM: OMAP2+: PM: implement a caching mechanism on the power domains state registers

[jean.pihet at newoldbits.com]

From: Jean Pihet <j-pihet@ti.com>

The OMAP3 PRCM registers accesses are known to be slow, with a PRCM register
read taking up to 12-14us depending on the OPP.

This patch adds a caching mechanism on the power domains state registers.
When the cache is cold or has been invalidated a register access is
performed, otherwise the register value is retrieved from the registers
cache.
The API is made of read and write functions for fields in the cache, as well
as an invalidate and helper functions to invalidate parts of the cache
contents (i.e. previous, current power states and all fields in the cache).
The power domain code is converted to use the API to read and write the
previous, current and next states for the power domains states, logical
and memory states.
The PM debug code also uses the caching API instead of the internal
pwrdm->state variable.

Using the caching mechanism optimizes the performance of the system in the
transitions to and from the low power states.

Tested on Beagleboard using cpuidle in RET and OFF modes.

Based on the linux-omap git tree (3.4.0-rc2) [1] with the changes for the
functional power states [2] and the per-device PM QoS support for OMAP [3].

[1] git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git
[2] http://marc.info/?l=linux-omap&m=133475291911194&w=2
[3] http://marc.info/?l=linux-omap&m=133475685213067&w=2


Jean Pihet (5):
  ARM: OMAP2+: PM: implement a caching mechanism on the power domains
    state registers
  ARM: OMAP2+: PM: use the power domains registers cache for the power
    states
  ARM: OMAP2+: PM: use the power domains registers cache for the logic
    and mem states
  ARM: OMAP2+: PM: use the power domains registers cache invalidate API
  ARM: OMAP2+: PM debug: use the power domains registers caching API

 arch/arm/mach-omap2/pm-debug.c    |   16 ++-
 arch/arm/mach-omap2/pm34xx.c      |    6 +
 arch/arm/mach-omap2/powerdomain.c |  272 ++++++++++++++++++++++++++++++++-----
 arch/arm/mach-omap2/powerdomain.h |   45 ++++++-
 4 files changed, 297 insertions(+), 42 deletions(-)

-- 
1.7.7.6

[PATCH 1/5] ARM: OMAP2+: PM: implement a caching mechanism on the power domains state registers

[jean.pihet at newoldbits.com]

From: Jean Pihet <j-pihet@ti.com>

The OMAP3 PRCM registers accesses are known to be slow. A PRCM read
can take up to 12-14us depending on the OPP.

This patch adds a caching mechanism on the power domains state registers.
When the cache is cold or has been invalidated a register access is
performed, otherwise the register value is retrieved from the registers
cache.
The API is made of read and write functions for fields in the cache, as well
as an invalidate function and helper functions to invalidate parts of the
cache contents (i.e. previous, current power states and all fields in the
cache).

Signed-off-by: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/mach-omap2/powerdomain.c |  128 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-omap2/powerdomain.h |   45 ++++++++++++-
 2 files changed, 171 insertions(+), 2 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 100c422..18e1ffc 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -697,6 +697,134 @@ out:
 }
 
 /**
+ * pwrdm_cache_read - read from the registers cache
+ * @pwrdm: struct powerdomain *
+ * @index: index of the data to be read from the cache
+ * @value: data value to be returned
+ *
+ * Returns 0 if the data is read from the cache.
+ * Returns -ENODATA if the data is not present, -EINVAL if the data is
+ * to be read past the end of the cache.
+ */
+static int pwrdm_cache_read(struct powerdomain *pwrdm, int index, int *value)
+{
+	if (index >= PWRDM_CACHE_SIZE)
+		return -EINVAL;
+
+	if (!(pwrdm->cache_state & (1 << index)))
+		return -ENODATA;
+
+	*value = pwrdm->cache[index];
+	return 0;
+}
+
+/**
+ * pwrdm_cache_write - store data to the registers cache
+ * @pwrdm: struct powerdomain *
+ * @index: index of the data to write to the cache
+ * @value: data value to be stored
+ *
+ * Stores the data in the cache and maintains the internal cache state.
+ *
+ * Returns -EINVAL if the data is to be written past the end of the cache,
+ * 0 otherwise.
+ */
+static int pwrdm_cache_write(struct powerdomain *pwrdm, int index, int value)
+{
+	if (index >= PWRDM_CACHE_SIZE)
+		return -EINVAL;
+
+	pwrdm->cache[index] = value;
+	pwrdm->cache_state |= (1 << index);
+
+	return 0;
+}
+
+/**
+ * pwrdm_invalidate_regs_cache_fields - invalidate fields in the
+ * power domain registers cache
+ * @pwrdm: struct powerdomain *
+ * @mask: cache fields to invalidate, made by OR'ing the
+ * PWRDM_CACHE_* values
+ *
+ * If pwrdm is NULL, invalidate the fields for all power domains,
+ * otherwise invalidate them for the given pwrdm only.
+ *
+ * Invalidates some of the power domain cache fields, thus imposing
+ * an effective re-read from the HW register@the next access.
+ */
+void pwrdm_invalidate_regs_cache_fields(struct powerdomain *pwrdm, int mask)
+{
+	struct powerdomain *temp_pwrdm;
+
+	if (pwrdm) {
+		pwrdm->cache_state &= ~mask;
+	} else {
+		list_for_each_entry(temp_pwrdm, &pwrdm_list, node)
+			temp_pwrdm->cache_state &= ~mask;
+	}
+}
+
+/**
+ * pwrdm_invalidate_regs_cache_fields_prev - invalidate the previous states
+ * in the power domain registers cache
+ * @pwrdm: struct powerdomain *
+ *
+ * If pwrdm is NULL, invalidate the previous states for all power domains,
+ * otherwise invalidate them for the given pwrdm only.
+ *
+ * Invalidates only the previous states of the power domain cache fields,
+ * thus imposing an effective re-read from the HW register at the next access.
+ */
+void pwrdm_invalidate_regs_cache_fields_prev(struct powerdomain *pwrdm)
+{
+	int i;
+
+	pwrdm_invalidate_regs_cache_fields(pwrdm,
+				PWRDM_CACHE_PREV_PWRST |
+				PWRDM_CACHE_PREV_LOGIC_PWRST);
+	for (i = 0; i < PWRDM_MAX_MEM_BANKS; i++)
+		pwrdm_invalidate_regs_cache_fields(pwrdm,
+				PWRDM_CACHE_PREV_MEM_PWRST + i);
+}
+
+/**
+ * pwrdm_invalidate_regs_cache_fields_current - invalidate the current
+ * states in the power domains' registers cache
+ *
+ * If pwrdm is NULL, invalidate the current states for all power domains,
+ * otherwise invalidate them for the given pwrdm only.
+ *
+ * Invalidates only the current states of the power domains cache fields,
+ * thus imposing an effective re-read from the HW register at the next access.
+ */
+void pwrdm_invalidate_regs_cache_fields_current(struct powerdomain *pwrdm)
+{
+	int i;
+
+	pwrdm_invalidate_regs_cache_fields(pwrdm, PWRDM_CACHE_PWRST |
+					   PWRDM_CACHE_LOGIC_RETST);
+	for (i = 0; i < PWRDM_MAX_MEM_BANKS; i++) {
+		pwrdm_invalidate_regs_cache_fields(pwrdm,
+					PWRDM_CACHE_MEM_PWRST + i);
+		pwrdm_invalidate_regs_cache_fields(pwrdm,
+					PWRDM_CACHE_MEM_ONST + i);
+	}
+}
+
+/**
+ * pwrdm_invalidate_regs_cache_all - invalidate all fields in the registers
+ * cache for all power domains
+ *
+ * Invalidates all of the power domain cache fields of all power domains,
+ * thus imposing an effective re-read from the HW register at the next access
+ */
+void pwrdm_invalidate_regs_cache_all(void)
+{
+	pwrdm_invalidate_regs_cache_fields(NULL, PWRDM_CACHE_ALL);
+}
+
+/**
  * pwrdm_set_next_pwrst - set next powerdomain power state
  * @pwrdm: struct powerdomain * to set
  * @pwrst: one of the PWRDM_POWER_* macros
diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h
index 63028c0..e5d621c 100644
--- a/arch/arm/mach-omap2/powerdomain.h
+++ b/arch/arm/mach-omap2/powerdomain.h
@@ -93,6 +93,37 @@
 /* XXX A completely arbitrary number. What is reasonable here? */
 #define PWRDM_TRANSITION_BAILOUT 100000
 
+/* Power domains state cache fields */
+enum cache_fields {
+	/* current power state */
+	PWRDM_CACHE_PWRST,
+	/* prev power state */
+	PWRDM_CACHE_PREV_PWRST,
+	/* next power state */
+	PWRDM_CACHE_NEXT_PWRST,
+	/* current logic RET state */
+	PWRDM_CACHE_LOGIC_RETST,
+	/* prev logic power state */
+	PWRDM_CACHE_PREV_LOGIC_PWRST,
+	/* next logic RET state */
+	PWRDM_CACHE_NEXT_LOGIC_RETST,
+	/* current mem bank pwrst, up to PWRDM_MAX_MEM_BANKS banks */
+	PWRDM_CACHE_MEM_PWRST,
+	/* previous mem bank pwrst, up to PWRDM_MAX_MEM_BANKS banks */
+	PWRDM_CACHE_PREV_MEM_PWRST	= PWRDM_CACHE_MEM_PWRST +
+					  PWRDM_MAX_MEM_BANKS,
+	/* next mem bank RET state, up to PWRDM_MAX_MEM_BANKS banks */
+	PWRDM_CACHE_NEXT_MEM_RETST	= PWRDM_CACHE_PREV_MEM_PWRST +
+					  PWRDM_MAX_MEM_BANKS,
+	/* mem ON state, up to PWRDM_MAX_MEM_BANKS banks */
+	PWRDM_CACHE_MEM_ONST		= PWRDM_CACHE_NEXT_MEM_RETST +
+					  PWRDM_MAX_MEM_BANKS,
+	/* keep this item@the end of the list */
+	PWRDM_CACHE_SIZE		= PWRDM_CACHE_MEM_ONST +
+					  PWRDM_MAX_MEM_BANKS
+};
+#define PWRDM_CACHE_ALL		-1
+
 struct clockdomain;
 struct powerdomain;
 
@@ -104,14 +135,17 @@ struct powerdomain;
  * @prcm_partition: (OMAP4 only) the PRCM partition ID containing @prcm_offs
  * @pwrsts: Possible powerdomain power states
  * @pwrsts_logic_ret: Possible logic power states when pwrdm in RETENTION
- * @flags: Powerdomain flags
+ * @flags: Powerdomain capability flags
  * @banks: Number of software-controllable memory banks in this powerdomain
  * @pwrsts_mem_ret: Possible memory bank pwrstates when pwrdm in RETENTION
  * @pwrsts_mem_on: Possible memory bank pwrstates when pwrdm in ON
  * @pwrdm_clkdms: Clockdomains in this powerdomain
  * @node: list_head linking all powerdomains
  * @voltdm_node: list_head linking all powerdomains in a voltagedomain
- * @state:
+ * @state: last power state of the pwrdm, used to detect state transitions
+ * @cache: Power domain registers cache contents
+ * @cache_state: State of the power domains cache. Each bit indicates if
+ * the corresponding data field is available from the cache
  * @state_counter:
  * @timer:
  * @state_timer:
@@ -137,6 +171,8 @@ struct powerdomain {
 	struct list_head voltdm_node;
 	struct mutex lock;
 	int state;
+	int cache[PWRDM_CACHE_SIZE];
+	long cache_state;
 	unsigned state_counter[PWRDM_MAX_FUNC_PWRSTS];
 	unsigned ret_logic_off_counter;
 	unsigned ret_mem_off_counter[PWRDM_MAX_MEM_BANKS];
@@ -233,6 +269,11 @@ int pwrdm_read_prev_func_pwrst(struct powerdomain *pwrdm);
 int pwrdm_read_func_pwrst(struct powerdomain *pwrdm);
 int pwrdm_read_next_func_pwrst(struct powerdomain *pwrdm);
 
+void pwrdm_invalidate_regs_cache_fields(struct powerdomain *pwrdm, int mask);
+void pwrdm_invalidate_regs_cache_fields_prev(struct powerdomain *pwrdm);
+void pwrdm_invalidate_regs_cache_fields_current(struct powerdomain *pwrdm);
+void pwrdm_invalidate_regs_cache_all(void);
+
 int omap2_pwrdm_func_to_pwrst(struct powerdomain *pwrdm, u8 func_pwrst);
 int omap2_pwrdm_func_to_logic_pwrst(struct powerdomain *pwrdm, u8 func_pwrst);
 int omap2_pwrdm_pwrst_to_func(struct powerdomain *pwrdm, u8 pwrst, u8 logic);
-- 
1.7.7.6

[PATCH 2/5] ARM: OMAP2+: PM: use the power domains registers cache for the power states

[jean.pihet at newoldbits.com]

From: Jean Pihet <j-pihet@ti.com>

Use the caching API for the previous, current and next power domains states.

Signed-off-by: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/mach-omap2/powerdomain.c |   32 ++++++++++++++++++++++++++------
 1 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 18e1ffc..2058e27 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -854,6 +854,8 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
 					  smp_processor_id());
 		/* Program the pwrdm desired target state */
 		ret = arch_pwrdm->pwrdm_set_next_pwrst(pwrdm, pwrst);
+		if (!ret)
+			pwrdm_cache_write(pwrdm, PWRDM_CACHE_NEXT_PWRST, pwrst);
 	}
 
 	return ret;
@@ -869,13 +871,19 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
  */
 int pwrdm_read_next_pwrst(struct powerdomain *pwrdm)
 {
-	int ret = -EINVAL;
+	int pwrst, ret = -EINVAL;
 
 	if (!pwrdm)
 		return -EINVAL;
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_read_next_pwrst)
+	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_NEXT_PWRST, &pwrst))
+		return pwrst;
+
+	if (arch_pwrdm && arch_pwrdm->pwrdm_read_next_pwrst) {
 		ret = arch_pwrdm->pwrdm_read_next_pwrst(pwrdm);
+		if (ret >= 0)
+			pwrdm_cache_write(pwrdm, PWRDM_CACHE_NEXT_PWRST, ret);
+	}
 
 	return ret;
 }
@@ -906,13 +914,19 @@ int pwrdm_read_next_func_pwrst(struct powerdomain *pwrdm)
  */
 int pwrdm_read_pwrst(struct powerdomain *pwrdm)
 {
-	int ret = -EINVAL;
+	int pwrst, ret = -EINVAL;
 
 	if (!pwrdm)
 		return -EINVAL;
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_read_pwrst)
+	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PWRST, &pwrst))
+		return pwrst;
+
+	if (arch_pwrdm && arch_pwrdm->pwrdm_read_pwrst) {
 		ret = arch_pwrdm->pwrdm_read_pwrst(pwrdm);
+		if (ret >= 0)
+			pwrdm_cache_write(pwrdm, PWRDM_CACHE_PWRST, ret);
+	}
 
 	return ret;
 }
@@ -943,13 +957,19 @@ int pwrdm_read_func_pwrst(struct powerdomain *pwrdm)
  */
 int pwrdm_read_prev_pwrst(struct powerdomain *pwrdm)
 {
-	int ret = -EINVAL;
+	int pwrst, ret = -EINVAL;
 
 	if (!pwrdm)
 		return -EINVAL;
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_read_prev_pwrst)
+	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PREV_PWRST, &pwrst))
+		return pwrst;
+
+	if (arch_pwrdm && arch_pwrdm->pwrdm_read_prev_pwrst) {
 		ret = arch_pwrdm->pwrdm_read_prev_pwrst(pwrdm);
+		if (ret >= 0)
+			pwrdm_cache_write(pwrdm, PWRDM_CACHE_PREV_PWRST, ret);
+	}
 
 	return ret;
 }
-- 
1.7.7.6

[PATCH 2/5] ARM: OMAP2+: PM: use the power domains registers cache for the power states

[Jon Hunter]

Hi Jean,

On 05/01/2012 08:07 AM, jean.pihet at newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
> 
> Use the caching API for the previous, current and next power domains states.
> 
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
>  arch/arm/mach-omap2/powerdomain.c |   32 ++++++++++++++++++++++++++------
>  1 files changed, 26 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index 18e1ffc..2058e27 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -854,6 +854,8 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
>  					  smp_processor_id());
>  		/* Program the pwrdm desired target state */
>  		ret = arch_pwrdm->pwrdm_set_next_pwrst(pwrdm, pwrst);
> +		if (!ret)
> +			pwrdm_cache_write(pwrdm, PWRDM_CACHE_NEXT_PWRST, pwrst);
>  	}
>  
>  	return ret;
> @@ -869,13 +871,19 @@ int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst)
>   */
>  int pwrdm_read_next_pwrst(struct powerdomain *pwrdm)
>  {
> -	int ret = -EINVAL;
> +	int pwrst, ret = -EINVAL;
>  
>  	if (!pwrdm)
>  		return -EINVAL;
>  
> -	if (arch_pwrdm && arch_pwrdm->pwrdm_read_next_pwrst)
> +	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_NEXT_PWRST, &pwrst))
> +		return pwrst;
> +
> +	if (arch_pwrdm && arch_pwrdm->pwrdm_read_next_pwrst) {
>  		ret = arch_pwrdm->pwrdm_read_next_pwrst(pwrdm);
> +		if (ret >= 0)
> +			pwrdm_cache_write(pwrdm, PWRDM_CACHE_NEXT_PWRST, ret);
> +	}
>  
>  	return ret;
>  }
> @@ -906,13 +914,19 @@ int pwrdm_read_next_func_pwrst(struct powerdomain *pwrdm)
>   */
>  int pwrdm_read_pwrst(struct powerdomain *pwrdm)
>  {
> -	int ret = -EINVAL;
> +	int pwrst, ret = -EINVAL;
>  
>  	if (!pwrdm)
>  		return -EINVAL;
>  
> -	if (arch_pwrdm && arch_pwrdm->pwrdm_read_pwrst)
> +	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PWRST, &pwrst))
> +		return pwrst;
> +
> +	if (arch_pwrdm && arch_pwrdm->pwrdm_read_pwrst) {
>  		ret = arch_pwrdm->pwrdm_read_pwrst(pwrdm);
> +		if (ret >= 0)
> +			pwrdm_cache_write(pwrdm, PWRDM_CACHE_PWRST, ret);
> +	}

Do we really want to use the cache for the current state? This is
updated by hardware. In the above it appears that once we have read it
once we will just return this value until the cache is invalidated.
Makes me a little nervous.

Cheers
Jon

[PATCH 2/5] ARM: OMAP2+: PM: use the power domains registers cache for the power states

[Bedia, Vaibhav]

On Tue, May 01, 2012 at 21:07:39, Hunter, Jon wrote:
[...]
> >  int pwrdm_read_pwrst(struct powerdomain *pwrdm)
> >  {
> > -	int ret = -EINVAL;
> > +	int pwrst, ret = -EINVAL;
> >  
> >  	if (!pwrdm)
> >  		return -EINVAL;
> >  
> > -	if (arch_pwrdm && arch_pwrdm->pwrdm_read_pwrst)
> > +	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PWRST, &pwrst))
> > +		return pwrst;
> > +
> > +	if (arch_pwrdm && arch_pwrdm->pwrdm_read_pwrst) {
> >  		ret = arch_pwrdm->pwrdm_read_pwrst(pwrdm);
> > +		if (ret >= 0)
> > +			pwrdm_cache_write(pwrdm, PWRDM_CACHE_PWRST, ret);
> > +	}
> 
> Do we really want to use the cache for the current state? This is
> updated by hardware. In the above it appears that once we have read it
> once we will just return this value until the cache is invalidated.
> Makes me a little nervous.
> 

I echo the concern here. If a powerdomain transition occurs when h/w conditions
are met, the cached values will go out of sync and this may lead to unexpected
behavior wherever this API is being used.

As Jon pointed out in another patch, the registers which can be updated by h/w should
be handled differently.

Just a wild thought, can PRCM_MPU_IRQ ([4]TRANSITION_EN) be used as a trigger to update
the cache if the transition happened under s/w control?

Regards,
Vaibhav

[PATCH 2/5] ARM: OMAP2+: PM: use the power domains registers cache for the power states

[Jean Pihet]

Hi Jon, Vaibhav,

On Thu, May 3, 2012 at 8:38 AM, Bedia, Vaibhav <vaibhav.bedia@ti.com> wrote:
> On Tue, May 01, 2012 at 21:07:39, Hunter, Jon wrote:
> [...]
>> > ?int pwrdm_read_pwrst(struct powerdomain *pwrdm)
>> > ?{
>> > - ? int ret = -EINVAL;
>> > + ? int pwrst, ret = -EINVAL;
>> >
>> > ? ? if (!pwrdm)
>> > ? ? ? ? ? ? return -EINVAL;
>> >
>> > - ? if (arch_pwrdm && arch_pwrdm->pwrdm_read_pwrst)
>> > + ? if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PWRST, &pwrst))
>> > + ? ? ? ? ? return pwrst;
>> > +
>> > + ? if (arch_pwrdm && arch_pwrdm->pwrdm_read_pwrst) {
>> > ? ? ? ? ? ? ret = arch_pwrdm->pwrdm_read_pwrst(pwrdm);
>> > + ? ? ? ? ? if (ret >= 0)
>> > + ? ? ? ? ? ? ? ? ? pwrdm_cache_write(pwrdm, PWRDM_CACHE_PWRST, ret);
>> > + ? }
>>
>> Do we really want to use the cache for the current state? This is
>> updated by hardware. In the above it appears that once we have read it
>> once we will just return this value until the cache is invalidated.
>> Makes me a little nervous.
I agree with your concerns. The HW can update things in our back and
so there is a risk of having the wrong state reported by the API.
Ideally the registers accesses shall be optimized in the HW itself
(hint for next gen chipsets ;-).

IMO a generic approach on caching is preferred on a hack on the low power code.

The cache implementation is based on the following principles:
1. the registers value are read from the cache if it is 'hot', the
values are read from the registers if the cache has been invalidated
2. the coherency of the cache fields relies on the invalidate of the
fields at the right time during the low power transitions
3. the previous states fields of the cache are invalidated when
clearing the previous power states
4. the current states fields of the cache are invalidated after coming
back from the low power mode, i.e. after the return from the low level
WFI instruction
5. the pwrdm_state_switch function detects the states changes, updates
the last internal state (pwrdm->state) and the states statistics
6. in PM debug any discrepancy between pwrdm->state and the current
state from the API is reported. More checking could be added in PM
debug

So there must be a compromise on which fields are invalidated and
when. I first tried to invalidate the whole cache after every low
power transition, which works fine but does not gain much in
performance. The current invalidate scheme (points 3, 4) is optimized
for performance and has been tested succesfully on OMAP3 (using
cpuidle in RET and OFF).

In any case if the HW asynchronously changes the registers states it
will not always be detected, with or without the cache implementation.
For example if a power domain transitions more than once, only the
last transition -to a different state than pwrdm->state- will be
detected the next time pwrdm_state_switch runs.

>
> I echo the concern here. If a powerdomain transition occurs when h/w conditions
> are met, the cached values will go out of sync and this may lead to unexpected
> behavior wherever this API is being used.
>
> As Jon pointed out in another patch, the registers which can be updated by h/w should
> be handled differently.
Agree but if you do not cache the previous and current registers there
is no point in having the cache in place.

>
> Just a wild thought, can PRCM_MPU_IRQ ([4]TRANSITION_EN) be used as a trigger to update
> the cache if the transition happened under s/w control?
That is a nice suggestion. We need to explore the ways to detect transitions.

>
> Regards,
> Vaibhav

Thanks for your comments!

Regards,
Jean

[PATCH 3/5] ARM: OMAP2+: PM: use the power domains registers cache for the logic and mem states

[jean.pihet at newoldbits.com]

From: Jean Pihet <j-pihet@ti.com>

Use the caching API for the previous, current and next
power domains logical and memory states.

Signed-off-by: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/mach-omap2/powerdomain.c |  108 ++++++++++++++++++++++++++++---------
 1 files changed, 82 insertions(+), 26 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 2058e27..9800b2b 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -1011,11 +1011,15 @@ int pwrdm_set_logic_retst(struct powerdomain *pwrdm, u8 pwrst)
 	if (!(pwrdm->pwrsts_logic_ret & (1 << pwrst)))
 		return -EINVAL;
 
-	pr_debug("powerdomain: setting next logic powerstate for %s to %0x\n",
+	pr_debug("powerdomain: setting next logic RET state for %s to %0x\n",
 		 pwrdm->name, pwrst);
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_set_logic_retst)
+	if (arch_pwrdm && arch_pwrdm->pwrdm_set_logic_retst) {
 		ret = arch_pwrdm->pwrdm_set_logic_retst(pwrdm, pwrst);
+		if (!ret)
+			pwrdm_cache_write(pwrdm, PWRDM_CACHE_NEXT_LOGIC_RETST,
+					  pwrst);
+	}
 
 	return ret;
 }
@@ -1023,13 +1027,13 @@ int pwrdm_set_logic_retst(struct powerdomain *pwrdm, u8 pwrst)
 /**
  * pwrdm_set_mem_onst - set memory power state while powerdomain ON
  * @pwrdm: struct powerdomain * to set
- * @bank: memory bank number to set (0-3)
+ * @bank: memory bank number to set (0-PWRDM_MAX_MEM_BANKS)
  * @pwrst: one of the PWRDM_LOGIC_MEM_PWRST_* macros
  *
  * Set the next power state @pwrst that memory bank @bank of the
  * powerdomain @pwrdm will enter when the powerdomain enters the ON
- * state.  @bank will be a number from 0 to 3, and represents different
- * types of memory, depending on the powerdomain.  Returns -EINVAL if
+ * state.  @bank represents different types of memory, depending on
+ * the powerdomain.  Returns -EINVAL if
  * the powerdomain pointer is null or the target power state is not
  * not supported for this memory bank, -EEXIST if the target memory
  * bank does not exist or is not controllable, or returns 0 upon
@@ -1051,8 +1055,12 @@ int pwrdm_set_mem_onst(struct powerdomain *pwrdm, u8 bank, u8 pwrst)
 	pr_debug("powerdomain: setting next memory powerstate for domain %s "
 		 "bank %0x while pwrdm-ON to %0x\n", pwrdm->name, bank, pwrst);
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_set_mem_onst)
+	if (arch_pwrdm && arch_pwrdm->pwrdm_set_mem_onst) {
 		ret = arch_pwrdm->pwrdm_set_mem_onst(pwrdm, bank, pwrst);
+		if (!ret)
+			pwrdm_cache_write(pwrdm, PWRDM_CACHE_MEM_ONST + bank,
+					  pwrst);
+	}
 
 	return ret;
 }
@@ -1060,13 +1068,13 @@ int pwrdm_set_mem_onst(struct powerdomain *pwrdm, u8 bank, u8 pwrst)
 /**
  * pwrdm_set_mem_retst - set memory power state while powerdomain in RET
  * @pwrdm: struct powerdomain * to set
- * @bank: memory bank number to set (0-3)
+ * @bank: memory bank number to set (0-PWRDM_MAX_MEM_BANKS)
  * @pwrst: one of the PWRDM_LOGIC_MEM_PWRST_* macros
  *
  * Set the next power state @pwrst that memory bank @bank of the
  * powerdomain @pwrdm will enter when the powerdomain enters the
- * RETENTION state.  Bank will be a number from 0 to 3, and represents
- * different types of memory, depending on the powerdomain.  @pwrst
+ * RETENTION state.  Bank represents different types of memory,
+ * depending on the powerdomain.  @pwrst
  * will be either RETENTION or OFF, if supported.  Returns -EINVAL if
  * the powerdomain pointer is null or the target power state is not
  * not supported for this memory bank, -EEXIST if the target memory
@@ -1089,8 +1097,13 @@ int pwrdm_set_mem_retst(struct powerdomain *pwrdm, u8 bank, u8 pwrst)
 	pr_debug("powerdomain: setting next memory powerstate for domain %s "
 		 "bank %0x while pwrdm-RET to %0x\n", pwrdm->name, bank, pwrst);
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_set_mem_retst)
+	if (arch_pwrdm && arch_pwrdm->pwrdm_set_mem_retst) {
 		ret = arch_pwrdm->pwrdm_set_mem_retst(pwrdm, bank, pwrst);
+		if (!ret)
+			pwrdm_cache_write(pwrdm,
+					  PWRDM_CACHE_NEXT_MEM_RETST + bank,
+					  pwrst);
+	}
 
 	return ret;
 }
@@ -1106,13 +1119,19 @@ int pwrdm_set_mem_retst(struct powerdomain *pwrdm, u8 bank, u8 pwrst)
  */
 int pwrdm_read_logic_pwrst(struct powerdomain *pwrdm)
 {
-	int ret = -EINVAL;
+	int retst, ret = -EINVAL;
 
 	if (!pwrdm)
 		return -EINVAL;
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_read_logic_pwrst)
+	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_LOGIC_RETST, &retst))
+		return retst;
+
+	if (arch_pwrdm && arch_pwrdm->pwrdm_read_logic_pwrst) {
 		ret = arch_pwrdm->pwrdm_read_logic_pwrst(pwrdm);
+		if (ret >= 0)
+			pwrdm_cache_write(pwrdm, PWRDM_CACHE_LOGIC_RETST, ret);
+	}
 
 	return ret;
 }
@@ -1127,13 +1146,20 @@ int pwrdm_read_logic_pwrst(struct powerdomain *pwrdm)
  */
 int pwrdm_read_prev_logic_pwrst(struct powerdomain *pwrdm)
 {
-	int ret = -EINVAL;
+	int retst, ret = -EINVAL;
 
 	if (!pwrdm)
 		return -EINVAL;
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_read_prev_logic_pwrst)
+	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PREV_LOGIC_PWRST, &retst))
+		return retst;
+
+	if (arch_pwrdm && arch_pwrdm->pwrdm_read_prev_logic_pwrst) {
 		ret = arch_pwrdm->pwrdm_read_prev_logic_pwrst(pwrdm);
+		if (ret >= 0)
+			pwrdm_cache_write(pwrdm, PWRDM_CACHE_PREV_LOGIC_PWRST,
+					  ret);
+	}
 
 	return ret;
 }
@@ -1142,19 +1168,26 @@ int pwrdm_read_prev_logic_pwrst(struct powerdomain *pwrdm)
  * pwrdm_read_logic_retst - get next powerdomain logic power state
  * @pwrdm: struct powerdomain * to get next logic power state
  *
- * Return the powerdomain pwrdm's logic power state.  Returns -EINVAL
+ * Return the powerdomain pwrdm's next logic power state.  Returns -EINVAL
  * if the powerdomain pointer is null or returns the next logic
  * power state upon success.
  */
 int pwrdm_read_logic_retst(struct powerdomain *pwrdm)
 {
-	int ret = -EINVAL;
+	int retst, ret = -EINVAL;
 
 	if (!pwrdm)
 		return -EINVAL;
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_read_logic_retst)
+	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_NEXT_LOGIC_RETST, &retst))
+		return retst;
+
+	if (arch_pwrdm && arch_pwrdm->pwrdm_read_logic_retst) {
 		ret = arch_pwrdm->pwrdm_read_logic_retst(pwrdm);
+		if (ret >= 0)
+			pwrdm_cache_write(pwrdm, PWRDM_CACHE_NEXT_LOGIC_RETST,
+					  ret);
+	}
 
 	return ret;
 }
@@ -1162,7 +1195,7 @@ int pwrdm_read_logic_retst(struct powerdomain *pwrdm)
 /**
  * pwrdm_read_mem_pwrst - get current memory bank power state
  * @pwrdm: struct powerdomain * to get current memory bank power state
- * @bank: memory bank number (0-3)
+ * @bank: memory bank number (0-PWRDM_MAX_MEM_BANKS)
  *
  * Return the powerdomain @pwrdm's current memory power state for bank
  * @bank.  Returns -EINVAL if the powerdomain pointer is null, -EEXIST if
@@ -1171,7 +1204,7 @@ int pwrdm_read_logic_retst(struct powerdomain *pwrdm)
  */
 int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 {
-	int ret = -EINVAL;
+	int pwrst, ret = -EINVAL;
 
 	if (!pwrdm)
 		return ret;
@@ -1182,8 +1215,15 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 	if (pwrdm->flags & PWRDM_HAS_MPU_QUIRK)
 		bank = 1;
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_read_mem_pwrst)
+	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_MEM_PWRST + bank, &pwrst))
+		return pwrst;
+
+	if (arch_pwrdm && arch_pwrdm->pwrdm_read_mem_pwrst) {
 		ret = arch_pwrdm->pwrdm_read_mem_pwrst(pwrdm, bank);
+		if (ret >= 0)
+			pwrdm_cache_write(pwrdm, PWRDM_CACHE_MEM_PWRST + bank,
+					  ret);
+	}
 
 	return ret;
 }
@@ -1191,7 +1231,7 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 /**
  * pwrdm_read_prev_mem_pwrst - get previous memory bank power state
  * @pwrdm: struct powerdomain * to get previous memory bank power state
- * @bank: memory bank number (0-3)
+ * @bank: memory bank number (0-PWRDM_MAX_MEM_BANKS)
  *
  * Return the powerdomain @pwrdm's previous memory power state for
  * bank @bank.  Returns -EINVAL if the powerdomain pointer is null,
@@ -1201,7 +1241,7 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
  */
 int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 {
-	int ret = -EINVAL;
+	int pwrst, ret = -EINVAL;
 
 	if (!pwrdm)
 		return ret;
@@ -1212,8 +1252,16 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 	if (pwrdm->flags & PWRDM_HAS_MPU_QUIRK)
 		bank = 1;
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_read_prev_mem_pwrst)
+	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PREV_MEM_PWRST + bank, &pwrst))
+		return pwrst;
+
+	if (arch_pwrdm && arch_pwrdm->pwrdm_read_prev_mem_pwrst) {
 		ret = arch_pwrdm->pwrdm_read_prev_mem_pwrst(pwrdm, bank);
+		if (ret >= 0)
+			pwrdm_cache_write(pwrdm,
+					  PWRDM_CACHE_PREV_MEM_PWRST + bank,
+					  ret);
+	}
 
 	return ret;
 }
@@ -1221,7 +1269,7 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
 /**
  * pwrdm_read_mem_retst - get next memory bank power state
  * @pwrdm: struct powerdomain * to get mext memory bank power state
- * @bank: memory bank number (0-3)
+ * @bank: memory bank number (0-PWRDM_MAX_MEM_BANKS)
  *
  * Return the powerdomain pwrdm's next memory power state for bank
  * x.  Returns -EINVAL if the powerdomain pointer is null, -EEXIST if
@@ -1230,7 +1278,7 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
  */
 int pwrdm_read_mem_retst(struct powerdomain *pwrdm, u8 bank)
 {
-	int ret = -EINVAL;
+	int pwrst, ret = -EINVAL;
 
 	if (!pwrdm)
 		return ret;
@@ -1238,8 +1286,16 @@ int pwrdm_read_mem_retst(struct powerdomain *pwrdm, u8 bank)
 	if (pwrdm->banks < (bank + 1))
 		return ret;
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_read_mem_retst)
+	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_NEXT_MEM_RETST + bank, &pwrst))
+		return pwrst;
+
+	if (arch_pwrdm && arch_pwrdm->pwrdm_read_mem_retst) {
 		ret = arch_pwrdm->pwrdm_read_mem_retst(pwrdm, bank);
+		if (ret >= 0)
+			pwrdm_cache_write(pwrdm,
+					  PWRDM_CACHE_NEXT_MEM_RETST + bank,
+					  ret);
+	}
 
 	return ret;
 }
-- 
1.7.7.6

[PATCH 3/5] ARM: OMAP2+: PM: use the power domains registers cache for the logic and mem states

[Jon Hunter]

Hi Jean,

On 05/01/2012 08:07 AM, jean.pihet at newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
> 
> Use the caching API for the previous, current and next
> power domains logical and memory states.
> 
> Signed-off-by: Jean Pihet <j-pihet@ti.com>
> ---
>  arch/arm/mach-omap2/powerdomain.c |  108 ++++++++++++++++++++++++++++---------
>  1 files changed, 82 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index 2058e27..9800b2b 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -1011,11 +1011,15 @@ int pwrdm_set_logic_retst(struct powerdomain *pwrdm, u8 pwrst)
>  	if (!(pwrdm->pwrsts_logic_ret & (1 << pwrst)))
>  		return -EINVAL;
>  
> -	pr_debug("powerdomain: setting next logic powerstate for %s to %0x\n",
> +	pr_debug("powerdomain: setting next logic RET state for %s to %0x\n",
>  		 pwrdm->name, pwrst);
>  
> -	if (arch_pwrdm && arch_pwrdm->pwrdm_set_logic_retst)
> +	if (arch_pwrdm && arch_pwrdm->pwrdm_set_logic_retst) {
>  		ret = arch_pwrdm->pwrdm_set_logic_retst(pwrdm, pwrst);
> +		if (!ret)
> +			pwrdm_cache_write(pwrdm, PWRDM_CACHE_NEXT_LOGIC_RETST,
> +					  pwrst);
> +	}
>  
>  	return ret;
>  }
> @@ -1023,13 +1027,13 @@ int pwrdm_set_logic_retst(struct powerdomain *pwrdm, u8 pwrst)
>  /**
>   * pwrdm_set_mem_onst - set memory power state while powerdomain ON
>   * @pwrdm: struct powerdomain * to set
> - * @bank: memory bank number to set (0-3)
> + * @bank: memory bank number to set (0-PWRDM_MAX_MEM_BANKS)
>   * @pwrst: one of the PWRDM_LOGIC_MEM_PWRST_* macros
>   *
>   * Set the next power state @pwrst that memory bank @bank of the
>   * powerdomain @pwrdm will enter when the powerdomain enters the ON
> - * state.  @bank will be a number from 0 to 3, and represents different
> - * types of memory, depending on the powerdomain.  Returns -EINVAL if
> + * state.  @bank represents different types of memory, depending on
> + * the powerdomain.  Returns -EINVAL if
>   * the powerdomain pointer is null or the target power state is not
>   * not supported for this memory bank, -EEXIST if the target memory
>   * bank does not exist or is not controllable, or returns 0 upon
> @@ -1051,8 +1055,12 @@ int pwrdm_set_mem_onst(struct powerdomain *pwrdm, u8 bank, u8 pwrst)
>  	pr_debug("powerdomain: setting next memory powerstate for domain %s "
>  		 "bank %0x while pwrdm-ON to %0x\n", pwrdm->name, bank, pwrst);
>  
> -	if (arch_pwrdm && arch_pwrdm->pwrdm_set_mem_onst)
> +	if (arch_pwrdm && arch_pwrdm->pwrdm_set_mem_onst) {
>  		ret = arch_pwrdm->pwrdm_set_mem_onst(pwrdm, bank, pwrst);
> +		if (!ret)
> +			pwrdm_cache_write(pwrdm, PWRDM_CACHE_MEM_ONST + bank,
> +					  pwrst);
> +	}
>  
>  	return ret;
>  }
> @@ -1060,13 +1068,13 @@ int pwrdm_set_mem_onst(struct powerdomain *pwrdm, u8 bank, u8 pwrst)
>  /**
>   * pwrdm_set_mem_retst - set memory power state while powerdomain in RET
>   * @pwrdm: struct powerdomain * to set
> - * @bank: memory bank number to set (0-3)
> + * @bank: memory bank number to set (0-PWRDM_MAX_MEM_BANKS)
>   * @pwrst: one of the PWRDM_LOGIC_MEM_PWRST_* macros
>   *
>   * Set the next power state @pwrst that memory bank @bank of the
>   * powerdomain @pwrdm will enter when the powerdomain enters the
> - * RETENTION state.  Bank will be a number from 0 to 3, and represents
> - * different types of memory, depending on the powerdomain.  @pwrst
> + * RETENTION state.  Bank represents different types of memory,
> + * depending on the powerdomain.  @pwrst
>   * will be either RETENTION or OFF, if supported.  Returns -EINVAL if
>   * the powerdomain pointer is null or the target power state is not
>   * not supported for this memory bank, -EEXIST if the target memory
> @@ -1089,8 +1097,13 @@ int pwrdm_set_mem_retst(struct powerdomain *pwrdm, u8 bank, u8 pwrst)
>  	pr_debug("powerdomain: setting next memory powerstate for domain %s "
>  		 "bank %0x while pwrdm-RET to %0x\n", pwrdm->name, bank, pwrst);
>  
> -	if (arch_pwrdm && arch_pwrdm->pwrdm_set_mem_retst)
> +	if (arch_pwrdm && arch_pwrdm->pwrdm_set_mem_retst) {
>  		ret = arch_pwrdm->pwrdm_set_mem_retst(pwrdm, bank, pwrst);
> +		if (!ret)
> +			pwrdm_cache_write(pwrdm,
> +					  PWRDM_CACHE_NEXT_MEM_RETST + bank,
> +					  pwrst);
> +	}
>  
>  	return ret;
>  }
> @@ -1106,13 +1119,19 @@ int pwrdm_set_mem_retst(struct powerdomain *pwrdm, u8 bank, u8 pwrst)
>   */
>  int pwrdm_read_logic_pwrst(struct powerdomain *pwrdm)
>  {
> -	int ret = -EINVAL;
> +	int retst, ret = -EINVAL;
>  
>  	if (!pwrdm)
>  		return -EINVAL;
>  
> -	if (arch_pwrdm && arch_pwrdm->pwrdm_read_logic_pwrst)
> +	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_LOGIC_RETST, &retst))
> +		return retst;
> +
> +	if (arch_pwrdm && arch_pwrdm->pwrdm_read_logic_pwrst) {
>  		ret = arch_pwrdm->pwrdm_read_logic_pwrst(pwrdm);
> +		if (ret >= 0)
> +			pwrdm_cache_write(pwrdm, PWRDM_CACHE_LOGIC_RETST, ret);
> +	}
>  
>  	return ret;
>  }
> @@ -1127,13 +1146,20 @@ int pwrdm_read_logic_pwrst(struct powerdomain *pwrdm)
>   */
>  int pwrdm_read_prev_logic_pwrst(struct powerdomain *pwrdm)
>  {
> -	int ret = -EINVAL;
> +	int retst, ret = -EINVAL;
>  
>  	if (!pwrdm)
>  		return -EINVAL;
>  
> -	if (arch_pwrdm && arch_pwrdm->pwrdm_read_prev_logic_pwrst)
> +	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PREV_LOGIC_PWRST, &retst))
> +		return retst;
> +
> +	if (arch_pwrdm && arch_pwrdm->pwrdm_read_prev_logic_pwrst) {
>  		ret = arch_pwrdm->pwrdm_read_prev_logic_pwrst(pwrdm);
> +		if (ret >= 0)
> +			pwrdm_cache_write(pwrdm, PWRDM_CACHE_PREV_LOGIC_PWRST,
> +					  ret);
> +	}

Is it necessary to cache the previous state? I don't believe this read
that often. In fact I am curious how often the cache value is read.

>  	return ret;
>  }
> @@ -1142,19 +1168,26 @@ int pwrdm_read_prev_logic_pwrst(struct powerdomain *pwrdm)
>   * pwrdm_read_logic_retst - get next powerdomain logic power state
>   * @pwrdm: struct powerdomain * to get next logic power state
>   *
> - * Return the powerdomain pwrdm's logic power state.  Returns -EINVAL
> + * Return the powerdomain pwrdm's next logic power state.  Returns -EINVAL
>   * if the powerdomain pointer is null or returns the next logic
>   * power state upon success.
>   */
>  int pwrdm_read_logic_retst(struct powerdomain *pwrdm)
>  {
> -	int ret = -EINVAL;
> +	int retst, ret = -EINVAL;
>  
>  	if (!pwrdm)
>  		return -EINVAL;
>  
> -	if (arch_pwrdm && arch_pwrdm->pwrdm_read_logic_retst)
> +	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_NEXT_LOGIC_RETST, &retst))
> +		return retst;
> +
> +	if (arch_pwrdm && arch_pwrdm->pwrdm_read_logic_retst) {
>  		ret = arch_pwrdm->pwrdm_read_logic_retst(pwrdm);
> +		if (ret >= 0)
> +			pwrdm_cache_write(pwrdm, PWRDM_CACHE_NEXT_LOGIC_RETST,
> +					  ret);
> +	}
>  
>  	return ret;
>  }
> @@ -1162,7 +1195,7 @@ int pwrdm_read_logic_retst(struct powerdomain *pwrdm)
>  /**
>   * pwrdm_read_mem_pwrst - get current memory bank power state
>   * @pwrdm: struct powerdomain * to get current memory bank power state
> - * @bank: memory bank number (0-3)
> + * @bank: memory bank number (0-PWRDM_MAX_MEM_BANKS)
>   *
>   * Return the powerdomain @pwrdm's current memory power state for bank
>   * @bank.  Returns -EINVAL if the powerdomain pointer is null, -EEXIST if
> @@ -1171,7 +1204,7 @@ int pwrdm_read_logic_retst(struct powerdomain *pwrdm)
>   */
>  int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>  {
> -	int ret = -EINVAL;
> +	int pwrst, ret = -EINVAL;
>  
>  	if (!pwrdm)
>  		return ret;
> @@ -1182,8 +1215,15 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>  	if (pwrdm->flags & PWRDM_HAS_MPU_QUIRK)
>  		bank = 1;
>  
> -	if (arch_pwrdm && arch_pwrdm->pwrdm_read_mem_pwrst)
> +	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_MEM_PWRST + bank, &pwrst))
> +		return pwrst;
> +
> +	if (arch_pwrdm && arch_pwrdm->pwrdm_read_mem_pwrst) {
>  		ret = arch_pwrdm->pwrdm_read_mem_pwrst(pwrdm, bank);
> +		if (ret >= 0)
> +			pwrdm_cache_write(pwrdm, PWRDM_CACHE_MEM_PWRST + bank,
> +					  ret);
> +	}
>  
>  	return ret;
>  }
> @@ -1191,7 +1231,7 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>  /**
>   * pwrdm_read_prev_mem_pwrst - get previous memory bank power state
>   * @pwrdm: struct powerdomain * to get previous memory bank power state
> - * @bank: memory bank number (0-3)
> + * @bank: memory bank number (0-PWRDM_MAX_MEM_BANKS)
>   *
>   * Return the powerdomain @pwrdm's previous memory power state for
>   * bank @bank.  Returns -EINVAL if the powerdomain pointer is null,
> @@ -1201,7 +1241,7 @@ int pwrdm_read_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>   */
>  int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>  {
> -	int ret = -EINVAL;
> +	int pwrst, ret = -EINVAL;
>  
>  	if (!pwrdm)
>  		return ret;
> @@ -1212,8 +1252,16 @@ int pwrdm_read_prev_mem_pwrst(struct powerdomain *pwrdm, u8 bank)
>  	if (pwrdm->flags & PWRDM_HAS_MPU_QUIRK)
>  		bank = 1;
>  
> -	if (arch_pwrdm && arch_pwrdm->pwrdm_read_prev_mem_pwrst)
> +	if (!pwrdm_cache_read(pwrdm, PWRDM_CACHE_PREV_MEM_PWRST + bank, &pwrst))
> +		return pwrst;
> +
> +	if (arch_pwrdm && arch_pwrdm->pwrdm_read_prev_mem_pwrst) {
>  		ret = arch_pwrdm->pwrdm_read_prev_mem_pwrst(pwrdm, bank);
> +		if (ret >= 0)
> +			pwrdm_cache_write(pwrdm,
> +					  PWRDM_CACHE_PREV_MEM_PWRST + bank,
> +					  ret);
> +	}

This field is also updated by hardware. Is this appropriate for the cache?

Cheers
Jon

[PATCH 4/5] ARM: OMAP2+: PM: use the power domains registers cache invalidate API

[jean.pihet at newoldbits.com]

From: Jean Pihet <j-pihet@ti.com>

The power domains registers cache is partially invalidated when clearing
the previous power states and after coming back from the low power mode,
i.e. after the return from the low level WFI instruction.

Note: this invalidate use scheme is optimized for performance, the states
returned by the state read functions might not reflect the real state if
the power domains registers have not been changed using the API or have
not been changed by the operating system (e.g. the ROM code in case of
secure code execution).

Signed-off-by: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/mach-omap2/pm34xx.c      |    6 ++++++
 arch/arm/mach-omap2/powerdomain.c |    4 +++-
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index ed73ffc..a222f517 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -353,6 +353,12 @@ void omap_sram_idle(void)
 	else
 		omap34xx_do_sram_idle(save_state);
 
+	/*
+	 * Invalidate the current states from the regs cache
+	 * for all power domains
+	 */
+	pwrdm_invalidate_regs_cache_fields_current(NULL);
+
 	/* Restore normal SDRC POWER settings */
 	if (cpu_is_omap3430() && omap_rev() >= OMAP3430_REV_ES3_0 &&
 	    (omap_type() == OMAP2_DEVICE_TYPE_EMU ||
diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 9800b2b..0b9259b 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -1324,8 +1324,10 @@ int pwrdm_clear_all_prev_pwrst(struct powerdomain *pwrdm)
 	pr_debug("powerdomain: clearing previous power state reg for %s\n",
 		 pwrdm->name);
 
-	if (arch_pwrdm && arch_pwrdm->pwrdm_clear_all_prev_pwrst)
+	if (arch_pwrdm && arch_pwrdm->pwrdm_clear_all_prev_pwrst) {
 		ret = arch_pwrdm->pwrdm_clear_all_prev_pwrst(pwrdm);
+		pwrdm_invalidate_regs_cache_fields_prev(pwrdm);
+	}
 
 	return ret;
 }
-- 
1.7.7.6

[PATCH 5/5] ARM: OMAP2+: PM debug: use the power domains registers caching API

[jean.pihet at newoldbits.com]

From: Jean Pihet <j-pihet@ti.com>

Use the caching API instead of using the internal pwrdm->state field
for PM debug statistics display.

Note: some power domains states mismatch messages can be printed out with
the power domains states statstics, indicating that the power domains are
not controlled by any driver or that the invalidate API is not correctly
used.

Signed-off-by: Jean Pihet <j-pihet@ti.com>
---
 arch/arm/mach-omap2/pm-debug.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
index 13c00fb..ed9846e 100644
--- a/arch/arm/mach-omap2/pm-debug.c
+++ b/arch/arm/mach-omap2/pm-debug.c
@@ -96,19 +96,20 @@ static int clkdm_dbg_show_counter(struct clockdomain *clkdm, void *user)
 static int pwrdm_dbg_show_counter(struct powerdomain *pwrdm, void *user)
 {
 	struct seq_file *s = (struct seq_file *)user;
-	int i;
+	int i, pwrst;
 
 	if (strcmp(pwrdm->name, "emu_pwrdm") == 0 ||
 		strcmp(pwrdm->name, "wkup_pwrdm") == 0 ||
 		strncmp(pwrdm->name, "dpll", 4) == 0)
 		return 0;
 
-	if (pwrdm->state != pwrdm_read_func_pwrst(pwrdm))
-		printk(KERN_ERR "pwrdm state mismatch(%s) %d != %d\n",
-		       pwrdm->name, pwrdm->state, pwrdm_read_func_pwrst(pwrdm));
+	pwrst = pwrdm_read_func_pwrst(pwrdm);
+	if (pwrdm->state != pwrst)
+		printk(KERN_ERR "pwrdm state mismatch(%s): last saved %d != current %d\n",
+		       pwrdm->name, pwrdm->state, pwrst);
 
 	seq_printf(s, "%s (%s)", pwrdm->name,
-			pwrdm_state_names[pwrdm->state]);
+			pwrdm_state_names[pwrst]);
 	for (i = 0; i < PWRDM_MAX_FUNC_PWRSTS; i++)
 		seq_printf(s, ",%s:%d", pwrdm_state_names[i],
 			pwrdm->state_counter[i]);
@@ -126,7 +127,7 @@ static int pwrdm_dbg_show_counter(struct powerdomain *pwrdm, void *user)
 static int pwrdm_dbg_show_timer(struct powerdomain *pwrdm, void *user)
 {
 	struct seq_file *s = (struct seq_file *)user;
-	int i;
+	int i, pwrst;
 
 	if (strcmp(pwrdm->name, "emu_pwrdm") == 0 ||
 		strcmp(pwrdm->name, "wkup_pwrdm") == 0 ||
@@ -134,9 +135,10 @@ static int pwrdm_dbg_show_timer(struct powerdomain *pwrdm, void *user)
 		return 0;
 
 	pwrdm_state_switch(pwrdm);
+	pwrst = pwrdm_read_func_pwrst(pwrdm);
 
 	seq_printf(s, "%s (%s)", pwrdm->name,
-		pwrdm_state_names[pwrdm->state]);
+		pwrdm_state_names[pwrst]);
 
 	for (i = 0; i < PWRDM_MAX_FUNC_PWRSTS; i++)
 		seq_printf(s, ",%s:%lld", pwrdm_state_names[i],
-- 
1.7.7.6

[PATCH 0/5] ARM: OMAP2+: PM: implement a caching mechanism on the power domains state registers

[Jon Hunter]

Hi Jean,

On 05/01/2012 08:07 AM, jean.pihet at newoldbits.com wrote:
> From: Jean Pihet <j-pihet@ti.com>
> 
> The OMAP3 PRCM registers accesses are known to be slow, with a PRCM register
> read taking up to 12-14us depending on the OPP.
> 
> This patch adds a caching mechanism on the power domains state registers.
> When the cache is cold or has been invalidated a register access is
> performed, otherwise the register value is retrieved from the registers
> cache.
> The API is made of read and write functions for fields in the cache, as well
> as an invalidate and helper functions to invalidate parts of the cache
> contents (i.e. previous, current power states and all fields in the cache).
> The power domain code is converted to use the API to read and write the
> previous, current and next states for the power domains states, logical
> and memory states.
> The PM debug code also uses the caching API instead of the internal
> pwrdm->state variable.
> 
> Using the caching mechanism optimizes the performance of the system in the
> transitions to and from the low power states.

Looks interesting!

A couple high level questions for you ...

1. Do you intend to cache registers that are updated by hardware?
2. If yes to 1, should there be some "cache debug mode" we can enable to
test the cache and registers are in sync for testing?

Cheers
Jon