[PATCH/RFC 1/2] OMAP2+: powerdomain: add API to get context loss count

[PATCH/RFC 1/2] OMAP2+: powerdomain: add API to get context loss count

[Kevin Hilman]

Add new powerdomain API

    int pwrdm_get_context_loss_count(struct powerdomain *pwrdm)

for checking how many times the powerdomain has lost context.  The
loss count is the sum sum of the powerdomain off-mode counter, the
logic off counter and the per-bank memory off counter.

Cc: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/powerdomain.c |   23 +++++++++++++++++++++++
 arch/arm/mach-omap2/powerdomain.h |    1 +
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
index 06ef60e..78e7d22 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -909,3 +909,26 @@ int pwrdm_post_transition(void)
 	pwrdm_for_each(_pwrdm_post_transition_cb, NULL);
 	return 0;
 }
+
+/**
+ * pwrdm_get_context_loss_count - get powerdomain's context loss count
+ * @pwrdm: struct powerdomain * to wait for
+ *
+ * Context loss count is a sum of powerdomain off-mode counter,
+ * the logic off counter and the per-bank memory off counter.
+ */
+int pwrdm_get_context_loss_count(struct powerdomain *pwrdm)
+{
+	int i, count;
+
+	count = pwrdm->state_counter[PWRDM_POWER_OFF];
+	count += pwrdm->ret_logic_off_counter;
+
+	for (i = 0; i < pwrdm->banks; i++)
+		count += pwrdm->ret_mem_off_counter[i];
+
+	pr_debug("powerdomain: %s: context loss count = %u\n",
+		 pwrdm->name, count);
+
+	return count;
+}
diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h
index 35b5b48..d269eff 100644
--- a/arch/arm/mach-omap2/powerdomain.h
+++ b/arch/arm/mach-omap2/powerdomain.h
@@ -211,6 +211,7 @@ int pwrdm_clkdm_state_switch(struct clockdomain *clkdm);
 int pwrdm_pre_transition(void);
 int pwrdm_post_transition(void);
 int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm);
+int pwrdm_get_context_loss_count(struct powerdomain *pwrdm);
 
 extern void omap2xxx_powerdomains_init(void);
 extern void omap3xxx_powerdomains_init(void);
-- 
1.7.2.1

[PATCH/RFC 2/2] OMAP: PM: implement context loss count APIs

[Kevin Hilman]

Implement OMAP PM layer omap_pm_get_dev_context_loss_count() API by
creating similar APIs at the omap_device and omap_hwmod levels.  The
omap_hwmod level call is the layer with access to the powerdomain
core, so it is the place where the powerdomain is queried to get the
context loss count.

NOTE: only works for devices which have been converted to use
      omap_device/omap_hwmod.

Longer term, we could possibly remove this API from the OMAP PM layer,
and instead directly use the omap_device level API.

Cc: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/omap_hwmod.c              |   18 ++++++++++++++++++
 arch/arm/plat-omap/include/plat/omap_device.h |    1 +
 arch/arm/plat-omap/include/plat/omap_hwmod.h  |    1 +
 arch/arm/plat-omap/omap-pm-noop.c             |   17 +++++++++--------
 arch/arm/plat-omap/omap_device.c              |   20 ++++++++++++++++++++
 5 files changed, 49 insertions(+), 8 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index 81c1097..0ec9c70 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2220,3 +2220,21 @@ ohsps_unlock:
 
 	return ret;
 }
+
+/**
+ * omap_hwmod_get_context_loss_count - get lost context count
+ * @oh: struct omap_hwmod *
+ *
+ * Query the powerdomain of of @oh to get the context loss
+ * count for this device.
+ */
+int omap_hwmod_get_context_loss_count(struct omap_hwmod *oh)
+{
+	struct powerdomain *pwrdm;
+
+	pwrdm = omap_hwmod_get_pwrdm(oh);
+	if (!pwrdm)
+		return -ENODEV;
+
+	return pwrdm_get_context_loss_count(pwrdm);
+}
diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
index 28e2d1a..70d31d0 100644
--- a/arch/arm/plat-omap/include/plat/omap_device.h
+++ b/arch/arm/plat-omap/include/plat/omap_device.h
@@ -107,6 +107,7 @@ void __iomem *omap_device_get_rt_va(struct omap_device *od);
 int omap_device_align_pm_lat(struct platform_device *pdev,
 			     u32 new_wakeup_lat_limit);
 struct powerdomain *omap_device_get_pwrdm(struct omap_device *od);
+int omap_device_get_context_loss_count(struct platform_device *pdev);
 
 /* Other */
 
diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
index 62bdb23..5a96ac5 100644
--- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
+++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
@@ -568,6 +568,7 @@ int omap_hwmod_for_each_by_class(const char *classname,
 				 void *user);
 
 int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state);
+int omap_hwmod_get_context_loss_count(struct omap_hwmod *oh);
 
 /*
  * Chip variant-specific hwmod init routines - XXX should be converted
diff --git a/arch/arm/plat-omap/omap-pm-noop.c b/arch/arm/plat-omap/omap-pm-noop.c
index 7578366..5a58f97 100644
--- a/arch/arm/plat-omap/omap-pm-noop.c
+++ b/arch/arm/plat-omap/omap-pm-noop.c
@@ -20,9 +20,11 @@
 #include <linux/init.h>
 #include <linux/cpufreq.h>
 #include <linux/device.h>
+#include <linux/platform_device.h>
 
 /* Interface documentation is in mach/omap-pm.h */
 #include <plat/omap-pm.h>
+#include <plat/omap_device.h>
 
 struct omap_opp *dsp_opps;
 struct omap_opp *mpu_opps;
@@ -288,20 +290,19 @@ unsigned long omap_pm_cpu_get_freq(void)
 
 int omap_pm_get_dev_context_loss_count(struct device *dev)
 {
+	struct platform_device *pdev = to_platform_device(dev);
+	int count;
+
 	if (!dev) {
 		WARN_ON(1);
 		return -EINVAL;
 	};
 
-	pr_debug("OMAP PM: returning context loss count for dev %s\n",
-		 dev_name(dev));
+	count = omap_device_get_context_loss_count(pdev);
+	pr_debug("OMAP PM: context loss count for dev %s = %d\n",
+		 dev_name(dev), count);
 
-	/*
-	 * Map the device to the powerdomain.  Return the powerdomain
-	 * off counter.
-	 */
-
-	return 0;
+	return count;
 }
 
 
diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
index abe933c..e29c2d6 100644
--- a/arch/arm/plat-omap/omap_device.c
+++ b/arch/arm/plat-omap/omap_device.c
@@ -280,6 +280,26 @@ static void _add_optional_clock_alias(struct omap_device *od,
 /* Public functions for use by core code */
 
 /**
+ * omap_device_get_context_loss_count - get lost context count
+ * @od: struct omap_device *
+ *
+ * Using the primary hwmod for this device, query the context loss
+ * count for this device.
+ */
+int omap_device_get_context_loss_count(struct platform_device *pdev)
+{
+	struct omap_device *od;
+	int ret = -ENODEV;
+
+	od = _find_by_pdev(pdev);
+
+	if (od->hwmods_cnt)
+		omap_hwmod_get_context_loss_count(od->hwmods[0]);
+
+	return ret;
+}
+
+/**
  * omap_device_count_resources - count number of struct resource entries needed
  * @od: struct omap_device *
  *
-- 
1.7.2.1

[PATCH/RFC 3/2] OMAP: PM noop: implement context loss count for non-omap_devices

[Kevin Hilman]

For devices which have not (yet) been converted to use omap_device,
implement the context loss counter using the "brutal method" as
originally proposed by Paul Walmsley[1].

The dummy context loss counter is incremented every time it is
checked, but only when off-mode is enabled.  When off-mode is
disabled, the dummy counter stops incrementing.

Tested on 36xx/Zoom3 using MMC driver, which is currently the
only in-tree user of this API.

This patch should be reverted after all devices are converted to using
omap_device.

[1] http://marc.info/?l=linux-omap&m=129176260000626&w=2

Cc: Paul Walmsley <paul@pwsan.com>
Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
---
 arch/arm/mach-omap2/pm-debug.c            |    2 ++
 arch/arm/plat-omap/include/plat/omap-pm.h |    1 +
 arch/arm/plat-omap/omap-pm-noop.c         |   19 ++++++++++++++++++-
 3 files changed, 21 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
index e535082..af66acb 100644
--- a/arch/arm/mach-omap2/pm-debug.c
+++ b/arch/arm/mach-omap2/pm-debug.c
@@ -32,6 +32,7 @@
 #include "powerdomain.h"
 #include "clockdomain.h"
 #include <plat/dmtimer.h>
+#include <plat/omap-pm.h>
 
 #include "cm2xxx_3xxx.h"
 #include "prm2xxx_3xxx.h"
@@ -581,6 +582,7 @@ static int option_set(void *data, u64 val)
 	*option = val;
 
 	if (option == &enable_off_mode) {
+		omap_pm_enable_off_mode(val ? true : false);
 		if (cpu_is_omap34xx())
 			omap3_pm_off_mode_enable(val);
 	}
diff --git a/arch/arm/plat-omap/include/plat/omap-pm.h b/arch/arm/plat-omap/include/plat/omap-pm.h
index 9870b4f..cfa5e44 100644
--- a/arch/arm/plat-omap/include/plat/omap-pm.h
+++ b/arch/arm/plat-omap/include/plat/omap-pm.h
@@ -372,5 +372,6 @@ unsigned long omap_pm_cpu_get_freq(void);
  */
 int omap_pm_get_dev_context_loss_count(struct device *dev);
 
+void omap_pm_enable_off_mode(bool enable);
 
 #endif
diff --git a/arch/arm/plat-omap/omap-pm-noop.c b/arch/arm/plat-omap/omap-pm-noop.c
index 5a58f97..32f2818 100644
--- a/arch/arm/plat-omap/omap-pm-noop.c
+++ b/arch/arm/plat-omap/omap-pm-noop.c
@@ -30,6 +30,9 @@ struct omap_opp *dsp_opps;
 struct omap_opp *mpu_opps;
 struct omap_opp *l3_opps;
 
+static bool off_mode_enabled;
+static u32 dummy_context_loss_counter;
+
 /*
  * Device-driver-originated constraints (via board-*.c files)
  */
@@ -287,6 +290,10 @@ unsigned long omap_pm_cpu_get_freq(void)
 /*
  * Device context loss tracking
  */
+void omap_pm_enable_off_mode(bool enable)
+{
+	off_mode_enabled = enable;
+}
 
 int omap_pm_get_dev_context_loss_count(struct device *dev)
 {
@@ -298,7 +305,17 @@ int omap_pm_get_dev_context_loss_count(struct device *dev)
 		return -EINVAL;
 	};
 
-	count = omap_device_get_context_loss_count(pdev);
+	if (dev->parent == &omap_device_parent) {
+		count = omap_device_get_context_loss_count(pdev);
+	} else {
+		WARN_ONCE(off_mode_enabled, "using dummy context loss counter, "
+			  "device %s should be converted to omap_device.",
+			  __func__, dev_name(dev));
+		if (off_mode_enabled)
+			dummy_context_loss_counter++;
+		count = dummy_context_loss_counter;
+	}
+
 	pr_debug("OMAP PM: context loss count for dev %s = %d\n",
 		 dev_name(dev), count);
 
-- 
1.7.2.1

RE: [PATCH/RFC 2/2] OMAP: PM: implement context loss count APIs

[Vishwanath Sripathy]

Kevin,

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Kevin Hilman
> Sent: Friday, December 10, 2010 5:39 AM
> To: linux-omap@vger.kernel.org
> Cc: Paul Walmsely
> Subject: [PATCH/RFC 2/2] OMAP: PM: implement context loss count APIs
>
> Implement OMAP PM layer omap_pm_get_dev_context_loss_count() API
> by
> creating similar APIs at the omap_device and omap_hwmod levels.  The
> omap_hwmod level call is the layer with access to the powerdomain
> core, so it is the place where the powerdomain is queried to get the
> context loss count.
>
> NOTE: only works for devices which have been converted to use
>       omap_device/omap_hwmod.
>
> Longer term, we could possibly remove this API from the OMAP PM layer,
> and instead directly use the omap_device level API.
>
> Cc: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c              |   18
> ++++++++++++++++++
>  arch/arm/plat-omap/include/plat/omap_device.h |    1 +
>  arch/arm/plat-omap/include/plat/omap_hwmod.h  |    1 +
>  arch/arm/plat-omap/omap-pm-noop.c             |   17 +++++++++-----
> ---
>  arch/arm/plat-omap/omap_device.c              |   20
> ++++++++++++++++++++
>  5 files changed, 49 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-
> omap2/omap_hwmod.c
> index 81c1097..0ec9c70 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -2220,3 +2220,21 @@ ohsps_unlock:
>
>  	return ret;
>  }
> +
> +/**
> + * omap_hwmod_get_context_loss_count - get lost context count
> + * @oh: struct omap_hwmod *
> + *
> + * Query the powerdomain of of @oh to get the context loss
> + * count for this device.
> + */
> +int omap_hwmod_get_context_loss_count(struct omap_hwmod *oh)
> +{
> +	struct powerdomain *pwrdm;
> +
> +	pwrdm = omap_hwmod_get_pwrdm(oh);
> +	if (!pwrdm)
> +		return -ENODEV;
> +
> +	return pwrdm_get_context_loss_count(pwrdm);
> +}
> diff --git a/arch/arm/plat-omap/include/plat/omap_device.h
> b/arch/arm/plat-omap/include/plat/omap_device.h
> index 28e2d1a..70d31d0 100644
> --- a/arch/arm/plat-omap/include/plat/omap_device.h
> +++ b/arch/arm/plat-omap/include/plat/omap_device.h
> @@ -107,6 +107,7 @@ void __iomem *omap_device_get_rt_va(struct
> omap_device *od);
>  int omap_device_align_pm_lat(struct platform_device *pdev,
>  			     u32 new_wakeup_lat_limit);
>  struct powerdomain *omap_device_get_pwrdm(struct omap_device
> *od);
> +int omap_device_get_context_loss_count(struct platform_device
> *pdev);
>
>  /* Other */
>
> diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h
> b/arch/arm/plat-omap/include/plat/omap_hwmod.h
> index 62bdb23..5a96ac5 100644
> --- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
> +++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
> @@ -568,6 +568,7 @@ int omap_hwmod_for_each_by_class(const char
> *classname,
>  				 void *user);
>
>  int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8
> state);
> +int omap_hwmod_get_context_loss_count(struct omap_hwmod *oh);
>
>  /*
>   * Chip variant-specific hwmod init routines - XXX should be converted
> diff --git a/arch/arm/plat-omap/omap-pm-noop.c b/arch/arm/plat-
> omap/omap-pm-noop.c
> index 7578366..5a58f97 100644
> --- a/arch/arm/plat-omap/omap-pm-noop.c
> +++ b/arch/arm/plat-omap/omap-pm-noop.c
> @@ -20,9 +20,11 @@
>  #include <linux/init.h>
>  #include <linux/cpufreq.h>
>  #include <linux/device.h>
> +#include <linux/platform_device.h>
>
>  /* Interface documentation is in mach/omap-pm.h */
>  #include <plat/omap-pm.h>
> +#include <plat/omap_device.h>
>
>  struct omap_opp *dsp_opps;
>  struct omap_opp *mpu_opps;
> @@ -288,20 +290,19 @@ unsigned long omap_pm_cpu_get_freq(void)
>
>  int omap_pm_get_dev_context_loss_count(struct device *dev)
>  {
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int count;
> +
>  	if (!dev) {
>  		WARN_ON(1);
>  		return -EINVAL;
>  	};
>
> -	pr_debug("OMAP PM: returning context loss count for dev %s\n",
> -		 dev_name(dev));
> +	count = omap_device_get_context_loss_count(pdev);
> +	pr_debug("OMAP PM: context loss count for dev %s = %d\n",
> +		 dev_name(dev), count);
Shouldn't this implementation be part of omap-pm.c where all the OMAP PM
functions are to be implemented? I thought omap-pm-noop.c should have
dummy implementation.

Vishwa

>
> -	/*
> -	 * Map the device to the powerdomain.  Return the powerdomain
> -	 * off counter.
> -	 */
> -
> -	return 0;
> +	return count;
>  }
>
>
> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-
> omap/omap_device.c
> index abe933c..e29c2d6 100644
> --- a/arch/arm/plat-omap/omap_device.c
> +++ b/arch/arm/plat-omap/omap_device.c
> @@ -280,6 +280,26 @@ static void _add_optional_clock_alias(struct
> omap_device *od,
>  /* Public functions for use by core code */
>
>  /**
> + * omap_device_get_context_loss_count - get lost context count
> + * @od: struct omap_device *
> + *
> + * Using the primary hwmod for this device, query the context loss
> + * count for this device.
> + */
> +int omap_device_get_context_loss_count(struct platform_device
> *pdev)
> +{
> +	struct omap_device *od;
> +	int ret = -ENODEV;
> +
> +	od = _find_by_pdev(pdev);
> +
> +	if (od->hwmods_cnt)
> +		omap_hwmod_get_context_loss_count(od->hwmods[0]);
> +
> +	return ret;
> +}
> +
> +/**
>   * omap_device_count_resources - count number of struct resource
> entries needed
>   * @od: struct omap_device *
>   *
> --
> 1.7.2.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH/RFC 2/2] OMAP: PM: implement context loss count APIs

[Kevin Hilman]

Vishwanath Sripathy <vishwanath.bs@ti.com> writes:

> Kevin,
[...]

>> @@ -288,20 +290,19 @@ unsigned long omap_pm_cpu_get_freq(void)
>>
>>  int omap_pm_get_dev_context_loss_count(struct device *dev)
>>  {
>> +	struct platform_device *pdev = to_platform_device(dev);
>> +	int count;
>> +
>>  	if (!dev) {
>>  		WARN_ON(1);
>>  		return -EINVAL;
>>  	};
>>
>> -	pr_debug("OMAP PM: returning context loss count for dev %s\n",
>> -		 dev_name(dev));
>> +	count = omap_device_get_context_loss_count(pdev);
>> +	pr_debug("OMAP PM: context loss count for dev %s = %d\n",
>> +		 dev_name(dev), count);
>
> Shouldn't this implementation be part of omap-pm.c where all the OMAP PM
> functions are to be implemented? 

Where is omap-pm.c?

Kevin

RE: [PATCH/RFC 2/2] OMAP: PM: implement context loss count APIs

[Vishwanath Sripathy]

Kevin,

> -----Original Message-----
> From: Kevin Hilman [mailto:khilman@deeprootsystems.com]
> Sent: Saturday, December 11, 2010 6:14 AM
> To: Vishwanath Sripathy
> Cc: linux-omap@vger.kernel.org; Paul Walmsely
> Subject: Re: [PATCH/RFC 2/2] OMAP: PM: implement context loss count
> APIs
>
> Vishwanath Sripathy <vishwanath.bs@ti.com> writes:
>
> > Kevin,
> [...]
>
> >> @@ -288,20 +290,19 @@ unsigned long
> omap_pm_cpu_get_freq(void)
> >>
> >>  int omap_pm_get_dev_context_loss_count(struct device *dev)
> >>  {
> >> +	struct platform_device *pdev = to_platform_device(dev);
> >> +	int count;
> >> +
> >>  	if (!dev) {
> >>  		WARN_ON(1);
> >>  		return -EINVAL;
> >>  	};
> >>
> >> -	pr_debug("OMAP PM: returning context loss count for dev %s\n",
> >> -		 dev_name(dev));
> >> +	count = omap_device_get_context_loss_count(pdev);
> >> +	pr_debug("OMAP PM: context loss count for dev %s = %d\n",
> >> +		 dev_name(dev), count);
> >
> > Shouldn't this implementation be part of omap-pm.c where all the
> OMAP PM
> > functions are to be implemented?
>
> Where is omap-pm.c?
It's not present and needs to be added. This file is anyway required for
adding device latency constraints as well.

Vishwa
>
> Kevin

RE: [PATCH/RFC 2/2] OMAP: PM: implement context loss count APIs

[Paul Walmsley]

Hello Vishwa,

On Fri, 10 Dec 2010, Vishwanath Sripathy wrote:

> > +	count = omap_device_get_context_loss_count(pdev);
> > +	pr_debug("OMAP PM: context loss count for dev %s = %d\n",
> > +		 dev_name(dev), count);
> Shouldn't this implementation be part of omap-pm.c where all the OMAP PM
> functions are to be implemented? I thought omap-pm-noop.c should have
> dummy implementation.

In general, yes.  But we also want the code in omap-pm-noop.c not to cause 
additional breakage.  Unlike most of the other functions in this file, if 
the context loss count function doesn't do something minimally useful, then
the system is going to break badly.  You've probably seen this thread:

http://www.mail-archive.com/linux-omap@vger.kernel.org/msg40079.html

(By the way, the reason why I think we shouldn't use the approach 
described in 

http://www.mail-archive.com/linux-omap@vger.kernel.org/msg40101.html

is because I suspect it is going to seriously damage retention idle 
performance.  For example, the HSMMC driver resets its entire IP block in 
its context restore function...)

But to confirm your general point, yes, in general, further functional
development of the OMAP PM code should take place outside the no-op file.
Hopefully, at some point, we'll be able to drop the no-op file.  Once 
there is a useful replacement, we should be able to switch to it as a 
default.


- Paul

RE: [PATCH/RFC 2/2] OMAP: PM: implement context loss count APIs

[Vishwanath Sripathy]

Paul,

> -----Original Message-----
> From: Paul Walmsley [mailto:paul@pwsan.com]
> Sent: Monday, December 13, 2010 4:13 PM
> To: Vishwanath Sripathy
> Cc: Kevin Hilman; linux-omap@vger.kernel.org
> Subject: RE: [PATCH/RFC 2/2] OMAP: PM: implement context loss count
> APIs
>
> Hello Vishwa,
>
> On Fri, 10 Dec 2010, Vishwanath Sripathy wrote:
>
> > > +	count = omap_device_get_context_loss_count(pdev);
> > > +	pr_debug("OMAP PM: context loss count for dev %s = %d\n",
> > > +		 dev_name(dev), count);
> > Shouldn't this implementation be part of omap-pm.c where all the
> OMAP PM
> > functions are to be implemented? I thought omap-pm-noop.c should
> have
> > dummy implementation.
>
> In general, yes.  But we also want the code in omap-pm-noop.c not to
> cause
> additional breakage.  Unlike most of the other functions in this file,
if
> the context loss count function doesn't do something minimally useful,
> then
> the system is going to break badly.  You've probably seen this thread:
>
> http://www.mail-archive.com/linux-
> omap@vger.kernel.org/msg40079.html
>
> (By the way, the reason why I think we shouldn't use the approach
> described in
>
> http://www.mail-archive.com/linux-
> omap@vger.kernel.org/msg40101.html
>
> is because I suspect it is going to seriously damage retention idle
> performance.  For example, the HSMMC driver resets its entire IP block
in
> its context restore function...)
>
> But to confirm your general point, yes, in general, further functional
> development of the OMAP PM code should take place outside the no-op
> file.
> Hopefully, at some point, we'll be able to drop the no-op file.  Once
> there is a useful replacement, we should be able to switch to it as a
> default.
I have no issues with the implementation and I agree. All I am saying is
that why can't this implementation be added in omap-pm.c and compile this
file instead of omap-pm-noop.c when OMAP_PM is enabled. I believe this
function is useful when off mode is enabled which means OMAP_PM is
enabled.

Vishwa
>
>
> - Paul

RE: [PATCH/RFC 2/2] OMAP: PM: implement context loss count APIs

[Paul Walmsley]

Vishwa

On Mon, 13 Dec 2010, Vishwanath Sripathy wrote:

> I have no issues with the implementation and I agree. All I am saying is 
> that why can't this implementation be added in omap-pm.c and compile 
> this file instead of omap-pm-noop.c

1. because omap-pm-noop.c needs to not actively break anything when it is 
   used, and this function, unlike most of the other functions in 
   omap-pm-noop.c, has no valid 'no-op' failure mode; and

2. because no one has yet posted a complete, clean, working, 
   SRF-free OMAP PM layer to the lists.  It was the hope several years ago
   when this interface was created that someone else would be capable of 
   doing this.

> when OMAP_PM is enabled.

What is this OMAP_PM that you are referring to?

> I believe this function is useful when off mode is enabled which means 
> OMAP_PM is enabled.

Huh?  Mainline Linux on OMAP can enter off-mode with omap2plus_defconfig 
right now via a runtime debugfs setting.  There is no KConfig setting to 
enable.


- Paul

Re: [PATCH/RFC 1/2] OMAP2+: powerdomain: add API to get context loss count

[Paul Walmsley]

Hi Kevin,

working my way through these...

On Thu, 9 Dec 2010, Kevin Hilman wrote:

> Add new powerdomain API
> 
>     int pwrdm_get_context_loss_count(struct powerdomain *pwrdm)
> 
> for checking how many times the powerdomain has lost context.  The
> loss count is the sum sum of the powerdomain off-mode counter, the
> logic off counter and the per-bank memory off counter.
> 
> Cc: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/powerdomain.c |   23 +++++++++++++++++++++++
>  arch/arm/mach-omap2/powerdomain.h |    1 +
>  2 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/powerdomain.c b/arch/arm/mach-omap2/powerdomain.c
> index 06ef60e..78e7d22 100644
> --- a/arch/arm/mach-omap2/powerdomain.c
> +++ b/arch/arm/mach-omap2/powerdomain.c
> @@ -909,3 +909,26 @@ int pwrdm_post_transition(void)
>  	pwrdm_for_each(_pwrdm_post_transition_cb, NULL);
>  	return 0;
>  }
> +
> +/**
> + * pwrdm_get_context_loss_count - get powerdomain's context loss count
> + * @pwrdm: struct powerdomain * to wait for
> + *
> + * Context loss count is a sum of powerdomain off-mode counter,
> + * the logic off counter and the per-bank memory off counter.
> + */
> +int pwrdm_get_context_loss_count(struct powerdomain *pwrdm)
> +{
> +	int i, count;

There should be a test here to prevent a null pointer deref if pwrdm is 
NULL.

> +
> +	count = pwrdm->state_counter[PWRDM_POWER_OFF];
> +	count += pwrdm->ret_logic_off_counter;
> +
> +	for (i = 0; i < pwrdm->banks; i++)
> +		count += pwrdm->ret_mem_off_counter[i];

Looks like these state counters are unsigned ints.  So they could easily 
overflow count when they are summed.  This is probably not a major problem 
as far as this function is concerned, but the next patch can return 
-ENODEV upon error, which, if this function were really unlucky, it could 
effectively return -ENODEV.

It would be good to constrain the minimum successful return value of this 
function to 0.  This will require some creativity since the function 
shouldn't just return 0 for all sums that wrap around and wind up 
negative; the return value should still continue to differ from the value 
when it was called previously.  Otherwise devices might not restore their 
context when they should.

> +
> +	pr_debug("powerdomain: %s: context loss count = %u\n",
> +		 pwrdm->name, count);
> +
> +	return count;
> +}
> diff --git a/arch/arm/mach-omap2/powerdomain.h b/arch/arm/mach-omap2/powerdomain.h
> index 35b5b48..d269eff 100644
> --- a/arch/arm/mach-omap2/powerdomain.h
> +++ b/arch/arm/mach-omap2/powerdomain.h
> @@ -211,6 +211,7 @@ int pwrdm_clkdm_state_switch(struct clockdomain *clkdm);
>  int pwrdm_pre_transition(void);
>  int pwrdm_post_transition(void);
>  int pwrdm_set_lowpwrstchange(struct powerdomain *pwrdm);
> +int pwrdm_get_context_loss_count(struct powerdomain *pwrdm);
>  
>  extern void omap2xxx_powerdomains_init(void);
>  extern void omap3xxx_powerdomains_init(void);

Also, as we discussed privately, there is at least one bug in the counters 
where the logic-off and membank-off counters don't increment correctly 
when the entire powerdomain is off.  But that problem does not have to be 
dealt with as part of this series...


- Paul

Re: [PATCH/RFC 2/2] OMAP: PM: implement context loss count APIs

[Paul Walmsley]

Hi Kevin

a few comments here

On Thu, 9 Dec 2010, Kevin Hilman wrote:

> Implement OMAP PM layer omap_pm_get_dev_context_loss_count() API by
> creating similar APIs at the omap_device and omap_hwmod levels.  The
> omap_hwmod level call is the layer with access to the powerdomain
> core, so it is the place where the powerdomain is queried to get the
> context loss count.
> 
> NOTE: only works for devices which have been converted to use
>       omap_device/omap_hwmod.
> 
> Longer term, we could possibly remove this API from the OMAP PM layer,
> and instead directly use the omap_device level API.
> 
> Cc: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/omap_hwmod.c              |   18 ++++++++++++++++++
>  arch/arm/plat-omap/include/plat/omap_device.h |    1 +
>  arch/arm/plat-omap/include/plat/omap_hwmod.h  |    1 +
>  arch/arm/plat-omap/omap-pm-noop.c             |   17 +++++++++--------
>  arch/arm/plat-omap/omap_device.c              |   20 ++++++++++++++++++++
>  5 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index 81c1097..0ec9c70 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -2220,3 +2220,21 @@ ohsps_unlock:
>  
>  	return ret;
>  }
> +
> +/**
> + * omap_hwmod_get_context_loss_count - get lost context count
> + * @oh: struct omap_hwmod *
> + *
> + * Query the powerdomain of of @oh to get the context loss
> + * count for this device.

Might want to document the possible return values of this function, and 
suggest that its callers consider the context to be lost whenever a 
successful call to this function results in any different value than the 
caller got the last time it called this code.

> + */
> +int omap_hwmod_get_context_loss_count(struct omap_hwmod *oh)
> +{
> +	struct powerdomain *pwrdm;
> +

Please test to see if oh is null here...

> +	pwrdm = omap_hwmod_get_pwrdm(oh);
> +	if (!pwrdm)
> +		return -ENODEV;
> +
> +	return pwrdm_get_context_loss_count(pwrdm);
> +}
> diff --git a/arch/arm/plat-omap/include/plat/omap_device.h b/arch/arm/plat-omap/include/plat/omap_device.h
> index 28e2d1a..70d31d0 100644
> --- a/arch/arm/plat-omap/include/plat/omap_device.h
> +++ b/arch/arm/plat-omap/include/plat/omap_device.h
> @@ -107,6 +107,7 @@ void __iomem *omap_device_get_rt_va(struct omap_device *od);
>  int omap_device_align_pm_lat(struct platform_device *pdev,
>  			     u32 new_wakeup_lat_limit);
>  struct powerdomain *omap_device_get_pwrdm(struct omap_device *od);
> +int omap_device_get_context_loss_count(struct platform_device *pdev);
>  
>  /* Other */
>  
> diff --git a/arch/arm/plat-omap/include/plat/omap_hwmod.h b/arch/arm/plat-omap/include/plat/omap_hwmod.h
> index 62bdb23..5a96ac5 100644
> --- a/arch/arm/plat-omap/include/plat/omap_hwmod.h
> +++ b/arch/arm/plat-omap/include/plat/omap_hwmod.h
> @@ -568,6 +568,7 @@ int omap_hwmod_for_each_by_class(const char *classname,
>  				 void *user);
>  
>  int omap_hwmod_set_postsetup_state(struct omap_hwmod *oh, u8 state);
> +int omap_hwmod_get_context_loss_count(struct omap_hwmod *oh);
>  
>  /*
>   * Chip variant-specific hwmod init routines - XXX should be converted
> diff --git a/arch/arm/plat-omap/omap-pm-noop.c b/arch/arm/plat-omap/omap-pm-noop.c
> index 7578366..5a58f97 100644
> --- a/arch/arm/plat-omap/omap-pm-noop.c
> +++ b/arch/arm/plat-omap/omap-pm-noop.c
> @@ -20,9 +20,11 @@
>  #include <linux/init.h>
>  #include <linux/cpufreq.h>
>  #include <linux/device.h>
> +#include <linux/platform_device.h>
>  
>  /* Interface documentation is in mach/omap-pm.h */
>  #include <plat/omap-pm.h>
> +#include <plat/omap_device.h>
>  
>  struct omap_opp *dsp_opps;
>  struct omap_opp *mpu_opps;
> @@ -288,20 +290,19 @@ unsigned long omap_pm_cpu_get_freq(void)
>  
>  int omap_pm_get_dev_context_loss_count(struct device *dev)
>  {
> +	struct platform_device *pdev = to_platform_device(dev);
> +	int count;
> +
>  	if (!dev) {
>  		WARN_ON(1);
>  		return -EINVAL;
>  	};
>  
> -	pr_debug("OMAP PM: returning context loss count for dev %s\n",
> -		 dev_name(dev));
> +	count = omap_device_get_context_loss_count(pdev);
> +	pr_debug("OMAP PM: context loss count for dev %s = %d\n",
> +		 dev_name(dev), count);
>  
> -	/*
> -	 * Map the device to the powerdomain.  Return the powerdomain
> -	 * off counter.
> -	 */
> -
> -	return 0;
> +	return count;
>  }
>  
>  
> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c
> index abe933c..e29c2d6 100644
> --- a/arch/arm/plat-omap/omap_device.c
> +++ b/arch/arm/plat-omap/omap_device.c
> @@ -280,6 +280,26 @@ static void _add_optional_clock_alias(struct omap_device *od,
>  /* Public functions for use by core code */
>  
>  /**
> + * omap_device_get_context_loss_count - get lost context count
> + * @od: struct omap_device *
> + *
> + * Using the primary hwmod for this device, query the context loss
> + * count for this device.

Please document the possible return values here (as above).

> + */
> +int omap_device_get_context_loss_count(struct platform_device *pdev)
> +{
> +	struct omap_device *od;
> +	int ret = -ENODEV;
> +
> +	od = _find_by_pdev(pdev);
> +
> +	if (od->hwmods_cnt)
> +		omap_hwmod_get_context_loss_count(od->hwmods[0]);
> +
> +	return ret;
> +}
> +
> +/**
>   * omap_device_count_resources - count number of struct resource entries needed
>   * @od: struct omap_device *
>   *


- Paul

Re: [PATCH/RFC 3/2] OMAP: PM noop: implement context loss count for non-omap_devices

[Paul Walmsley]

Hi

a few more minor comments

On Thu, 9 Dec 2010, Kevin Hilman wrote:

> For devices which have not (yet) been converted to use omap_device,
> implement the context loss counter using the "brutal method" as
> originally proposed by Paul Walmsley[1].
> 
> The dummy context loss counter is incremented every time it is
> checked, but only when off-mode is enabled.  When off-mode is
> disabled, the dummy counter stops incrementing.
> 
> Tested on 36xx/Zoom3 using MMC driver, which is currently the
> only in-tree user of this API.
> 
> This patch should be reverted after all devices are converted to using
> omap_device.
> 
> [1] http://marc.info/?l=linux-omap&m=129176260000626&w=2
> 
> Cc: Paul Walmsley <paul@pwsan.com>
> Signed-off-by: Kevin Hilman <khilman@deeprootsystems.com>
> ---
>  arch/arm/mach-omap2/pm-debug.c            |    2 ++
>  arch/arm/plat-omap/include/plat/omap-pm.h |    1 +
>  arch/arm/plat-omap/omap-pm-noop.c         |   19 ++++++++++++++++++-
>  3 files changed, 21 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/pm-debug.c b/arch/arm/mach-omap2/pm-debug.c
> index e535082..af66acb 100644
> --- a/arch/arm/mach-omap2/pm-debug.c
> +++ b/arch/arm/mach-omap2/pm-debug.c
> @@ -32,6 +32,7 @@
>  #include "powerdomain.h"
>  #include "clockdomain.h"
>  #include <plat/dmtimer.h>
> +#include <plat/omap-pm.h>
>  
>  #include "cm2xxx_3xxx.h"
>  #include "prm2xxx_3xxx.h"
> @@ -581,6 +582,7 @@ static int option_set(void *data, u64 val)
>  	*option = val;
>  
>  	if (option == &enable_off_mode) {
> +		omap_pm_enable_off_mode(val ? true : false);
>  		if (cpu_is_omap34xx())
>  			omap3_pm_off_mode_enable(val);
>  	}
> diff --git a/arch/arm/plat-omap/include/plat/omap-pm.h b/arch/arm/plat-omap/include/plat/omap-pm.h
> index 9870b4f..cfa5e44 100644
> --- a/arch/arm/plat-omap/include/plat/omap-pm.h
> +++ b/arch/arm/plat-omap/include/plat/omap-pm.h
> @@ -372,5 +372,6 @@ unsigned long omap_pm_cpu_get_freq(void);
>   */
>  int omap_pm_get_dev_context_loss_count(struct device *dev);
>  
> +void omap_pm_enable_off_mode(bool enable);
>  
>  #endif
> diff --git a/arch/arm/plat-omap/omap-pm-noop.c b/arch/arm/plat-omap/omap-pm-noop.c
> index 5a58f97..32f2818 100644
> --- a/arch/arm/plat-omap/omap-pm-noop.c
> +++ b/arch/arm/plat-omap/omap-pm-noop.c
> @@ -30,6 +30,9 @@ struct omap_opp *dsp_opps;
>  struct omap_opp *mpu_opps;
>  struct omap_opp *l3_opps;
>  
> +static bool off_mode_enabled;
> +static u32 dummy_context_loss_counter;
> +
>  /*
>   * Device-driver-originated constraints (via board-*.c files)
>   */
> @@ -287,6 +290,10 @@ unsigned long omap_pm_cpu_get_freq(void)
>  /*
>   * Device context loss tracking
>   */

Please add some basic kerneldoc here -- maybe just document that it's 
meant to be called from the core OMAP PM or PM debug code whenever 
off-mode is enabled or disabled...

> +void omap_pm_enable_off_mode(bool enable)
> +{
> +	off_mode_enabled = enable;
> +}

Personal preference: please split this into two functions, 
omap_pm_{enable,disable}_off_mode(void).  That way, when I read it in 
other code, I don't have to try to remember what the argument means :-)

>  int omap_pm_get_dev_context_loss_count(struct device *dev)
>  {
> @@ -298,7 +305,17 @@ int omap_pm_get_dev_context_loss_count(struct device *dev)
>  		return -EINVAL;
>  	};
>  
> -	count = omap_device_get_context_loss_count(pdev);
> +	if (dev->parent == &omap_device_parent) {
> +		count = omap_device_get_context_loss_count(pdev);
> +	} else {
> +		WARN_ONCE(off_mode_enabled, "using dummy context loss counter, "
> +			  "device %s should be converted to omap_device.",
> +			  __func__, dev_name(dev));
> +		if (off_mode_enabled)
> +			dummy_context_loss_counter++;
> +		count = dummy_context_loss_counter;
> +	}
> +

This part looks fine to me, thanks for working on this stuff.  Once you 
fix the arithmetic rollover stuff, I think it will be good for 2.6.38.


- Paul