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

2010-12-14 Thread 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 p...@pwsan.com
 Signed-off-by: Kevin Hilman khil...@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

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

2010-12-13 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@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

2010-12-13 Thread Vishwanath Sripathy 
Paul,

 -Original Message-
 From: Paul Walmsley [mailto:p...@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-
 o...@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-
 o...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@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

2010-12-13 Thread 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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@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

2010-12-10 Thread Vishwanath Sripathy 
Kevin,

 -Original Message-
 From: linux-omap-ow...@vger.kernel.org [mailto:linux-omap-
 ow...@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 p...@pwsan.com
 Signed-off-by: Kevin Hilman khil...@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

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

2010-12-10 Thread Vishwanath Sripathy 
Kevin,

 -Original Message-
 From: Kevin Hilman [mailto:khil...@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...@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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html