Re: [PATCH 4/7] ARM: OMAP2+: powerdomain: introduce logic for finding valid power domain

2014-08-27 Thread Kevin Hilman
Nishanth Menon n...@ti.com writes:

 powerdomain configuration in OMAP is done using PWRSTCTRL register for
 each power domain. However, PRCM lets us write any value we'd like to
 the logic and power domain target states, however the SoC integration
 tends to actually function only at a few discrete states. These valid
 states are already in our powerdomains_xxx_data.c file.

 So, provide a function to easily query valid low power state that the
 power domain is allowed to go to.

 Based on work originally done by Jean Pihet j-pi...@ti.com
 https://patchwork.kernel.org/patch/1325091/ . There is no attempt to
 create a new powerdomain solution here, except fixing issues seen
 attempting invalid programming attempts. Future consolidation to the
 generic powerdomain framework should consider this requirement as
 well.

 Similar solutions have been done in product kernels in the past such
 as:
 https://android.googlesource.com/kernel/omap.git/+blame/android-omap-panda-3.0/arch/arm/mach-omap2/pm44xx.c

 Signed-off-by: Nishanth Menon n...@ti.com
 ---

nit: this is part of a fixes series, but it's more of a new feature.

That being said, the feature is needed and looks OK, except for...

 +up_search:
 + /* OK, no deeper ones, can we get a higher match? */
 + new_pwrst = req_state + 1;
 + while (!(pwrdm_states  BIT(new_pwrst))) {
 + /* BUG if we have messed up database */
 + BUG_ON(new_pwrst  PWRDM_POWER_ON);

I don't think this is BUG() worthy, and should have a saner way to recover.

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


Re: [PATCH 4/7] ARM: OMAP2+: powerdomain: introduce logic for finding valid power domain

2014-08-27 Thread Nishanth Menon
On Wed, Aug 27, 2014 at 1:27 PM, Kevin Hilman
khil...@deeprootsystems.com wrote:
 Nishanth Menon n...@ti.com writes:

 powerdomain configuration in OMAP is done using PWRSTCTRL register for
 each power domain. However, PRCM lets us write any value we'd like to
 the logic and power domain target states, however the SoC integration
 tends to actually function only at a few discrete states. These valid
 states are already in our powerdomains_xxx_data.c file.

 So, provide a function to easily query valid low power state that the
 power domain is allowed to go to.

 Based on work originally done by Jean Pihet j-pi...@ti.com
 https://patchwork.kernel.org/patch/1325091/ . There is no attempt to
 create a new powerdomain solution here, except fixing issues seen
 attempting invalid programming attempts. Future consolidation to the
 generic powerdomain framework should consider this requirement as
 well.

 Similar solutions have been done in product kernels in the past such
 as:
 https://android.googlesource.com/kernel/omap.git/+blame/android-omap-panda-3.0/arch/arm/mach-omap2/pm44xx.c

 Signed-off-by: Nishanth Menon n...@ti.com
 ---

 nit: this is part of a fixes series, but it's more of a new feature.

 That being said, the feature is needed and looks OK, except for...

 +up_search:
 + /* OK, no deeper ones, can we get a higher match? */
 + new_pwrst = req_state + 1;
 + while (!(pwrdm_states  BIT(new_pwrst))) {
 + /* BUG if we have messed up database */
 + BUG_ON(new_pwrst  PWRDM_POWER_ON);

 I don't think this is BUG() worthy, and should have a saner way to recover.

it is not even a legal value to have a power state higher than ON. I
mean, yeah, we can do
if (new_pwrst  PWRDM_POWER_ON) {
 pr_debug(powerdomain: %s: fix my powerdomain max to ON\n,
pwrdm-name);
 return PWRDM_POWER_ON;
}

if that is your suggestion here, personally, I would use a WARN at least here..

-- 
---
Regards,
Nishanth Menon
--
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 4/7] ARM: OMAP2+: powerdomain: introduce logic for finding valid power domain

2014-08-27 Thread Kevin Hilman
On Wed, Aug 27, 2014 at 11:35 AM, Nishanth Menon n...@ti.com wrote:
 On Wed, Aug 27, 2014 at 1:27 PM, Kevin Hilman
 khil...@deeprootsystems.com wrote:
 Nishanth Menon n...@ti.com writes:

 powerdomain configuration in OMAP is done using PWRSTCTRL register for
 each power domain. However, PRCM lets us write any value we'd like to
 the logic and power domain target states, however the SoC integration
 tends to actually function only at a few discrete states. These valid
 states are already in our powerdomains_xxx_data.c file.

 So, provide a function to easily query valid low power state that the
 power domain is allowed to go to.

 Based on work originally done by Jean Pihet j-pi...@ti.com
 https://patchwork.kernel.org/patch/1325091/ . There is no attempt to
 create a new powerdomain solution here, except fixing issues seen
 attempting invalid programming attempts. Future consolidation to the
 generic powerdomain framework should consider this requirement as
 well.

 Similar solutions have been done in product kernels in the past such
 as:
 https://android.googlesource.com/kernel/omap.git/+blame/android-omap-panda-3.0/arch/arm/mach-omap2/pm44xx.c

 Signed-off-by: Nishanth Menon n...@ti.com
 ---

 nit: this is part of a fixes series, but it's more of a new feature.

 That being said, the feature is needed and looks OK, except for...

 +up_search:
 + /* OK, no deeper ones, can we get a higher match? */
 + new_pwrst = req_state + 1;
 + while (!(pwrdm_states  BIT(new_pwrst))) {
 + /* BUG if we have messed up database */
 + BUG_ON(new_pwrst  PWRDM_POWER_ON);

 I don't think this is BUG() worthy, and should have a saner way to recover.

 it is not even a legal value to have a power state higher than ON. I
 mean, yeah, we can do
 if (new_pwrst  PWRDM_POWER_ON) {
  pr_debug(powerdomain: %s: fix my powerdomain max to ON\n,
 pwrdm-name);
  return PWRDM_POWER_ON;
 }

 if that is your suggestion here, personally, I would use a WARN at least 
 here..

WARN, pr_warn() as you like.

The point is that BUG* calls panic() and locks up the system tight.
As what your'e adding is not fatal to the entire system, you should
not be using bug.  From asm-generic/bug.h:

*
 * Don't use BUG() or BUG_ON() unless there's really no way out; one
 * example might be detecting data structure corruption in the middle
 * of an operation that can't be backed out of.  If the (sub)system
 * can somehow continue operating, perhaps with reduced functionality,
 * it's probably not BUG-worthy.
 *
 * If you're tempted to BUG(), think again:  is completely giving up
 * really the *only* solution?  There are usually better options, where
 * users don't need to reboot ASAP and can mostly shut down cleanly.
 */
--
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


[PATCH 4/7] ARM: OMAP2+: powerdomain: introduce logic for finding valid power domain

2014-08-22 Thread Nishanth Menon
powerdomain configuration in OMAP is done using PWRSTCTRL register for
each power domain. However, PRCM lets us write any value we'd like to
the logic and power domain target states, however the SoC integration
tends to actually function only at a few discrete states. These valid
states are already in our powerdomains_xxx_data.c file.

So, provide a function to easily query valid low power state that the
power domain is allowed to go to.

Based on work originally done by Jean Pihet j-pi...@ti.com
https://patchwork.kernel.org/patch/1325091/ . There is no attempt to
create a new powerdomain solution here, except fixing issues seen
attempting invalid programming attempts. Future consolidation to the
generic powerdomain framework should consider this requirement as
well.

Similar solutions have been done in product kernels in the past such
as:
https://android.googlesource.com/kernel/omap.git/+blame/android-omap-panda-3.0/arch/arm/mach-omap2/pm44xx.c

Signed-off-by: Nishanth Menon n...@ti.com
---
 arch/arm/mach-omap2/powerdomain.c |   73 +
 arch/arm/mach-omap2/powerdomain.h |3 ++
 2 files changed, 76 insertions(+)

diff --git a/arch/arm/mach-omap2/powerdomain.c 
b/arch/arm/mach-omap2/powerdomain.c
index f391948..831a2bc 100644
--- a/arch/arm/mach-omap2/powerdomain.c
+++ b/arch/arm/mach-omap2/powerdomain.c
@@ -1081,6 +1081,79 @@ int pwrdm_post_transition(struct powerdomain *pwrdm)
 }
 
 /**
+ * pwrdm_get_valid_lp_state() - Find best match deep power state
+ * @pwrdm: power domain for which we want to find best match
+ * @is_logic_state: Are we looking for logic state match here? Should
+ * be one of PWRDM_xxx macro values
+ * @req_state: requested power state
+ *
+ * Returns: closest match for requested power state. default fallback
+ * is RET for logic state and ON for power state.
+ *
+ * This does a search from the power domain data looking for the
+ * closest valid power domain state that the hardware can achieve.
+ * PRCM definitions for PWRSTCTRL allows us to program whatever
+ * configuration we'd like, and PRCM will actually attempt such
+ * a transition, however if the powerdomain does not actually support it,
+ * we endup with a hung system. The valid power domain states are already
+ * available in our powerdomain data files. So this function tries to do
+ * the following:
+ * a) find if we have an exact match to the request - no issues.
+ * b) else find if a deeper power state is possible.
+ * c) failing which, it tries to find closest higher power state for the
+ * request.
+ */
+u8 pwrdm_get_valid_lp_state(struct powerdomain *pwrdm,
+   bool is_logic_state, u8 req_state)
+{
+   u8 pwrdm_states = is_logic_state ? pwrdm-pwrsts_logic_ret :
+   pwrdm-pwrsts;
+   /* For logic, ret is highest and others, ON is highest */
+   u8 default_pwrst = is_logic_state ? PWRDM_POWER_RET : PWRDM_POWER_ON;
+   u8 new_pwrst;
+   bool found;
+
+   /* If it is already supported, nothing to search */
+   if (pwrdm_states  BIT(req_state))
+   return req_state;
+
+   if (!req_state)
+   goto up_search;
+
+   /*
+* So, we dont have a exact match
+* Can we get a deeper power state match?
+*/
+   new_pwrst = req_state - 1;
+   found = true;
+   while (!(pwrdm_states  BIT(new_pwrst))) {
+   /* No match even at OFF? Not available */
+   if (new_pwrst == PWRDM_POWER_OFF) {
+   found = false;
+   break;
+   }
+   new_pwrst--;
+   }
+
+   if (found)
+   goto done;
+
+up_search:
+   /* OK, no deeper ones, can we get a higher match? */
+   new_pwrst = req_state + 1;
+   while (!(pwrdm_states  BIT(new_pwrst))) {
+   /* BUG if we have messed up database */
+   BUG_ON(new_pwrst  PWRDM_POWER_ON);
+
+   if (new_pwrst == default_pwrst)
+   break;
+   new_pwrst++;
+   }
+done:
+   return new_pwrst;
+}
+
+/**
  * omap_set_pwrdm_state - change a powerdomain's current power state
  * @pwrdm: struct powerdomain * to change the power state of
  * @pwrst: power state to change to
diff --git a/arch/arm/mach-omap2/powerdomain.h 
b/arch/arm/mach-omap2/powerdomain.h
index a754c82..11bd4dd 100644
--- a/arch/arm/mach-omap2/powerdomain.h
+++ b/arch/arm/mach-omap2/powerdomain.h
@@ -220,6 +220,9 @@ struct voltagedomain *pwrdm_get_voltdm(struct powerdomain 
*pwrdm);
 
 int pwrdm_get_mem_bank_count(struct powerdomain *pwrdm);
 
+u8 pwrdm_get_valid_lp_state(struct powerdomain *pwrdm,
+   bool is_logic_state, u8 req_state);
+
 int pwrdm_set_next_pwrst(struct powerdomain *pwrdm, u8 pwrst);
 int pwrdm_read_next_pwrst(struct powerdomain *pwrdm);
 int pwrdm_read_pwrst(struct powerdomain *pwrdm);
-- 
1.7.9.5

--
To unsubscribe from this list: send