[PATCH 0/9 v2] omap3: pm: introduce support for 3630 OPPs

[PATCH 0/9 v2] omap3: pm: introduce support for 3630 OPPs

[Nishanth Menon]

Hi,
V2 of the patch series.
What changed in V2:
* Rebased to latest pm branch as of today (with zoom3 and SDP3630
  support -thanks kevin and Vikram in getting it done) 8/9 changed
* Jon Hunter pointed me to errors in vdd2 opp2 voltage that I made
  a mistake-9/9 changed
* Lam pointed me to an error in OPP enable for 3630OPP I made-
  vdd1 opp3 was disabled by default in DSP but I missed MPU :( -
  9/9 changed
* Accepted Kevin's recommendation that I rename opp_onoff to
  opp_enable 2/9 changed
* I realized that macro paramaters usage in IS_OPP_TERMINATOR,
  so made them mess up safe (2/9 changed)
* Few of the patch subjects got renamed from "introduce opp accessor
  functions" to "use accessor function" to better reflect what was
  being done.

Finally,
This series has been tested on SDP3430 and SDP3630 :) - though
not stress tested for various drivers - basic OPP changes on VDD1 &
VDD2, and cpufreq tests only. Requesting more thorough testing from
as many folks as possible. (omap3_pm_defconfig used)
NOTE: SDP3630 needs the patch for 8250 as discussed here:
http://marc.info/?l=linux-omap&m=125795354108797&w=2

I am reposting the complete rebased set as v2 to maintain continuity
on the new baseline

 ====
This patch series is based on previous discussions:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg17632.html

I have modified the initial patch From Sanjeev:
http://patchwork.kernel.org/patch/50998/

Other than these, we cannot use the old VDDx_MAX based usage
anymore as 3630 OPPs are now different and runtime handling
is a must-have, hence introducing these accessor functions is not
avoidable.

The changes are incremental and tries to avoid intrusive change
as much as possible.

The OPP accessor functions introduced in this series is hopefully
a start for us to optimize this heavily used path.

Finally, I have only done limited testing of SR, as it is still in
my TODO list to revamp SR in the form of a v5 patch next time I get
some bandwidth.

An alternate approach for detecting 3630 OPP is using FEATURES
instead of detecting the cpu_type as I have implemented in this series.

Nishanth Menon (9):
  omap3: pm: introduce enabled flag to omap_opp
  omap3: pm: introduce opp accessor functions
  omap3: pm: srf: use opp accessor function
  omap3: pm: use opp accessor functions for omap-target
  omap3: pm: sr: replace get_opp with freq_to_opp
  omap3: clk: use pm accessor functions for cpufreq table
  omap3: pm: remove VDDx_MIN/MAX macros
  omap3: pm: introduce dynamic OPP
  omap3: pm: introduce 3630 opps

 arch/arm/mach-omap2/board-3430sdp.c        |    1 +
 arch/arm/mach-omap2/board-3630sdp.c        |    6 +-
 arch/arm/mach-omap2/board-omap3beagle.c    |    1 +
 arch/arm/mach-omap2/board-omap3evm.c       |    1 +
 arch/arm/mach-omap2/board-rx51.c           |    1 +
 arch/arm/mach-omap2/board-zoom2.c          |    7 +-
 arch/arm/mach-omap2/board-zoom3.c          |    6 +-
 arch/arm/mach-omap2/clock34xx.c            |   46 +++++---
 arch/arm/mach-omap2/omap3-opp.h            |   49 ++------
 arch/arm/mach-omap2/pm.c                   |  160 +++++++++++++++++++++++++
 arch/arm/mach-omap2/pm.h                   |    7 +
 arch/arm/mach-omap2/pm34xx.c               |  122 +++++++++++++++++++
 arch/arm/mach-omap2/resource34xx.c         |  174 +++++++++++++++++----------
 arch/arm/mach-omap2/smartreflex.c          |   36 ++-----
 arch/arm/plat-omap/cpu-omap.c              |   12 +--
 arch/arm/plat-omap/include/plat/omap-pm.h  |  111 ++++++++++++++++++
 arch/arm/plat-omap/include/plat/omap34xx.h |    5 -
 17 files changed, 584 insertions(+), 161 deletions(-)

Regards,
Nishanth Menon

PS: Sample log from 3630SDP:
 3630SDP cpufreq results:
 # head /sys/devices/system/cpu/cpu0/cpu
 /sys/devices/system/cpu/cpu0/cpufreq/  /sys/devices/system/cpu/cpu0/cpuidle/
 /tests/pm-test-scripts # head /sys/devices/system/cpu/cpu0/cpufreq/*
 ==> /sys/devices/system/cpu/cpu0/cpufreq/affected_cpus <==
 0
 
 ==> /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_cur_freq <==
 300000
 
 ==> /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq <==
 600000
 
 ==> /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_min_freq <==
 300000
 
 ==> /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_transition_latency <==
 300000
 
 ==> /sys/devices/system/cpu/cpu0/cpufreq/related_cpus <==
 0
 
 ==> /sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies <==
 600000 300000 
 
 ==> /sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors <==
 userspace 
 
 ==> /sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq <==
 300000

 ==> /sys/devices/system/cpu/cpu0/cpufreq/scaling_driver <==
 omap

 ==> /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor <==
 userspace

 ==> /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq <==
 600000

 ==> /sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq <==
 300000

 ==> /sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed <==
 300000

 # head /sys/devices/system/cpu/cpu0/cpufreq/stats/*
 ==> /sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state <==
 600000 9246
 300000 159753

 ==> /sys/devices/system/cpu/cpu0/cpufreq/stats/total_trans <==
 13
 [..]
 /dbg/pm_debug# cat voltage_off_while_idle 
 0
 /dbg/pm_debug # cat enable_off_mode 
 0
 /dbg/pm_debug # echo -n '1' >enable_off_mode 
 Unable to Changelevel for resource dsp_freq to 0
 Error: could not refresh resources
 /dbg/pm_debug # echo -n '1' >voltage_off_while_idle 
 /dbg/pm_debug # cat enable_off_mode 
 1
 /dbg/pm_debug # echo 'mem' >/sys/power/state 
 PM: Syncing filesystems ... done.
 Freezing user space processes ... (elapsed 0.00 seconds) done.
 Freezing remaining freezable tasks ... (elapsed 0.00 seconds) done.
 Suspending console(s) (use no_console_suspend to debug)
 Successfully put all powerdomains to target state
 Restarting tasks ... done.
 /dbg/pm_debug # 
 /dbg/pm_debug # echo 'mem' >/sys/power/state 
 PM: Syncing filesystems ... done.
 Freezing user space processes ... (elapsed 0.00 seconds) done.
 Freezing remaining freezable tasks ... (elapsed 0.00 seconds) done.
 Suspending console(s) (use no_console_suspend to debug)
 Successfully put all powerdomains to target state
 Restarting tasks ... done.
 /dbg/pm_debug # 
 /dbg/pm_debug # 
 /dbg/pm_debug # ls
 cam_pwrdm               iva2_pwrdm              sleep_while_idle
 core_pwrdm              mpu_pwrdm               time
 count                   neon_pwrdm              usbhost_pwrdm
 dss_pwrdm               per_pwrdm               voltage_off_while_idle
 emu_pwrdm               registers               wakeup_timer_seconds
 enable_off_mode         sgx_pwrdm               wkup_pwrdm
 /dbg/pm_debug # cat count time
 usbhost_pwrdm (OFF),OFF:1,RET:1,INA:0,ON:1
 sgx_pwrdm (OFF),OFF:1,RET:0,INA:0,ON:1
 per_pwrdm (ON),OFF:2,RET:143,INA:0,ON:146
 dss_pwrdm (OFF),OFF:1,RET:1,INA:0,ON:1
 cam_pwrdm (OFF),OFF:1,RET:1,INA:0,ON:1
 core_pwrdm (ON),OFF:2,RET:1,INA:0,ON:4
 neon_pwrdm (ON),OFF:2,RET:1,INA:190,ON:194
 mpu_pwrdm (ON),OFF:2,RET:1,INA:190,ON:194
 iva2_pwrdm (OFF),OFF:1,RET:1,INA:0,ON:1
 per_clkdm->per_pwrdm (9)
 usbhost_clkdm->usbhost_pwrdm (0)
 cam_clkdm->cam_pwrdm (0)
 dss_clkdm->dss_pwrdm (0)
 core_l4_clkdm->core_pwrdm (5)
 core_l3_clkdm->core_pwrdm (4)
 d2d_clkdm->core_pwrdm (0)
 sgx_clkdm->sgx_pwrdm (0)
 iva2_clkdm->iva2_pwrdm (0)
 neon_clkdm->neon_pwrdm (0)
 mpu_clkdm->mpu_pwrdm (0)
 prm_clkdm->wkup_pwrdm (0)
 cm_clkdm->core_pwrdm (0)
 usbhost_pwrdm (OFF),OFF:47586242676,RET:150255279541,INA:0,ON:22111267089
 sgx_pwrdm (OFF),OFF:197841491699,RET:0,INA:0,ON:22111297607
 per_pwrdm (ON),OFF:4080291748,RET:5557220473,INA:0,ON:210315277085
 dss_pwrdm (OFF),OFF:47586303711,RET:150255249024,INA:0,ON:22111267089
 cam_pwrdm (OFF),OFF:47586303711,RET:150255249024,INA:0,ON:22111267089
 core_pwrdm (ON),OFF:4080291748,RET:2764923095,INA:0,ON:213107604981
 neon_pwrdm (ON),OFF:4080291748,RET:2764923095,INA:5508361840,ON:207599273658
 mpu_pwrdm (ON),OFF:4080291748,RET:2764923095,INA:5508972194,ON:207598663304
 iva2_pwrdm (OFF),OFF:47586364746,RET:150255187988,INA:0,ON:22111297607
 /dbg/pm_debug # 

[PATCH 1/9] omap3: pm: introduce enabled flag to omap_opp

[Nishanth Menon]

We used to enable and disable OPPs based on rate being set to 0, this
has been confusing in general. So, we now allow specific OPPs to be
now enabled/disabled by an explicit enabled flag instead of re-using
rate flag itself.

Tested on: SDP3430, SDP3630

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Romit Dasgupta <romit@ti.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
Cc: SuiLun Lam <s-lam@ti.com>
Cc: Thara Gopinath <thara@ti.com>
Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/omap3-opp.h           |   32 ++++++++++++++--------------
 arch/arm/mach-omap2/resource34xx.c        |    4 +++
 arch/arm/plat-omap/include/plat/omap-pm.h |    2 +
 3 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
index c773ea7..42557e1 100644
--- a/arch/arm/mach-omap2/omap3-opp.h
+++ b/arch/arm/mach-omap2/omap3-opp.h
@@ -22,41 +22,41 @@
 #define S166M   166000000
 
 static struct omap_opp omap3_mpu_rate_table[] = {
-	{0, 0, 0},
+	{0, 0, 0, 0},
 	/*OPP1*/
-	{S125M, VDD1_OPP1, 0x1E},
+	{true, S125M, VDD1_OPP1, 0x1E},
 	/*OPP2*/
-	{S250M, VDD1_OPP2, 0x26},
+	{true, S250M, VDD1_OPP2, 0x26},
 	/*OPP3*/
-	{S500M, VDD1_OPP3, 0x30},
+	{true, S500M, VDD1_OPP3, 0x30},
 	/*OPP4*/
-	{S550M, VDD1_OPP4, 0x36},
+	{true, S550M, VDD1_OPP4, 0x36},
 	/*OPP5*/
-	{S600M, VDD1_OPP5, 0x3C},
+	{true, S600M, VDD1_OPP5, 0x3C},
 };
 
 static struct omap_opp omap3_l3_rate_table[] = {
-	{0, 0, 0},
+	{0, 0, 0, 0},
 	/*OPP1*/
-	{0, VDD2_OPP1, 0x1E},
+	{false, 0, VDD2_OPP1, 0x1E},
 	/*OPP2*/
-	{S83M, VDD2_OPP2, 0x24},
+	{true, S83M, VDD2_OPP2, 0x24},
 	/*OPP3*/
-	{S166M, VDD2_OPP3, 0x2C},
+	{true, S166M, VDD2_OPP3, 0x2C},
 };
 
 static struct omap_opp omap3_dsp_rate_table[] = {
-	{0, 0, 0},
+	{0, 0, 0, 0},
 	/*OPP1*/
-	{S90M, VDD1_OPP1, 0x1E},
+	{true, S90M, VDD1_OPP1, 0x1E},
 	/*OPP2*/
-	{S180M, VDD1_OPP2, 0x26},
+	{true, S180M, VDD1_OPP2, 0x26},
 	/*OPP3*/
-	{S360M, VDD1_OPP3, 0x30},
+	{true, S360M, VDD1_OPP3, 0x30},
 	/*OPP4*/
-	{S400M, VDD1_OPP4, 0x36},
+	{true, S400M, VDD1_OPP4, 0x36},
 	/*OPP5*/
-	{S430M, VDD1_OPP5, 0x3C},
+	{true, S430M, VDD1_OPP5, 0x3C},
 };
 
 #endif
diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c
index 04be4d2..af6b3c1 100644
--- a/arch/arm/mach-omap2/resource34xx.c
+++ b/arch/arm/mach-omap2/resource34xx.c
@@ -285,6 +285,10 @@ static int program_opp(int res, struct omap_opp *opp, int target_level,
 	c_opp = ID_VDD(res) | ID_OPP_NO(opp[current_level].opp_id);
 #endif
 
+	/* Only allow enabled OPPs */
+	if (!opp[target_level].enabled)
+		return -EINVAL;
+
 	/* Sanity check of the OPP params before attempting to set */
 	if (!opp[target_level].rate || !opp[target_level].vsel)
 		return -EINVAL;
diff --git a/arch/arm/plat-omap/include/plat/omap-pm.h b/arch/arm/plat-omap/include/plat/omap-pm.h
index 583e540..5dc2048 100644
--- a/arch/arm/plat-omap/include/plat/omap-pm.h
+++ b/arch/arm/plat-omap/include/plat/omap-pm.h
@@ -21,6 +21,7 @@
 
 /**
  * struct omap_opp - clock frequency-to-OPP ID table for DSP, MPU
+ * @enabled: enabled if true, disabled if false
  * @rate: target clock rate
  * @opp_id: OPP ID
  * @min_vdd: minimum VDD1 voltage (in millivolts) for this OPP
@@ -28,6 +29,7 @@
  * Operating performance point data.  Can vary by OMAP chip and board.
  */
 struct omap_opp {
+	bool enabled;
 	unsigned long rate;
 	u8 opp_id;
 	u16 vsel;
-- 
1.6.3.3

[PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Nishanth Menon]

Modifies the initial patch From Sanjeev:
http://patchwork.kernel.org/patch/50998/

NOTE:
The original opp_table introduced by Sanjeev is not needed anymore as
we can use enabled flag to have better granularity in enable/disable
of OPPs.

This introduces the following accessor functions:

freq_to_opp and opp_to_freq: Matching functions to convert OPP to freq
and viceversa.

freq_to_vsel: Converts a frequency to corresponding voltage.

opp_enable: To enable/disable a specific OPP in a OPP table this allows
granular runtime disable/enable of specific OPPs, esp when used in
conjunction with search and mapping functions

get_next_freq: A search function to get next matching frequency. This
could possibly provide the basis for more complex OPP transition algos
of the future.

get_limit_freq: A search function to get the least or maximum
frequency based on search criteria. Allows for independence from
OPP_IDs in the future.

Since the accessor functions hide the details of the table
implementation, the opp table is now moved away from omap3-opp.h to
pm34xx.c. The terminator entry is needed at the start and end of the
table as it is still needed for reverse and forward search as the
length of the table is unknown.

Tests done: Accessor functions standalone tested on a PC host with
dummy OPP table to simulate boundary, invalid and valid conditions,
SDP3430, SDP3630 for system stability.

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Romit Dasgupta <romit@ti.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
Cc: SuiLun Lam <s-lam@ti.com>
Cc: Thara Gopinath <thara@ti.com>
Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Signed-off-by: Sanjeev Premi <premi@ti.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/omap3-opp.h           |   40 +-------
 arch/arm/mach-omap2/pm.c                  |  160 +++++++++++++++++++++++++++++
 arch/arm/mach-omap2/pm34xx.c              |   42 ++++++++
 arch/arm/plat-omap/include/plat/omap-pm.h |  109 ++++++++++++++++++++
 4 files changed, 314 insertions(+), 37 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
index 42557e1..27e2ca5 100644
--- a/arch/arm/mach-omap2/omap3-opp.h
+++ b/arch/arm/mach-omap2/omap3-opp.h
@@ -21,42 +21,8 @@
 #define S83M    83000000
 #define S166M   166000000
 
-static struct omap_opp omap3_mpu_rate_table[] = {
-	{0, 0, 0, 0},
-	/*OPP1*/
-	{true, S125M, VDD1_OPP1, 0x1E},
-	/*OPP2*/
-	{true, S250M, VDD1_OPP2, 0x26},
-	/*OPP3*/
-	{true, S500M, VDD1_OPP3, 0x30},
-	/*OPP4*/
-	{true, S550M, VDD1_OPP4, 0x36},
-	/*OPP5*/
-	{true, S600M, VDD1_OPP5, 0x3C},
-};
-
-static struct omap_opp omap3_l3_rate_table[] = {
-	{0, 0, 0, 0},
-	/*OPP1*/
-	{false, 0, VDD2_OPP1, 0x1E},
-	/*OPP2*/
-	{true, S83M, VDD2_OPP2, 0x24},
-	/*OPP3*/
-	{true, S166M, VDD2_OPP3, 0x2C},
-};
-
-static struct omap_opp omap3_dsp_rate_table[] = {
-	{0, 0, 0, 0},
-	/*OPP1*/
-	{true, S90M, VDD1_OPP1, 0x1E},
-	/*OPP2*/
-	{true, S180M, VDD1_OPP2, 0x26},
-	/*OPP3*/
-	{true, S360M, VDD1_OPP3, 0x30},
-	/*OPP4*/
-	{true, S400M, VDD1_OPP4, 0x36},
-	/*OPP5*/
-	{true, S430M, VDD1_OPP5, 0x3C},
-};
+extern struct omap_opp omap3_mpu_rate_table[];
+extern struct omap_opp omap3_dsp_rate_table[];
+extern struct omap_opp omap3_l3_rate_table[];
 
 #endif
diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
index f50e93d..b2cd30c 100644
--- a/arch/arm/mach-omap2/pm.c
+++ b/arch/arm/mach-omap2/pm.c
@@ -33,6 +33,7 @@
 #include <plat/powerdomain.h>
 #include <plat/resource.h>
 #include <plat/omap34xx.h>
+#include <plat/omap-pm.h>
 
 #include "prm-regbits-34xx.h"
 #include "pm.h"
@@ -203,3 +204,162 @@ static int __init omap_pm_init(void)
 	return error;
 }
 late_initcall(omap_pm_init);
+
+int opp_to_freq(unsigned long *freq, const struct omap_opp *opps, u8 opp_id)
+{
+	int i = 1;
+
+	BUG_ON(!freq || !opps);
+
+	/* The first entry is a dummy one, loop till we hit terminator */
+	while (!IS_OPP_TERMINATOR(opps, i)) {
+		if (opps[i].enabled && (opps[i].opp_id == opp_id)) {
+			*freq = opps[i].rate;
+			return 0;
+		}
+		i++;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(opp_to_freq);
+
+int freq_to_vsel(u8 *vsel, const struct omap_opp *opps, unsigned long freq)
+{
+	int i = 1;
+
+	BUG_ON(!vsel || !opps);
+
+	/* The first entry is a dummy one, loop till we hit terminator */
+	while (!IS_OPP_TERMINATOR(opps, i)) {
+		if (opps[i].enabled && (opps[i].rate == freq)) {
+			*vsel = opps[i].vsel;
+			return 0;
+		}
+		i++;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(freq_to_vsel);
+
+int freq_to_opp(u8 *opp_id, const struct omap_opp *opps, unsigned long freq)
+{
+	int i = 1;
+
+	BUG_ON(!opp_id || !opps);
+
+	/* The first entry is a dummy one, loop till we hit terminator */
+	while (!IS_OPP_TERMINATOR(opps, i)) {
+		if (opps[i].enabled && (opps[i].rate == freq)) {
+			*opp_id = opps[i].opp_id;
+			return 0;
+		}
+		i++;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(freq_to_opp);
+
+int opp_enable(struct omap_opp *opps, unsigned long freq, bool enable)
+{
+	int i = 1;
+
+	BUG_ON(!opps);
+
+	/* The first entry is a dummy one, loop till we hit terminator */
+	while (!IS_OPP_TERMINATOR(opps, i)) {
+		if (opps[i].rate == freq) {
+			opps[i].enabled = enable;
+			return 0;
+		}
+		i++;
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(opp_enable);
+
+int get_next_freq(unsigned long *freq, const struct omap_opp *opps,
+		bool search_higher, bool search_enabled_only, bool exact_search)
+{
+	int i = 1, inc = 1;
+	bool found = false;
+
+	BUG_ON(!opps || !freq || !(*freq));
+
+	/* The first entry is a dummy one, loop till we hit terminator
+	 * XXX: The following algorithm works only on a presorted
+	 * list of OPPs
+	 */
+	while (!IS_OPP_TERMINATOR(opps, i)) {
+		/* if we found the original freq, then
+		 * case 1: enabled search ONLY, check opp is enabled or not
+		 * case 2: the next available freq if enabled is not searched
+		 */
+		if ((found && search_enabled_only && opps[i].enabled) ||
+		    (found && !search_enabled_only)) {
+			*freq = opps[i].rate;
+			return 0;
+		}
+
+		/* Find the baseline freq first.. */
+		if (!found && ((exact_search && opps[i].rate == *freq) ||
+				(!exact_search && opps[i].rate >= *freq))) {
+			/* found.. next decide direction */
+			inc = search_higher ? 1 : -1;
+			found = true;
+			/* handle an exception case for exact search.. */
+			if (exact_search || !search_higher)
+				i += inc;
+			/* fall thru to search for the right match */
+		} else {
+			i += inc;
+		}
+	}
+
+	return -EINVAL;
+}
+EXPORT_SYMBOL(get_next_freq);
+
+int get_limit_freq(unsigned long *freq, const struct omap_opp *opps,
+		bool search_highest, bool search_enabled_only)
+{
+	int i = 1;
+	unsigned long cur_freq_match = search_highest ? 0 : -1;
+	bool found = false;
+
+	BUG_ON(!opps || !freq);
+
+	/* The first entry is a dummy one, loop till we hit terminator
+	 * XXX: The following algorithm works only on a presorted
+	 * list of OPPs
+	 * We could use get_next_freq to search, but that will tend
+	 * to be inefficient
+	 */
+	while (!IS_OPP_TERMINATOR(opps, i)) {
+		/* match condition:
+		 * check if the enabled cases match (only invalid case is:
+		 * search_enabled=1,enabled=0)
+		 * then we look for comparison condition, based on direction
+		 */
+		if (!(search_enabled_only && !opps[i].enabled) &&
+		     ((search_highest && (opps[i].rate > cur_freq_match)) ||
+		     (!search_highest && (opps[i].rate < cur_freq_match)))) {
+			cur_freq_match = opps[i].rate;
+			found = true;
+			/* if we are searching for least, the first match
+			 * is the right one, look no further.
+			 */
+			if (!search_highest)
+				break;
+		}
+		i++;
+	}
+	if (!found)
+		return -EINVAL;
+	*freq = cur_freq_match;
+	return 0;
+}
+EXPORT_SYMBOL(get_limit_freq);
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 9e471f7..2f17a40 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -51,6 +51,7 @@
 #include "prm.h"
 #include "pm.h"
 #include "sdrc.h"
+#include "omap3-opp.h"
 
 static int regset_save_on_suspend;
 
@@ -99,6 +100,47 @@ static struct prm_setup_vc prm_setup = {
 	.vdd1_off = 0x00,	/* 0.6v */
 };
 
+struct omap_opp omap3_mpu_rate_table[] = {
+	{0, 0, 0, 0},
+	/*OPP1*/
+	{true, S125M, VDD1_OPP1, 0x1E},
+	/*OPP2*/
+	{true, S250M, VDD1_OPP2, 0x26},
+	/*OPP3*/
+	{true, S500M, VDD1_OPP3, 0x30},
+	/*OPP4*/
+	{true, S550M, VDD1_OPP4, 0x36},
+	/*OPP5*/
+	{true, S600M, VDD1_OPP5, 0x3C},
+	{0, 0, 0, 0},
+};
+
+struct omap_opp omap3_l3_rate_table[] = {
+	{0, 0, 0, 0},
+	/*OPP1*/
+	{false, 0, VDD2_OPP1, 0x1E},
+	/*OPP2*/
+	{true, S83M, VDD2_OPP2, 0x24},
+	/*OPP3*/
+	{true, S166M, VDD2_OPP3, 0x2C},
+	{0, 0, 0, 0},
+};
+
+struct omap_opp omap3_dsp_rate_table[] = {
+	{0, 0, 0, 0},
+	/*OPP1*/
+	{true, S90M, VDD1_OPP1, 0x1E},
+	/*OPP2*/
+	{true, S180M, VDD1_OPP2, 0x26},
+	/*OPP3*/
+	{true, S360M, VDD1_OPP3, 0x30},
+	/*OPP4*/
+	{true, S400M, VDD1_OPP4, 0x36},
+	/*OPP5*/
+	{true, S430M, VDD1_OPP5, 0x3C},
+	{0, 0, 0, 0},
+};
+
 static inline void omap3_per_save_context(void)
 {
 	omap_gpio_save_context();
diff --git a/arch/arm/plat-omap/include/plat/omap-pm.h b/arch/arm/plat-omap/include/plat/omap-pm.h
index 5dc2048..8316999 100644
--- a/arch/arm/plat-omap/include/plat/omap-pm.h
+++ b/arch/arm/plat-omap/include/plat/omap-pm.h
@@ -35,6 +35,10 @@ struct omap_opp {
 	u16 vsel;
 };
 
+/* Identify a OPP terminator */
+#define IS_OPP_TERMINATOR(opps, i) (!(opps)[(i)].enabled &&\
+		((opps)[(i)].rate == 0) && ((opps)[(i)].vsel == 0))
+
 extern struct omap_opp *mpu_opps;
 extern struct omap_opp *dsp_opps;
 extern struct omap_opp *l3_opps;
@@ -324,5 +328,110 @@ unsigned long omap_pm_cpu_get_freq(void);
  */
 int omap_pm_get_dev_context_loss_count(struct device *dev);
 
+/**
+ * freq_to_opp - Get opp_id corresponding to a frequency if enabled
+ * This function can also be used to check if a certain frequency is enabled
+ * in the opp list.
+ *
+ * @opp_id: the ID to return
+ * @opps: the opps table to search through
+ * @freq: the frequency to search for
+ *
+ * returns 0 if *opp_id is populated, else returns EINVAL
+ */
+int freq_to_opp(u8 *opp_id, const struct omap_opp *opps, unsigned long freq);
 
+/**
+ * freq_to_vsel - Get voltage corresponding to a frequency if enabled
+ * This function can also be used to check if a certain frequency is enabled
+ * in the opp list.
+ *
+ * @vsel: voltage corresponding to return
+ * @opps: the opps table to search through
+ * @freq: the frequency to search for
+ *
+ * returns 0 if *vsel is populated, else returns EINVAL
+ */
+int freq_to_vsel(u8 *vsel, const struct omap_opp *opps, unsigned long freq);
+
+/**
+ * opp_to_freq - Get frequency corresponding to an OPP if enabled
+ * This function can also be used to check if a certain opp_id is enabled
+ * in the opp list.
+ *
+ * @freq: return the frequency on this
+ * @opps: the opps table to search through
+ * @opp_id: the ID to search for
+ *
+ * returns 0 if *freq is populated, else returns EINVAL
+ */
+int opp_to_freq(unsigned long *freq, const struct omap_opp *opps, u8 opp_id);
+
+/**
+ * opp_enable - Enable or Disable an OPP in the supported OPPs if available
+ *
+ * @opps: the opps table to search through
+ * @freq: the frequency to search for
+ * @enable: true to enable the OPP, false to disable it
+ *
+ * returns 0 if the OPP is found, else returns EINVAL. if the opp is found
+ * NOTE: Even if it was in the same state as requested, the functions returns 0.
+ */
+int opp_enable(struct omap_opp *opps, unsigned long freq, bool enable);
+
+/**
+ * get_next_freq - search for next matching frequency, given a starting
+ * frequency. This can be combined to create a search logic etc without
+ * knowing OPP IDs.
+ * Example usages:
+ * a) I have an approximate frequency, get enabled opp freq at least freq
+ *	if a match is achieved, result_freq >= requested_freq
+ *   res = get_next_freq(&freq, opps, true, true, false)
+ * b) I have an approximate frequency, get enabled opp freq less than freq
+ *	if a match is achieved, result_freq < requested_freq
+ *   res = get_next_freq(&freq, opps, false, true, false)
+ * c) I have exact OPP freq I want to check -> search for higher enabled
+ *    frequency
+ *   res = get_next_freq(&freq, opps, true, true, true)
+ * d) I have exact OPP freq I want to check -> search for lower enabled
+ *    frequency
+ *   res = get_next_freq(&freq, opps, false, true, true)
+ *
+ * Then we can create all sorts of search combinations -> including searching
+ * for an OPP freq we would like to enable by controlling search_enabled_only
+ *
+ * @freq: Which frequency to start from- should be a valid frequency,
+ *	  even if not enabled.
+ *	  if a match is found, this will contain the matched frequency
+ * @opps: the opp table to search through
+ * @search_higher: should the search go up the list (search for higher freq)
+ *	  if true, searches for next highest freq, else searches for the next
+ *	  lowest frequency
+ * @search_enabled_only: Should the search only for enabled frequencies.
+ *	  if true, searches for only enabled OPP frequencies, else does not
+ *	  care for enabled status of the OPP (useful to enable OPPs)
+ * @exact_search: start search iff start *freq gets an exact match
+ *
+ * If a match is found, returns the matched frequency in *freq and returns 0,
+ * else, returns EINVAL, *freq is unchanged
+ */
+int get_next_freq(unsigned long *freq, const struct omap_opp *opps,
+	bool search_higher, bool search_enabled_only, bool exact_search);
+
+/**
+ * get_limit_freq: Get the max or min frequency for an opp table
+ * we can search for just the enabled opps, or the max or least in
+ * the table
+ *
+ * @freq: returns the max or min opp if a match was found
+ * @opps: opp table to search
+ * @search_highest: search for the highest if true, else search for lowest
+ *	frequency
+ * @search_enabled_only: search only for enabled OPPs
+ *
+ * returns 0 if a match is found and *freq contains the matched frequency
+ * else, returns EINVAL, *freq is unchanged
+ */
+int get_limit_freq(unsigned long *freq, const struct omap_opp *opps,
+		bool search_highest, bool search_enabled_only);
 #endif
-- 
1.6.3.3

[PATCH 3/9] omap3: pm: srf: use opp accessor function

[Nishanth Menon]

With the accessor functions, many of the direct accesses are
redundant. Cleanup of SRF to:
a) Remove get_opp as it redundant as freq_to_opp does exactly the
   samething.
b) Remove any direct dereference of opp tables except thru accessor
   functions.

NOTE: the implementation is just a start and leaves scope for
further performance and robustness improvements which can be added on
top.

Tested on: SDP3430, SDP3630

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Romit Dasgupta <romit@ti.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
Cc: SuiLun Lam <s-lam@ti.com>
Cc: Thara Gopinath <thara@ti.com>
Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/resource34xx.c |  178 ++++++++++++++++++++++--------------
 1 files changed, 109 insertions(+), 69 deletions(-)

diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c
index af6b3c1..15b5cb6 100644
--- a/arch/arm/mach-omap2/resource34xx.c
+++ b/arch/arm/mach-omap2/resource34xx.c
@@ -155,29 +155,14 @@ static int curr_vdd1_opp;
 static int curr_vdd2_opp;
 static DEFINE_MUTEX(dvfs_mutex);
 
-static unsigned short get_opp(struct omap_opp *opp_freq_table,
-		unsigned long freq)
-{
-	struct omap_opp *prcm_config;
-	prcm_config = opp_freq_table;
-
-	if (prcm_config->rate <= freq)
-		return prcm_config->opp_id; /* Return the Highest OPP */
-	for (; prcm_config->rate; prcm_config--)
-		if (prcm_config->rate < freq)
-			return (prcm_config+1)->opp_id;
-		else if (prcm_config->rate == freq)
-			return prcm_config->opp_id;
-	/* Return the least OPP */
-	return (prcm_config+1)->opp_id;
-}
-
 /**
  * init_opp - Initialize the OPP resource
  */
 void init_opp(struct shared_resource *resp)
 {
 	struct clk *l3_clk;
+	int ret;
+	u8 opp_id;
 	resp->no_of_users = 0;
 
 	if (!mpu_opps || !dsp_opps || !l3_opps)
@@ -190,17 +175,18 @@ void init_opp(struct shared_resource *resp)
 		vdd1_resp = resp;
 		dpll1_clk = clk_get(NULL, "dpll1_ck");
 		dpll2_clk = clk_get(NULL, "dpll2_ck");
-		resp->curr_level = get_opp(mpu_opps + MAX_VDD1_OPP,
-				dpll1_clk->rate);
-		curr_vdd1_opp = resp->curr_level;
+		ret = freq_to_opp(&opp_id, mpu_opps, dpll1_clk->rate);
+		BUG_ON(ret); /* TBD Cleanup handling */
+		curr_vdd1_opp = opp_id;
 	} else if (strcmp(resp->name, "vdd2_opp") == 0) {
 		vdd2_resp = resp;
 		dpll3_clk = clk_get(NULL, "dpll3_m2_ck");
 		l3_clk = clk_get(NULL, "l3_ick");
-		resp->curr_level = get_opp(l3_opps + MAX_VDD2_OPP,
-				l3_clk->rate);
-		curr_vdd2_opp = resp->curr_level;
+		ret = freq_to_opp(&opp_id, l3_opps, l3_clk->rate);
+		BUG_ON(ret); /* TBD Cleanup handling */
+		curr_vdd2_opp = opp_id;
 	}
+	resp->curr_level = opp_id;
 	return;
 }
 
@@ -242,24 +228,40 @@ static int program_opp_freq(int res, int target_level, int current_level)
 {
 	int ret = 0, l3_div;
 	int *curr_opp;
+	unsigned long mpu_freq, dsp_freq, l3_freq;
+#ifndef CONFIG_CPU_FREQ
+	unsigned long mpu_cur_freq
+#endif
+
+	/* Check if I can actually switch or not */
+	if (res == VDD1_OPP) {
+		ret = opp_to_freq(&mpu_freq, mpu_opps, target_level);
+		ret |= opp_to_freq(&dsp_freq, dsp_opps, target_level);
+#ifndef CONFIG_CPU_FREQ
+		ret |= opp_to_freq(&mpu_cur_freq, mpu_opps, current_level);
+#endif
+	} else {
+		ret = opp_to_freq(&l3_freq, l3_opps, target_level);
+	}
+	/* we would have caught all bad levels earlier.. */
+	if (unlikely(ret))
+		return ret;
 
 	lock_scratchpad_sem();
 	if (res == VDD1_OPP) {
 		curr_opp = &curr_vdd1_opp;
-		clk_set_rate(dpll1_clk, mpu_opps[target_level].rate);
-		clk_set_rate(dpll2_clk, dsp_opps[target_level].rate);
+		clk_set_rate(dpll1_clk, mpu_freq);
+		clk_set_rate(dpll2_clk, dsp_freq);
 #ifndef CONFIG_CPU_FREQ
 		/*Update loops_per_jiffy if processor speed is being changed*/
 		loops_per_jiffy = compute_lpj(loops_per_jiffy,
-			mpu_opps[current_level].rate/1000,
-			mpu_opps[target_level].rate/1000);
+			mpu_cur_freq / 1000, mpu_freq / 1000);
 #endif
 	} else {
 		curr_opp = &curr_vdd2_opp;
 		l3_div = cm_read_mod_reg(CORE_MOD, CM_CLKSEL) &
 			OMAP3430_CLKSEL_L3_MASK;
-		ret = clk_set_rate(dpll3_clk,
-				l3_opps[target_level].rate * l3_div);
+		ret = clk_set_rate(dpll3_clk, l3_freq * l3_div);
 	}
 	if (ret) {
 		unlock_scratchpad_sem();
@@ -278,6 +280,7 @@ static int program_opp(int res, struct omap_opp *opp, int target_level,
 		int current_level)
 {
 	int i, ret = 0, raise;
+	unsigned long freq;
 #ifdef CONFIG_OMAP_SMARTREFLEX
 	unsigned long t_opp, c_opp;
 
@@ -285,13 +288,10 @@ static int program_opp(int res, struct omap_opp *opp, int target_level,
 	c_opp = ID_VDD(res) | ID_OPP_NO(opp[current_level].opp_id);
 #endif
 
-	/* Only allow enabled OPPs */
-	if (!opp[target_level].enabled)
-		return -EINVAL;
-
-	/* Sanity check of the OPP params before attempting to set */
-	if (!opp[target_level].rate || !opp[target_level].vsel)
-		return -EINVAL;
+	/* See if have a freq associated, if not, invalid opp */
+	ret = opp_to_freq(&freq, opp, target_level);
+	if (unlikely(ret))
+		return ret;
 
 	if (target_level > current_level)
 		raise = 1;
@@ -303,10 +303,16 @@ static int program_opp(int res, struct omap_opp *opp, int target_level,
 			ret = program_opp_freq(res, target_level,
 					current_level);
 #ifdef CONFIG_OMAP_SMARTREFLEX
-		else
+		else {
+			u8 v_current, v_target;
+			/* none of the following should fail.. */
+			BUG_ON(freq_to_vsel(&v_target, opp, freq));
+			BUG_ON(opp_to_freq(&freq, opp, current_level));
+			BUG_ON(freq_to_vsel(&v_current, opp, freq));
+			/* ok to scale.. */
 			sr_voltagescale_vcbypass(t_opp, c_opp,
-				opp[target_level].vsel,
-				opp[current_level].vsel);
+				v_target, v_current);
+		}
 #endif
 	}
 
@@ -315,7 +321,8 @@ static int program_opp(int res, struct omap_opp *opp, int target_level,
 
 int resource_set_opp_level(int res, u32 target_level, int flags)
 {
-	unsigned long mpu_freq, mpu_old_freq;
+	unsigned long mpu_freq, mpu_old_freq, l3_freq;
+	int ret;
 #ifdef CONFIG_CPU_FREQ
 	struct cpufreq_freqs freqs_notify;
 #endif
@@ -334,6 +341,16 @@ int resource_set_opp_level(int res, u32 target_level, int flags)
 	if (!mpu_opps || !dsp_opps || !l3_opps)
 		return 0;
 
+	/* Check if I can actually switch or not */
+	if (res == VDD1_OPP) {
+		ret = opp_to_freq(&mpu_freq, mpu_opps, target_level);
+		ret |= opp_to_freq(&mpu_old_freq, mpu_opps, resp->curr_level);
+	} else {
+		ret = opp_to_freq(&l3_freq, l3_opps, target_level);
+	}
+	if (ret)
+		return ret;
+
 	mutex_lock(&dvfs_mutex);
 
 	if (res == VDD1_OPP) {
@@ -341,9 +358,6 @@ int resource_set_opp_level(int res, u32 target_level, int flags)
 			mutex_unlock(&dvfs_mutex);
 			return 0;
 		}
-		mpu_old_freq = mpu_opps[resp->curr_level].rate;
-		mpu_freq = mpu_opps[target_level].rate;
-
 #ifdef CONFIG_CPU_FREQ
 		freqs_notify.old = mpu_old_freq/1000;
 		freqs_notify.new = mpu_freq/1000;
@@ -371,15 +385,13 @@ int resource_set_opp_level(int res, u32 target_level, int flags)
 
 int set_opp(struct shared_resource *resp, u32 target_level)
 {
-	unsigned long tput;
-	unsigned long req_l3_freq;
-	int ind;
+	int ret = -EINVAL;
 
 	if (resp == vdd1_resp) {
 		if (target_level < 3)
 			resource_release("vdd2_opp", &vdd2_dev);
 
-		resource_set_opp_level(VDD1_OPP, target_level, 0);
+		ret = resource_set_opp_level(VDD1_OPP, target_level, 0);
 		/*
 		 * For VDD1 OPP3 and above, make sure the interconnect
 		 * is at 100Mhz or above.
@@ -389,21 +401,28 @@ int set_opp(struct shared_resource *resp, u32 target_level)
 			resource_request("vdd2_opp", &vdd2_dev, 400000);
 
 	} else if (resp == vdd2_resp) {
-		tput = target_level;
+		unsigned long req_l3_freq;
 
 		/* Convert the tput in KiB/s to Bus frequency in MHz */
-		req_l3_freq = (tput * 1000)/4;
-
-		for (ind = 2; ind <= MAX_VDD2_OPP; ind++)
-			if ((l3_opps + ind)->rate >= req_l3_freq) {
-				target_level = ind;
-				break;
-			}
-
-		/* Set the highest OPP possible */
-		if (ind > MAX_VDD2_OPP)
-			target_level = ind-1;
-		resource_set_opp_level(VDD2_OPP, target_level, 0);
+		req_l3_freq = (target_level * 1000)/4;
+
+		/* Get the best freq around */
+		ret = get_next_freq(&req_l3_freq, l3_opps, true, true, false);
+
+		/* If we dont find a good freq, then use the highest
+		 * possible freq
+		 */
+		if (ret)
+			ret = get_limit_freq(&req_l3_freq, l3_opps, true, true);
+
+		/* uh uh.. no OPPs?? */
+		BUG_ON(ret);
+
+		ret = freq_to_opp((u8 *)&target_level, l3_opps, req_l3_freq);
+		/* we dont expect this to fail */
+		BUG_ON(ret);
+
+		ret = resource_set_opp_level(VDD2_OPP, target_level, 0);
 	}
 	return 0;
 }
@@ -416,6 +435,11 @@ int set_opp(struct shared_resource *resp, u32 target_level)
  */
 int validate_opp(struct shared_resource *resp, u32 target_level)
 {
+	unsigned long x;
+	if (strcmp(resp->name, "mpu_freq") == 0)
+		return opp_to_freq(&x, mpu_opps, target_level);
+	else if (strcmp(resp->name, "dsp_freq") == 0)
+		return opp_to_freq(&x, dsp_opps, target_level);
 	return 0;
 }
 
@@ -425,6 +449,8 @@ int validate_opp(struct shared_resource *resp, u32 target_level)
 void init_freq(struct shared_resource *resp)
 {
 	char *linked_res_name;
+	int ret = -EINVAL;
+	unsigned long freq;
 	resp->no_of_users = 0;
 
 	if (!mpu_opps || !dsp_opps)
@@ -436,32 +462,46 @@ void init_freq(struct shared_resource *resp)
 	*/
 	if (strcmp(resp->name, "mpu_freq") == 0)
 		/* MPU freq in Mhz */
-		resp->curr_level = mpu_opps[curr_vdd1_opp].rate;
+		ret = opp_to_freq(&freq, mpu_opps, curr_vdd1_opp);
 	else if (strcmp(resp->name, "dsp_freq") == 0)
 		/* DSP freq in Mhz */
-		resp->curr_level = dsp_opps[curr_vdd1_opp].rate;
+		ret = opp_to_freq(&freq, dsp_opps, curr_vdd1_opp);
+	BUG_ON(ret);
+
+	resp->curr_level = freq;
 	return;
 }
 
 int set_freq(struct shared_resource *resp, u32 target_level)
 {
-	unsigned int vdd1_opp;
+	u8 vdd1_opp;
+	int ret = -EINVAL;
 
 	if (!mpu_opps || !dsp_opps)
 		return 0;
 
 	if (strcmp(resp->name, "mpu_freq") == 0) {
-		vdd1_opp = get_opp(mpu_opps + MAX_VDD1_OPP, target_level);
-		resource_request("vdd1_opp", &dummy_mpu_dev, vdd1_opp);
+		ret = freq_to_opp(&vdd1_opp, mpu_opps, target_level);
+		if (!ret)
+			ret = resource_request("vdd1_opp", &dummy_mpu_dev,
+					vdd1_opp);
 	} else if (strcmp(resp->name, "dsp_freq") == 0) {
-		vdd1_opp = get_opp(dsp_opps + MAX_VDD1_OPP, target_level);
-		resource_request("vdd1_opp", &dummy_dsp_dev, vdd1_opp);
+		ret = freq_to_opp(&vdd1_opp, dsp_opps, target_level);
+		if (!ret)
+			ret = resource_request("vdd1_opp", &dummy_dsp_dev,
+					vdd1_opp);
 	}
-	resp->curr_level = target_level;
-	return 0;
+	if (!ret)
+		resp->curr_level = target_level;
+	return ret;
 }
 
 int validate_freq(struct shared_resource *resp, u32 target_level)
 {
+	u8 x;
+	if (strcmp(resp->name, "mpu_freq") == 0)
+		return freq_to_opp(&x, mpu_opps, target_level);
+	else if (strcmp(resp->name, "dsp_freq") == 0)
+		return freq_to_opp(&x, dsp_opps, target_level);
 	return 0;
 }
-- 
1.6.3.3

[PATCH 4/9] omap3: pm: use opp accessor functions for omap-target

[Nishanth Menon]

The logic in omap-target can now be improved with the accessor
functions. Dont scan through the list manually, instead use
get_next_freq to do the scanning.

Tested on: SDP3430, SDP3630

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Romit Dasgupta <romit@ti.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
Cc: SuiLun Lam <s-lam@ti.com>
Cc: Thara Gopinath <thara@ti.com>
Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/plat-omap/cpu-omap.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/arm/plat-omap/cpu-omap.c b/arch/arm/plat-omap/cpu-omap.c
index 449b6b6..16b5da2 100644
--- a/arch/arm/plat-omap/cpu-omap.c
+++ b/arch/arm/plat-omap/cpu-omap.c
@@ -111,14 +111,10 @@ static int omap_target(struct cpufreq_policy *policy,
 	cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 #elif defined(CONFIG_ARCH_OMAP3) && !defined(CONFIG_OMAP_PM_NONE)
 	if (mpu_opps) {
-		int ind;
-		for (ind = 1; ind <= MAX_VDD1_OPP; ind++) {
-			if (mpu_opps[ind].rate/1000 >= target_freq) {
-				omap_pm_cpu_set_freq
-					(mpu_opps[ind].rate);
-				break;
-			}
-		}
+		unsigned long freq = target_freq * 1000;
+		int res = get_next_freq(&freq, mpu_opps, true, true, false);
+		if (!res)
+			omap_pm_cpu_set_freq(freq);
 	}
 #endif
 	return ret;
-- 
1.6.3.3

[PATCH 5/9] omap3: pm: sr: replace get_opp with freq_to_opp

[Nishanth Menon]

SmartReflex implements a get_opp to search through the opp table,
replace it with the accessor function as it is a duplicate of
freq_to_opp

Tested on: SDP3430, SDP3630

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Romit Dasgupta <romit@ti.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
Cc: SuiLun Lam <s-lam@ti.com>
Cc: Thara Gopinath <thara@ti.com>
Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/smartreflex.c |   36 ++++++++----------------------------
 1 files changed, 8 insertions(+), 28 deletions(-)

diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index be3a1da..d34224d 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -146,49 +146,29 @@ static u32 cal_test_nvalue(u32 sennval, u32 senpval)
 		(rnsenn << NVALUERECIPROCAL_RNSENN_SHIFT);
 }
 
-/* determine the current OPP from the frequency
- * we need to give this function last element of OPP rate table
- * and the frequency
- */
-static u16 get_opp(struct omap_opp *opp_freq_table,
-					unsigned long freq)
-{
-	struct omap_opp *prcm_config;
-
-	prcm_config = opp_freq_table;
-
-	if (prcm_config->rate <= freq)
-		return prcm_config->opp_id; /* Return the Highest OPP */
-	for (; prcm_config->rate; prcm_config--)
-		if (prcm_config->rate < freq)
-			return (prcm_config+1)->opp_id;
-		else if (prcm_config->rate == freq)
-			return prcm_config->opp_id;
-	/* Return the least OPP */
-	return (prcm_config+1)->opp_id;
-}
-
-static u16 get_vdd1_opp(void)
+static u8 get_vdd1_opp(void)
 {
-	u16 opp;
+	u8 opp;
 
 	if (sr1.vdd_opp_clk == NULL || IS_ERR(sr1.vdd_opp_clk) ||
 							mpu_opps == NULL)
 		return 0;
 
-	opp = get_opp(mpu_opps + MAX_VDD1_OPP, sr1.vdd_opp_clk->rate);
+	/* Not expected to fail.. */
+	BUG_ON(freq_to_opp(&opp, mpu_opps, sr1.vdd_opp_clk->rate));
 	return opp;
 }
 
-static u16 get_vdd2_opp(void)
+static u8 get_vdd2_opp(void)
 {
-	u16 opp;
+	u8 opp;
 
 	if (sr2.vdd_opp_clk == NULL || IS_ERR(sr2.vdd_opp_clk) ||
 							l3_opps == NULL)
 		return 0;
 
-	opp = get_opp(l3_opps + MAX_VDD2_OPP, sr2.vdd_opp_clk->rate);
+	/* Not expected to fail.. */
+	BUG_ON(freq_to_opp(&opp, l3_opps, sr2.vdd_opp_clk->rate));
 	return opp;
 }
 
-- 
1.6.3.3

[PATCH 6/9] omap3: clk: use pm accessor functions for cpufreq table

[Nishanth Menon]

omap2_clk_init_cpufreq_table currently directly accesses the opp
table, making it unscalable to various OMAPs. Instead use the
accessor functions to populate the cpufreq table

Tested on: SDP3430, SDP3630

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Romit Dasgupta <romit@ti.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
Cc: SuiLun Lam <s-lam@ti.com>
Cc: Thara Gopinath <thara@ti.com>
Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/clock34xx.c |   46 +++++++++++++++++++++++++++-----------
 1 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-omap2/clock34xx.c b/arch/arm/mach-omap2/clock34xx.c
index da5bc1f..daec6ea 100644
--- a/arch/arm/mach-omap2/clock34xx.c
+++ b/arch/arm/mach-omap2/clock34xx.c
@@ -1042,28 +1042,46 @@ static unsigned long omap3_clkoutx2_recalc(struct clk *clk)
 #if defined(CONFIG_ARCH_OMAP3)
 
 #ifdef CONFIG_CPU_FREQ
-static struct cpufreq_frequency_table freq_table[MAX_VDD1_OPP+1];
+
+/* Use a dummy table with no entries to start with */
+static struct cpufreq_frequency_table dummy_freq_table = {
+	.frequency = CPUFREQ_TABLE_END,
+};
+static struct cpufreq_frequency_table *freq_table = &dummy_freq_table;
 
 void omap2_clk_init_cpufreq_table(struct cpufreq_frequency_table **table)
 {
-	struct omap_opp *prcm;
-	int i = 0;
+	int i;
+	int ret;
+	u8 opp_id;
+	unsigned long freq;
 
 	if (!mpu_opps)
 		return;
-
-	prcm = mpu_opps + MAX_VDD1_OPP;
-	for (; prcm->rate; prcm--) {
-		freq_table[i].index = i;
-		freq_table[i].frequency = prcm->rate / 1000;
-		i++;
+	ret = get_limit_freq(&freq, mpu_opps, true, true);
+	if (ret) {
+		pr_warning("%s: failed to initialize frequency"
+				"table\n", __func__);
+		return;
 	}
+	/* The following should'nt fail */
+	BUG_ON(freq_to_opp(&opp_id, mpu_opps, freq));
 
-	if (i == 0) {
-		printk(KERN_WARNING "%s: failed to initialize frequency \
-								table\n",
-								__func__);
-		return;
+	/* There is a risk of overallocating if the lower/intermediate
+	 * OPPs are disabled, but the amount is not expected to be high
+	 * in comparison to reallocating to exactly available opps
+	 */
+	freq_table = kmalloc(sizeof(struct cpufreq_frequency_table) *
+			(opp_id + 1), GFP_KERNEL);
+
+	/* Populate the first index.. we already found the freq */
+	freq_table[0].index = 0;
+	freq_table[0].frequency =  freq / 1000;
+
+	/* Populate the table highest to lowest */
+	for (i = 1; !get_next_freq(&freq, mpu_opps, false, true, true); i++) {
+		freq_table[i].index = i;
+		freq_table[i].frequency =  freq / 1000;
 	}
 
 	freq_table[i].index = i;
-- 
1.6.3.3

[PATCH 7/9] omap3: pm: remove VDDx_MIN/MAX macros

[Nishanth Menon]

Since accessor functions are used through out, we dont depend
on the predefined macros to know the limits of the opp table.
Hence, remove these.

Tested on: SDP3430, SDP3630

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Romit Dasgupta <romit@ti.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
Cc: SuiLun Lam <s-lam@ti.com>
Cc: Thara Gopinath <thara@ti.com>
Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/plat-omap/include/plat/omap34xx.h |    5 -----
 1 files changed, 0 insertions(+), 5 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/omap34xx.h b/arch/arm/plat-omap/include/plat/omap34xx.h
index 868e238..cc7cfae 100644
--- a/arch/arm/plat-omap/include/plat/omap34xx.h
+++ b/arch/arm/plat-omap/include/plat/omap34xx.h
@@ -104,10 +104,5 @@
 #define VDD2_OPP2	0x2
 #define VDD2_OPP3	0x3
 
-#define MIN_VDD1_OPP	VDD1_OPP1
-#define MAX_VDD1_OPP	VDD1_OPP5
-#define MIN_VDD2_OPP	VDD2_OPP1
-#define MAX_VDD2_OPP	VDD2_OPP3
-
 #endif /* __ASM_ARCH_OMAP34XX_H */
 
-- 
1.6.3.3

[PATCH 8/9 v2] omap3: pm: introduce dynamic OPP

[Nishanth Menon]

OPP tables have been used as static arrays till now this causes
limitations in terms of ability to dynamically making decisions
for cpu types and populating it with the right values. This
implementation is based on the discussions in the thread:
http://marc.info/?l=linux-omap&m=125482970102327&w=2
inputs from Romit, Benoit, Kevin, Sanjeev, Santosh acked.

omap3_pm_init_opp_table is introduced here, and the default omap3
tables have been renamed to omap34xx_ tables, original omap3_xx
tables are now dynamically allocated and populated.

Corresponding board files calling omap2_init_common_hw with the
active opp table params have been updated with this patch.

This now paves way to introduce 3630 OPP tables.

Tested on: SDP3430 - SDP3630 boot breaks with this patch
(as SDP3630 needs corresponding correct VDD2 freq w.r.t SDRC clk.
The next patch in this series fixes things)

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Romit Dasgupta <romit@ti.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
Cc: SuiLun Lam <s-lam@ti.com>
Cc: Thara Gopinath <thara@ti.com>
Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/board-3430sdp.c     |    1 +
 arch/arm/mach-omap2/board-3630sdp.c     |    6 +++++-
 arch/arm/mach-omap2/board-omap3beagle.c |    1 +
 arch/arm/mach-omap2/board-omap3evm.c    |    1 +
 arch/arm/mach-omap2/board-rx51.c        |    1 +
 arch/arm/mach-omap2/board-zoom2.c       |    7 +++++--
 arch/arm/mach-omap2/board-zoom3.c       |    6 +++++-
 arch/arm/mach-omap2/omap3-opp.h         |    6 +++---
 arch/arm/mach-omap2/pm.h                |    7 +++++++
 arch/arm/mach-omap2/pm34xx.c            |   31 ++++++++++++++++++++++++++++---
 10 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/arch/arm/mach-omap2/board-3430sdp.c b/arch/arm/mach-omap2/board-3430sdp.c
index 74c20ee..a326b39 100644
--- a/arch/arm/mach-omap2/board-3430sdp.c
+++ b/arch/arm/mach-omap2/board-3430sdp.c
@@ -220,6 +220,7 @@ static void __init omap_3430sdp_init_irq(void)
 {
 	omap_board_config = sdp3430_config;
 	omap_board_config_size = ARRAY_SIZE(sdp3430_config);
+	omap3_pm_init_opp_table();
 	omap3_pm_init_vc(&omap3_setuptime_table);
 	omap3_pm_init_cpuidle(omap3_cpuidle_params_table);
 	omap2_init_common_hw(hyb18m512160af6_sdrc_params, NULL, omap3_mpu_rate_table,
diff --git a/arch/arm/mach-omap2/board-3630sdp.c b/arch/arm/mach-omap2/board-3630sdp.c
index d130d67..f613f82 100755
--- a/arch/arm/mach-omap2/board-3630sdp.c
+++ b/arch/arm/mach-omap2/board-3630sdp.c
@@ -21,7 +21,9 @@
 #include <plat/mux.h>
 #include <plat/usb.h>
 
+#include "pm.h"
 #include "sdram-hynix-h8mbx00u0mer-0em.h"
+#include "omap3-opp.h"
 
 #if defined(CONFIG_SMC91X) || defined(CONFIG_SMC91X_MODULE)
 
@@ -74,9 +76,11 @@ static void __init omap_sdp_init_irq(void)
 {
 	omap_board_config = sdp_config;
 	omap_board_config_size = ARRAY_SIZE(sdp_config);
+	omap3_pm_init_opp_table();
 	omap2_init_common_hw(h8mbx00u0mer0em_sdrc_params,
 			     h8mbx00u0mer0em_sdrc_params,
-                             NULL, NULL, NULL);
+			     omap3_mpu_rate_table, omap3_dsp_rate_table,
+			     omap3_l3_rate_table);
 	omap_init_irq();
 	omap_gpio_init();
 }
diff --git a/arch/arm/mach-omap2/board-omap3beagle.c b/arch/arm/mach-omap2/board-omap3beagle.c
index 44de94b..e7bee77 100644
--- a/arch/arm/mach-omap2/board-omap3beagle.c
+++ b/arch/arm/mach-omap2/board-omap3beagle.c
@@ -351,6 +351,7 @@ static void __init omap3_beagle_init_irq(void)
 {
 	omap_board_config = omap3_beagle_config;
 	omap_board_config_size = ARRAY_SIZE(omap3_beagle_config);
+	omap3_pm_init_opp_table();
 	omap2_init_common_hw(mt46h32m32lf6_sdrc_params,
 			     mt46h32m32lf6_sdrc_params, omap3_mpu_rate_table,
 			     omap3_dsp_rate_table, omap3_l3_rate_table);
diff --git a/arch/arm/mach-omap2/board-omap3evm.c b/arch/arm/mach-omap2/board-omap3evm.c
index 2c63002..d8318cd 100644
--- a/arch/arm/mach-omap2/board-omap3evm.c
+++ b/arch/arm/mach-omap2/board-omap3evm.c
@@ -336,6 +336,7 @@ static void __init omap3_evm_init_irq(void)
 {
 	omap_board_config = omap3_evm_config;
 	omap_board_config_size = ARRAY_SIZE(omap3_evm_config);
+	omap3_pm_init_opp_table();
 	omap2_init_common_hw(mt46h32m32lf6_sdrc_params, NULL, omap3_mpu_rate_table,
 	                     omap3_dsp_rate_table, omap3_l3_rate_table);
 	omap_init_irq();
diff --git a/arch/arm/mach-omap2/board-rx51.c b/arch/arm/mach-omap2/board-rx51.c
index 2f1c2be..997fd1c 100644
--- a/arch/arm/mach-omap2/board-rx51.c
+++ b/arch/arm/mach-omap2/board-rx51.c
@@ -103,6 +103,7 @@ static void __init rx51_init_irq(void)
 
 	omap_board_config = rx51_config;
 	omap_board_config_size = ARRAY_SIZE(rx51_config);
+	omap3_pm_init_opp_table();
 	omap3_pm_init_cpuidle(rx51_cpuidle_params);
 	sdrc_params = rx51_get_sdram_timings();
 	omap2_init_common_hw(sdrc_params, sdrc_params,
diff --git a/arch/arm/mach-omap2/board-zoom2.c b/arch/arm/mach-omap2/board-zoom2.c
index 430b791..2799e3c 100644
--- a/arch/arm/mach-omap2/board-zoom2.c
+++ b/arch/arm/mach-omap2/board-zoom2.c
@@ -21,6 +21,7 @@
 #include <plat/common.h>
 #include <plat/board.h>
 
+#include "pm.h"
 #include "sdram-micron-mt46h32m32lf-6.h"
 #include "omap3-opp.h"
 
@@ -31,9 +32,11 @@ static void __init omap_zoom2_init_irq(void)
 {
 	omap_board_config = zoom2_config;
 	omap_board_config_size = ARRAY_SIZE(zoom2_config);
+	omap3_pm_init_opp_table();
 	omap2_init_common_hw(mt46h32m32lf6_sdrc_params,
-				 mt46h32m32lf6_sdrc_params, omap3_mpu_rate_table,
-				 omap3_dsp_rate_table, omap3_l3_rate_table);
+				 mt46h32m32lf6_sdrc_params,
+				 omap3_mpu_rate_table, omap3_dsp_rate_table,
+				 omap3_l3_rate_table);
 	omap_init_irq();
 	omap_gpio_init();
 }
diff --git a/arch/arm/mach-omap2/board-zoom3.c b/arch/arm/mach-omap2/board-zoom3.c
index 50a13ea..e21ad34 100644
--- a/arch/arm/mach-omap2/board-zoom3.c
+++ b/arch/arm/mach-omap2/board-zoom3.c
@@ -19,7 +19,9 @@
 #include <plat/common.h>
 #include <plat/board.h>
 
+#include "pm.h"
 #include "sdram-hynix-h8mbx00u0mer-0em.h"
+#include "omap3-opp.h"
 
 static void __init omap_zoom_map_io(void)
 {
@@ -34,9 +36,11 @@ static void __init omap_zoom_init_irq(void)
 {
 	omap_board_config = zoom_config;
 	omap_board_config_size = ARRAY_SIZE(zoom_config);
+	omap3_pm_init_opp_table();
 	omap2_init_common_hw(h8mbx00u0mer0em_sdrc_params,
 			     h8mbx00u0mer0em_sdrc_params,
-			     NULL, NULL, NULL);
+			     omap3_mpu_rate_table, omap3_dsp_rate_table,
+			     omap3_l3_rate_table);
 	omap_init_irq();
 	omap_gpio_init();
 }
diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
index 27e2ca5..7f27f44 100644
--- a/arch/arm/mach-omap2/omap3-opp.h
+++ b/arch/arm/mach-omap2/omap3-opp.h
@@ -21,8 +21,8 @@
 #define S83M    83000000
 #define S166M   166000000
 
-extern struct omap_opp omap3_mpu_rate_table[];
-extern struct omap_opp omap3_dsp_rate_table[];
-extern struct omap_opp omap3_l3_rate_table[];
+extern struct omap_opp *omap3_mpu_rate_table;
+extern struct omap_opp *omap3_dsp_rate_table;
+extern struct omap_opp *omap3_l3_rate_table;
 
 #endif
diff --git a/arch/arm/mach-omap2/pm.h b/arch/arm/mach-omap2/pm.h
index c195b14..76bbdc4 100644
--- a/arch/arm/mach-omap2/pm.h
+++ b/arch/arm/mach-omap2/pm.h
@@ -59,6 +59,13 @@ static inline void omap3_pm_init_cpuidle(
 }
 #endif
 
+/**
+ * omap3_pm_init_opp_table - OMAP opp table lookup called after cpu is detected.
+ * Initialize the basic opp table here, board files could choose to modify opp
+ * table after the basic initialization
+ */
+extern void omap3_pm_init_opp_table(void);
+
 extern int resource_set_opp_level(int res, u32 target_level, int flags);
 extern int resource_access_opp_lock(int res, int delta);
 #define resource_lock_opp(res) resource_access_opp_lock(res, 1)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 2f17a40..14131f8 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -100,7 +100,7 @@ static struct prm_setup_vc prm_setup = {
 	.vdd1_off = 0x00,	/* 0.6v */
 };
 
-struct omap_opp omap3_mpu_rate_table[] = {
+static __initdata struct omap_opp omap34xx_mpu_rate_table[] = {
 	{0, 0, 0, 0},
 	/*OPP1*/
 	{true, S125M, VDD1_OPP1, 0x1E},
@@ -115,7 +115,7 @@ struct omap_opp omap3_mpu_rate_table[] = {
 	{0, 0, 0, 0},
 };
 
-struct omap_opp omap3_l3_rate_table[] = {
+static __initdata struct omap_opp omap34xx_l3_rate_table[] = {
 	{0, 0, 0, 0},
 	/*OPP1*/
 	{false, 0, VDD2_OPP1, 0x1E},
@@ -126,7 +126,7 @@ struct omap_opp omap3_l3_rate_table[] = {
 	{0, 0, 0, 0},
 };
 
-struct omap_opp omap3_dsp_rate_table[] = {
+static __initdata struct omap_opp omap34xx_dsp_rate_table[] = {
 	{0, 0, 0, 0},
 	/*OPP1*/
 	{true, S90M, VDD1_OPP1, 0x1E},
@@ -141,6 +141,10 @@ struct omap_opp omap3_dsp_rate_table[] = {
 	{0, 0, 0, 0},
 };
 
+struct omap_opp *omap3_mpu_rate_table;
+struct omap_opp *omap3_dsp_rate_table;
+struct omap_opp *omap3_l3_rate_table;
+
 static inline void omap3_per_save_context(void)
 {
 	omap_gpio_save_context();
@@ -1280,6 +1284,27 @@ static void __init configure_vc(void)
 	pm_dbg_regset_init(2);
 }
 
+void __init omap3_pm_init_opp_table(void)
+{
+	/* Populate the base CPU rate tables here */
+	omap3_mpu_rate_table = kmalloc(sizeof(omap34xx_mpu_rate_table),
+			GFP_KERNEL);
+	omap3_dsp_rate_table = kmalloc(sizeof(omap34xx_dsp_rate_table),
+			GFP_KERNEL);
+	omap3_l3_rate_table = kmalloc(sizeof(omap34xx_l3_rate_table),
+			GFP_KERNEL);
+
+	BUG_ON(!omap3_mpu_rate_table || !omap3_dsp_rate_table ||
+		!omap3_l3_rate_table);
+
+	memcpy(omap3_mpu_rate_table, omap34xx_mpu_rate_table,
+			sizeof(omap34xx_mpu_rate_table));
+	memcpy(omap3_dsp_rate_table, omap34xx_dsp_rate_table,
+			sizeof(omap34xx_dsp_rate_table));
+	memcpy(omap3_l3_rate_table, omap34xx_l3_rate_table,
+			sizeof(omap34xx_l3_rate_table));
+}
+
 static int __init omap3_pm_early_init(void)
 {
 	prm_clear_mod_reg_bits(OMAP3430_OFFMODE_POL, OMAP3430_GR_MOD,
-- 
1.6.3.3

[PATCH 9/9 v2] omap3: pm: introduce 3630 opps

[Nishanth Menon]

Introduce the OMAP3630 OPPs including the defined OPP tuples.

Further information on OMAP3630 can be found here:
http://focus.ti.com/general/docs/wtbu/wtbuproductcontent.tsp?templateId=6123&navigationId=12836&contentId=52606

OMAP36xx family introduces:
VDD1 with 4 OPPs, of which OPP3 & 4 are available only on devices yet
to be introduced in 36xx family. Meanwhile, VDD2 has 2 opps.

Range of OPPs supported by Devices(tentative)->
           |<-3630-600->| (default)
           |<-      3630-800  ->| (device: TBD)
           |<-      3630-1000             ->| (device: TBD)
H/w OPP-> OPP50 OPP100       OPP-Turbo   OPP1G-SB
VDD1      OPP1  OPP2         OPP3        OPP4
VDD2      OPP1  OPP2         OPP2        OPP2

Note:
a) TI h/w naming for OPPs are now standardized in terms of OPP50, 100,
   Turbo and SB. This maps as shown above to the opp IDs (s/w term).
b) For boards which need custom VDD1/2 OPPs, the opp table can be
   updated by the board file on a need basis after the
   omap3_pm_init_opp_table call. The OPPs introduced here are the
   official OPP table at this point in time.

Tested on: SDP3430, SDP3630

Cc: Benoit Cousson <b-cousson@ti.com>
Cc: Jon Hunter <jon-hunter@ti.com>
Cc: Kevin Hilman <khilman@deeprootsystems.com>
Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
Cc: Paul Walmsley <paul@pwsan.com>
Cc: Romit Dasgupta <romit@ti.com>
Cc: Sanjeev Premi <premi@ti.com>
Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
Cc: SuiLun Lam <s-lam@ti.com>
Cc: Thara Gopinath <thara@ti.com>
Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>

Signed-off-by: Nishanth Menon <nm@ti.com>
---
 arch/arm/mach-omap2/omap3-opp.h |    9 ++++
 arch/arm/mach-omap2/pm34xx.c    |   87 +++++++++++++++++++++++++++++++-------
 2 files changed, 80 insertions(+), 16 deletions(-)

diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
index 7f27f44..a5880b8 100644
--- a/arch/arm/mach-omap2/omap3-opp.h
+++ b/arch/arm/mach-omap2/omap3-opp.h
@@ -4,22 +4,31 @@
 #include <plat/omap-pm.h>
 
 /* MPU speeds */
+#define S1000M  1000000000
+#define S800M   800000000
 #define S600M   600000000
 #define S550M   550000000
 #define S500M   500000000
+#define S300M   300000000
 #define S250M   250000000
 #define S125M   125000000
 
 /* DSP speeds */
+#define S875M   875000000
+#define S660M   660000000
+#define S520M   520000000
 #define S430M   430000000
 #define S400M   400000000
 #define S360M   360000000
+#define S260M   260000000
 #define S180M   180000000
 #define S90M    90000000
 
 /* L3 speeds */
 #define S83M    83000000
+#define S100M   100000000
 #define S166M   166000000
+#define S200M   200000000
 
 extern struct omap_opp *omap3_mpu_rate_table;
 extern struct omap_opp *omap3_dsp_rate_table;
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 14131f8..86137bb 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -141,6 +141,41 @@ static __initdata struct omap_opp omap34xx_dsp_rate_table[] = {
 	{0, 0, 0, 0},
 };
 
+static __initdata struct omap_opp omap36xx_mpu_rate_table[] = {
+	{0, 0, 0, 0},
+	/*OPP1 - 930mV - OPP50*/
+	{true, S300M, VDD1_OPP1, 0x1a},
+	/*OPP2 - 1.100V - OPP100*/
+	{true, S600M, VDD1_OPP2, 0x28},
+	/*OPP3 - 1.260V - OPP-Turbo*/
+	{false, S800M, VDD1_OPP3, 0x34},
+	/*OPP4 - 1.310V - OPP-SB*/
+	{false, S1000M, VDD1_OPP4, 0x38},
+	{0, 0, 0, 0},
+};
+
+static __initdata struct omap_opp omap36xx_l3_rate_table[] = {
+	{0, 0, 0, 0},
+	/*OPP1 - 930mV - OPP50 */
+	{true, S100M, VDD2_OPP1, 0x1a},
+	/*OPP2 - 1.375V - OPP100, OPP-Turbo, OPP-SB*/
+	{true, S200M, VDD2_OPP2, 0x2b},
+	{0, 0, 0, 0},
+};
+
+static __initdata struct omap_opp omap36xx_dsp_rate_table[] = {
+	{0, 0, 0, 0},
+	/*OPP1 - OPP50*/
+	{true, S260M, VDD1_OPP1, 0x1a},
+	/*OPP2 - OPP100*/
+	{true, S520M, VDD1_OPP2, 0x28},
+	/*OPP3 - OPP-Turbo*/
+	{false, S660M, VDD1_OPP3, 0x34},
+	/*OPP4 - OPP-SB*/
+	{false, S875M, VDD1_OPP4, 0x38},
+	{0, 0, 0, 0},
+};
+
 struct omap_opp *omap3_mpu_rate_table;
 struct omap_opp *omap3_dsp_rate_table;
 struct omap_opp *omap3_l3_rate_table;
@@ -1287,22 +1322,42 @@ static void __init configure_vc(void)
 void __init omap3_pm_init_opp_table(void)
 {
 	/* Populate the base CPU rate tables here */
-	omap3_mpu_rate_table = kmalloc(sizeof(omap34xx_mpu_rate_table),
-			GFP_KERNEL);
-	omap3_dsp_rate_table = kmalloc(sizeof(omap34xx_dsp_rate_table),
-			GFP_KERNEL);
-	omap3_l3_rate_table = kmalloc(sizeof(omap34xx_l3_rate_table),
-			GFP_KERNEL);
-
-	BUG_ON(!omap3_mpu_rate_table || !omap3_dsp_rate_table ||
-		!omap3_l3_rate_table);
-
-	memcpy(omap3_mpu_rate_table, omap34xx_mpu_rate_table,
-			sizeof(omap34xx_mpu_rate_table));
-	memcpy(omap3_dsp_rate_table, omap34xx_dsp_rate_table,
-			sizeof(omap34xx_dsp_rate_table));
-	memcpy(omap3_l3_rate_table, omap34xx_l3_rate_table,
-			sizeof(omap34xx_l3_rate_table));
+	if (cpu_is_omap3630()) {
+		omap3_mpu_rate_table = kmalloc(sizeof(omap36xx_mpu_rate_table),
+				GFP_KERNEL);
+		omap3_dsp_rate_table = kmalloc(sizeof(omap36xx_dsp_rate_table),
+				GFP_KERNEL);
+		omap3_l3_rate_table = kmalloc(sizeof(omap36xx_l3_rate_table),
+				GFP_KERNEL);
+
+		BUG_ON(!omap3_mpu_rate_table || !omap3_dsp_rate_table ||
+			!omap3_l3_rate_table);
+
+		memcpy(omap3_mpu_rate_table, omap36xx_mpu_rate_table,
+				sizeof(omap36xx_mpu_rate_table));
+		memcpy(omap3_dsp_rate_table, omap36xx_dsp_rate_table,
+				sizeof(omap36xx_dsp_rate_table));
+		memcpy(omap3_l3_rate_table, omap36xx_l3_rate_table,
+				sizeof(omap36xx_l3_rate_table));
+	} else {
+		/* Default to 34xx devices */
+		omap3_mpu_rate_table = kmalloc(sizeof(omap34xx_mpu_rate_table),
+				GFP_KERNEL);
+		omap3_dsp_rate_table = kmalloc(sizeof(omap34xx_dsp_rate_table),
+				GFP_KERNEL);
+		omap3_l3_rate_table = kmalloc(sizeof(omap34xx_l3_rate_table),
+				GFP_KERNEL);
+
+		BUG_ON(!omap3_mpu_rate_table || !omap3_dsp_rate_table ||
+			!omap3_l3_rate_table);
+
+		memcpy(omap3_mpu_rate_table, omap34xx_mpu_rate_table,
+				sizeof(omap34xx_mpu_rate_table));
+		memcpy(omap3_dsp_rate_table, omap34xx_dsp_rate_table,
+				sizeof(omap34xx_dsp_rate_table));
+		memcpy(omap3_l3_rate_table, omap34xx_l3_rate_table,
+				sizeof(omap34xx_l3_rate_table));
+	}
 }
 
 static int __init omap3_pm_early_init(void)
-- 
1.6.3.3

Re: [PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Modifies the initial patch From Sanjeev:
> http://patchwork.kernel.org/patch/50998/
>
> NOTE:
> The original opp_table introduced by Sanjeev is not needed anymore as
> we can use enabled flag to have better granularity in enable/disable
> of OPPs.
>
> This introduces the following accessor functions:
>
> freq_to_opp and opp_to_freq: Matching functions to convert OPP to freq
> and viceversa.
>
> freq_to_vsel: Converts a frequency to corresponding voltage.
>
> opp_enable: To enable/disable a specific OPP in a OPP table this allows
> granular runtime disable/enable of specific OPPs, esp when used in
> conjunction with search and mapping functions
>
> get_next_freq: A search function to get next matching frequency. This
> could possibly provide the basis for more complex OPP transition algos
> of the future.
>
> get_limit_freq: A search function to get the least or maximum
> frequency based on search criteria. Allows for independence from
> OPP_IDs in the future.
>
> Since the accessor functions hide the details of the table
> implementation, the opp table is now moved away from omap3-opp.h to
> pm34xx.c. The terminator entry is needed at the start and end of the
> table as it is still needed for reverse and forward search as the
> length of the table is unknown.
>
> Tests done: Accessor functions standalone tested on a PC host with
> dummy OPP table to simulate boundary, invalid and valid conditions,
> SDP3430, SDP3630 for system stability.
>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Jon Hunter <jon-hunter@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Romit Dasgupta <romit@ti.com>
> Cc: Sanjeev Premi <premi@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
> Cc: SuiLun Lam <s-lam@ti.com>
> Cc: Thara Gopinath <thara@ti.com>
> Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>
>
> Signed-off-by: Sanjeev Premi <premi@ti.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>

Some general style comments:

- Very good and throrough commenting, Nice!  Some minor changes needed
  - use kerneldoc style throughout (applies to other patches as well)
  - use standard multi-line comments

- overuse of BUG().  Failures should be more graceful, returning
  errors and checking errors in callers

- EXPORT_SYMBOL(): none of these OPP accessors should be exported to
  drivers.  This is PM core code only.

General design comments:

OPP accessor functions don't belong in omap-pm.h.  A new OPP header
should be used.  Maybe <plat/opp.h>

May be personal preference, but I'm not crazy about the implementation
of the accessor functions.  I think moving to a list and using the
standard list iterators will clean this up

I know we discussed this on the initial call and some said that
walking an array would be faster.  But looking at the code, I think
the readability would improve greatly and we don't need OPP tables
with dummy first and last entries etc.

A list implementation would also allow you make generalized iterators
like is done in the clkdm and pwrdm code (see pwrdm_for_each,
clkdm_for_each.)

At a high level, I'm not in line with the general direction here.  In
particular, this series continues the use of passing around OPP IDs
instead of real world values like frequencies.  I think a general
cleanup is needed to get rid of the OPP IDs, so either real world
values (frequencies and voltages) are passed around or pointers to OPP
structs are passed.

I know one of your goals was to make this less intrusive, but for me,
nothing in the current OPP layer and SRF is upstreamable anyways, so I
have no objectsion to intrusive changes to these layers. 

More detailed comments below...

> ---
>  arch/arm/mach-omap2/omap3-opp.h           |   40 +-------
>  arch/arm/mach-omap2/pm.c                  |  160 +++++++++++++++++++++++++++++
>  arch/arm/mach-omap2/pm34xx.c              |   42 ++++++++
>  arch/arm/plat-omap/include/plat/omap-pm.h |  109 ++++++++++++++++++++
>  4 files changed, 314 insertions(+), 37 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
> index 42557e1..27e2ca5 100644
> --- a/arch/arm/mach-omap2/omap3-opp.h
> +++ b/arch/arm/mach-omap2/omap3-opp.h
> @@ -21,42 +21,8 @@
>  #define S83M    83000000
>  #define S166M   166000000
>  
> -static struct omap_opp omap3_mpu_rate_table[] = {
> -	{0, 0, 0, 0},
> -	/*OPP1*/
> -	{true, S125M, VDD1_OPP1, 0x1E},
> -	/*OPP2*/
> -	{true, S250M, VDD1_OPP2, 0x26},
> -	/*OPP3*/
> -	{true, S500M, VDD1_OPP3, 0x30},
> -	/*OPP4*/
> -	{true, S550M, VDD1_OPP4, 0x36},
> -	/*OPP5*/
> -	{true, S600M, VDD1_OPP5, 0x3C},
> -};
> -
> -static struct omap_opp omap3_l3_rate_table[] = {
> -	{0, 0, 0, 0},
> -	/*OPP1*/
> -	{false, 0, VDD2_OPP1, 0x1E},
> -	/*OPP2*/
> -	{true, S83M, VDD2_OPP2, 0x24},
> -	/*OPP3*/
> -	{true, S166M, VDD2_OPP3, 0x2C},
> -};
> -
> -static struct omap_opp omap3_dsp_rate_table[] = {
> -	{0, 0, 0, 0},
> -	/*OPP1*/
> -	{true, S90M, VDD1_OPP1, 0x1E},
> -	/*OPP2*/
> -	{true, S180M, VDD1_OPP2, 0x26},
> -	/*OPP3*/
> -	{true, S360M, VDD1_OPP3, 0x30},
> -	/*OPP4*/
> -	{true, S400M, VDD1_OPP4, 0x36},
> -	/*OPP5*/
> -	{true, S430M, VDD1_OPP5, 0x3C},
> -};
> +extern struct omap_opp omap3_mpu_rate_table[];
> +extern struct omap_opp omap3_dsp_rate_table[];
> +extern struct omap_opp omap3_l3_rate_table[];
>  
>  #endif
> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
> index f50e93d..b2cd30c 100644
> --- a/arch/arm/mach-omap2/pm.c
> +++ b/arch/arm/mach-omap2/pm.c
> @@ -33,6 +33,7 @@
>  #include <plat/powerdomain.h>
>  #include <plat/resource.h>
>  #include <plat/omap34xx.h>
> +#include <plat/omap-pm.h>
>  
>  #include "prm-regbits-34xx.h"
>  #include "pm.h"
> @@ -203,3 +204,162 @@ static int __init omap_pm_init(void)
>  	return error;
>  }
>  late_initcall(omap_pm_init);
> +
> +int opp_to_freq(unsigned long *freq, const struct omap_opp *opps, u8 opp_id)
> +{
> +	int i = 1;
> +
> +	BUG_ON(!freq || !opps);
> +
> +	/* The first entry is a dummy one, loop till we hit terminator */
> +	while (!IS_OPP_TERMINATOR(opps, i)) {
> +		if (opps[i].enabled && (opps[i].opp_id == opp_id)) {
> +			*freq = opps[i].rate;
> +			return 0;
> +		}
> +		i++;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(opp_to_freq);

This one is only ever used in SRF, since SRF assumes the existence of
OPP IDs.

I still think we need to drop the concept of OPP IDs all together.

In setting an OPP through SRF, we turn a frequency into an OPP id,
then pass the OPP id through the multiple layers of SRF only to turn
it back into a frequency and then call the clk API.  This is along
path with little benefit that I can see.

> +int freq_to_vsel(u8 *vsel, const struct omap_opp *opps, unsigned long freq)
> +{
> +	int i = 1;
> +
> +	BUG_ON(!vsel || !opps);
> +
> +	/* The first entry is a dummy one, loop till we hit terminator */
> +	while (!IS_OPP_TERMINATOR(opps, i)) {
> +		if (opps[i].enabled && (opps[i].rate == freq)) {
> +			*vsel = opps[i].vsel;
> +			return 0;
> +		}
> +		i++;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(freq_to_vsel);

I don't see the need for this function.  In the places where it's
used, the OPP is already known so another table walk isn't required.
All that's needed is something like opp_get_vsel() which just does a
'return opp->vsel'.

That being said, another thing that is needed (not necessarily this
round) is getting rid of VSEL values from the OPPs all together in
favor of just the voltages.  Then, a VC layer would convert the
voltage into the HW specific value.

> +int freq_to_opp(u8 *opp_id, const struct omap_opp *opps, unsigned long freq)
> +{
> +	int i = 1;
> +
> +	BUG_ON(!opp_id || !opps);
> +
> +	/* The first entry is a dummy one, loop till we hit terminator */
> +	while (!IS_OPP_TERMINATOR(opps, i)) {
> +		if (opps[i].enabled && (opps[i].rate == freq)) {
> +			*opp_id = opps[i].opp_id;
> +			return 0;
> +		}
> +		i++;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(freq_to_opp);

This one is also built on the assumption of OPP IDs. being passed
around instead of frequencies.

> +int opp_enable(struct omap_opp *opps, unsigned long freq, bool enable)
> +{
> +	int i = 1;
> +
> +	BUG_ON(!opps);
> +
> +	/* The first entry is a dummy one, loop till we hit terminator */
> +	while (!IS_OPP_TERMINATOR(opps, i)) {
> +		if (opps[i].rate == freq) {
> +			opps[i].enabled = enable;
> +			return 0;
> +		}
> +		i++;
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(opp_enable);

Don't see any users of this one.

> +int get_next_freq(unsigned long *freq, const struct omap_opp *opps,
> +		bool search_higher, bool search_enabled_only, bool exact_search)
> +{
> +	int i = 1, inc = 1;
> +	bool found = false;
> +
> +	BUG_ON(!opps || !freq || !(*freq));
> +
> +	/* The first entry is a dummy one, loop till we hit terminator
> +	 * XXX: The following algorithm works only on a presorted
> +	 * list of OPPs
> +	 */
> +	while (!IS_OPP_TERMINATOR(opps, i)) {
> +		/* if we found the original freq, then
> +		 * case 1: enabled search ONLY, check opp is enabled or not
> +		 * case 2: the next available freq if enabled is not searched
> +		 */
> +		if ((found && search_enabled_only && opps[i].enabled) ||
> +		    (found && !search_enabled_only)) {
> +			*freq = opps[i].rate;
> +			return 0;
> +		}
> +
> +		/* Find the baseline freq first.. */
> +		if (!found && ((exact_search && opps[i].rate == *freq) ||
> +				(!exact_search && opps[i].rate >= *freq))) {
> +			/* found.. next decide direction */
> +			inc = search_higher ? 1 : -1;
> +			found = true;
> +			/* handle an exception case for exact search.. */
> +			if (exact_search || !search_higher)
> +				i += inc;
> +			/* fall thru to search for the right match */
> +		} else {
> +			i += inc;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL(get_next_freq);

I think all the options to this function make it not terribly
readable.  I see two users of this:

1) populating CPUfreq table.  

For this, using a list implmentation, something like this would suffice

  list_for_each(opp, &opp_list, node) {
      if (opp->enabled)
         freq_table[i].frequency = opp->freq;
  }

2) rounding up in resource34xx.c

Rather than having all the various options, I think a single function
for finding an OPP given a frequency  is needed.  Something like this:

#define OMAP_OPP_ROUND_NONE  0x0
#define OMAP_OPP_ROUND_UP    BIT(1)
#define OMAP_OPP_ROUND_DOWN  BIT(2)

/**
 * omap_opp_find_by_freq()
 * @opps:        Pointer to OPP table
 * @freq:        Frequency to look for in OPP table
 * @rounding:    Rounding option: NONE, UP, DOWN
 *
 * Look for an OPP with a frequency value matching @freq.  If
 * @rounding != ROUND_NONE, find closest match using rounding.
 * 
 * Returns pointer to OPP entry if match is found, or NULL if no match
 * found (only possible when no rounding is used)
 */
struct omap_opp *opp_find_by_freq(struct omap_opp *opps,
				  u32 freq, u32 rounding);

This could then be used for any lookup of OPP based on frequency.
For exact matches, use OMAP_OPP_ROUND_NONE, otherwise the other
rounding options can be used to find closest matches.

> +int get_limit_freq(unsigned long *freq, const struct omap_opp *opps,
> +		bool search_highest, bool search_enabled_only)
> +{
> +	int i = 1;
> +	unsigned long cur_freq_match = search_highest ? 0 : -1;
> +	bool found = false;
> +
> +	BUG_ON(!opps || !freq);
> +
> +	/* The first entry is a dummy one, loop till we hit terminator
> +	 * XXX: The following algorithm works only on a presorted
> +	 * list of OPPs
> +	 * We could use get_next_freq to search, but that will tend
> +	 * to be inefficient
> +	 */
> +	while (!IS_OPP_TERMINATOR(opps, i)) {
> +		/* match condition:
> +		 * check if the enabled cases match (only invalid case is:
> +		 * search_enabled=1,enabled=0)
> +		 * then we look for comparison condition, based on direction
> +		 */
> +		if (!(search_enabled_only && !opps[i].enabled) &&
> +		     ((search_highest && (opps[i].rate > cur_freq_match)) ||
> +		     (!search_highest && (opps[i].rate < cur_freq_match)))) {
> +			cur_freq_match = opps[i].rate;
> +			found = true;
> +			/* if we are searching for least, the first match
> +			 * is the right one, look no further.
> +			 */
> +			if (!search_highest)
> +				break;
> +		}
> +		i++;
> +	}
> +	if (!found)
> +		return -EINVAL;
> +	*freq = cur_freq_match;
> +	return 0;
> +}
> +EXPORT_SYMBOL(get_limit_freq);

Again, based on the users of this function, I don't see the reasons
for the various options.

1) populating CPUfreq table at init

This function is used to get the number of OPPs to allocate the
CPUfreq table.  Instead, I'd rather see this count calculated on
init when the OPP tables are initialized, and a single function
to get that value.   No need to do table walking when this
number can be updated whenever OPPs are enabled/disabled.

Something like this:

/**
 * opp_get_opp_count()
 * @opps: Pointer to OPP table
 *
 * Returns number of enabled OPPs.
 */
size_t opp_get_opp_count(struct omap_opp *opps);

2) Getting max value in SRF for L3 Freq.

If using the above 'find_opp_by_freq' function with rounding, there
should be no need to find a 'max' value.

Kevin

Re: [PATCH 8/9 v2] omap3: pm: introduce dynamic OPP

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> OPP tables have been used as static arrays till now this causes
> limitations in terms of ability to dynamically making decisions
> for cpu types and populating it with the right values. This
> implementation is based on the discussions in the thread:
> http://marc.info/?l=linux-omap&m=125482970102327&w=2
> inputs from Romit, Benoit, Kevin, Sanjeev, Santosh acked.
>
> omap3_pm_init_opp_table is introduced here, and the default omap3
> tables have been renamed to omap34xx_ tables, original omap3_xx
> tables are now dynamically allocated and populated.
>
> Corresponding board files calling omap2_init_common_hw with the
> active opp table params have been updated with this patch.
>
> This now paves way to introduce 3630 OPP tables.
>
> Tested on: SDP3430 - SDP3630 boot breaks with this patch
> (as SDP3630 needs corresponding correct VDD2 freq w.r.t SDRC clk.
> The next patch in this series fixes things)
>
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Jon Hunter <jon-hunter@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Romit Dasgupta <romit@ti.com>
> Cc: Sanjeev Premi <premi@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
> Cc: SuiLun Lam <s-lam@ti.com>
> Cc: Thara Gopinath <thara@ti.com>
> Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>
>
> Signed-off-by: Nishanth Menon <nm@ti.com>

[...]

> +void __init omap3_pm_init_opp_table(void)
> +{
> +	/* Populate the base CPU rate tables here */
> +	omap3_mpu_rate_table = kmalloc(sizeof(omap34xx_mpu_rate_table),
> +			GFP_KERNEL);
> +	omap3_dsp_rate_table = kmalloc(sizeof(omap34xx_dsp_rate_table),
> +			GFP_KERNEL);
> +	omap3_l3_rate_table = kmalloc(sizeof(omap34xx_l3_rate_table),
> +			GFP_KERNEL);
> +
> +	BUG_ON(!omap3_mpu_rate_table || !omap3_dsp_rate_table ||
> +		!omap3_l3_rate_table);
> +
> +	memcpy(omap3_mpu_rate_table, omap34xx_mpu_rate_table,
> +			sizeof(omap34xx_mpu_rate_table));
> +	memcpy(omap3_dsp_rate_table, omap34xx_dsp_rate_table,
> +			sizeof(omap34xx_dsp_rate_table));
> +	memcpy(omap3_l3_rate_table, omap34xx_l3_rate_table,
> +			sizeof(omap34xx_l3_rate_table));
> +}
> +

Minor issue, but FYI... rather than the kmalloc + memcpy, you can use
kmemdup() to do the same thing.

Kevin

Re: [PATCH 8/9 v2] omap3: pm: introduce dynamic OPP

[Menon, Nishanth]

Kevin Hilman said the following on 11/13/2009 07:31 PM:
[...]
>> +void __init omap3_pm_init_opp_table(void)
>> +{
>> +	/* Populate the base CPU rate tables here */
>> +	omap3_mpu_rate_table = kmalloc(sizeof(omap34xx_mpu_rate_table),
>> +			GFP_KERNEL);
>> +	omap3_dsp_rate_table = kmalloc(sizeof(omap34xx_dsp_rate_table),
>> +			GFP_KERNEL);
>> +	omap3_l3_rate_table = kmalloc(sizeof(omap34xx_l3_rate_table),
>> +			GFP_KERNEL);
>> +
>> +	BUG_ON(!omap3_mpu_rate_table || !omap3_dsp_rate_table ||
>> +		!omap3_l3_rate_table);
>> +
>> +	memcpy(omap3_mpu_rate_table, omap34xx_mpu_rate_table,
>> +			sizeof(omap34xx_mpu_rate_table));
>> +	memcpy(omap3_dsp_rate_table, omap34xx_dsp_rate_table,
>> +			sizeof(omap34xx_dsp_rate_table));
>> +	memcpy(omap3_l3_rate_table, omap34xx_l3_rate_table,
>> +			sizeof(omap34xx_l3_rate_table));
>> +}
>> +
>>     
>
> Minor issue, but FYI... rather than the kmalloc + memcpy, you can use
> kmemdup() to do the same thing
thanks.
Regards,
Nishanth Menon

Re: [PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Menon, Nishanth]

Kevin,

Firstly, thanks for your detailed comments.
This series looked at the problem as 2 objectives:
a) Introduce OPP accessor functions -> this series shows it is possible
b) Improve the implementation with discussions. I am all for it. I know
spending 7-14 hrs  like what I did on this, is not enough to bring in
the best of the implementation.


Kevin Hilman said the following on 11/13/2009 06:58 PM:
> Nishanth Menon <nm@ti.com> writes:
>
>   
[...]
>> Signed-off-by: Sanjeev Premi <premi@ti.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>>     
>
> Some general style comments:
>
> - Very good and throrough commenting, Nice!  Some minor changes needed
>   - use kerneldoc style throughout (applies to other patches as well)
>   - use standard multi-line comments
>   
/me kicking myself for not reading Documentation/ properly.. Ack. will
read and fix.

> - overuse of BUG().  Failures should be more graceful, returning
>   errors and checking errors in callers
>   
heh heh.. I am a bit of a paranoid for good reasons.. The rule I
followed is: BUG() has been used with static functions, and critical
functions that expect params. it is better to flag it to developer that
the assumptions of the function are not being met by the caller..
forcing an earlier fix than having to deal with half a dozen functions
that never check return values!! I dont mind discussion which locations
BUG is superfluous and where it is essential.

> - EXPORT_SYMBOL(): none of these OPP accessors should be exported to
>   drivers.  This is PM core code only.
>   
Ack.
> General design comments:
>
> OPP accessor functions don't belong in omap-pm.h.  A new OPP header
> should be used.  Maybe <plat/opp.h>
>   
What is the benefit? opp is a pm feature right?

> May be personal preference, but I'm not crazy about the implementation
> of the accessor functions.  I think moving to a list and using the
> standard list iterators will clean this up
>   
Why do i get a "deja-vu" feel about that statement :P..
but then, there are three alternatives, that I see:
a) array -straight walk.. (Order n)
b) list iterator: (Order n)
c) array - binary search (Order log n) <- should have used this.
d) array - hash table (Order 1) <- want to use this, but need to spend
some cool thinking time to write one hash function..

I personally like (d). But in any case, this is nothing but a bunch of
function implementation (and possibly structure improvements). I like to
think of it as phase 2 of accessor functions.. let me know if you disagree.

> I know we discussed this on the initial call and some said that
> walking an array would be faster.  But looking at the code, I think
> the readability would improve greatly and we don't need OPP tables
> with dummy first and last entries etc.
>   
yes I know -> look above. readability needs to match performance for a
critical path such as these functions.

> A list implementation would also allow you make generalized iterators
> like is done in the clkdm and pwrdm code (see pwrdm_for_each,
> clkdm_for_each.)
>   
one of my initial proposals ;)... but as I shown above, I think there
are alternatives instead of using list iterators.
> At a high level, I'm not in line with the general direction here.  In
> particular, this series continues the use of passing around OPP IDs
> instead of real world values like frequencies.  I think a general
> cleanup is needed to get rid of the OPP IDs, so either real world
> values (frequencies and voltages) are passed around or pointers to OPP
> structs are passed.
>   
I agree -> if you look at my accessor functions -> ONLY one function has
opp ID mapping. if you kill that function in the future, none of the
other users can use OPPID at all.. I think that is a phase 2 again..

> I know one of your goals was to make this less intrusive, but for me,
> nothing in the current OPP layer and SRF is upstreamable anyways, so I
> have no objectsion to intrusive changes to these layers. 
>   
unfortunately, I know of a bunch of folks who might riot if I rewrote
SRF (as I was itching to do) when I started dabbling with it..  further,
I do not have the bandwidth to do that kind of rewrite and end up
perpetually keeping 3630 patches at bay..

> More detailed comments below...
>
>   
>> ---
>>  arch/arm/mach-omap2/omap3-opp.h           |   40 +-------
>>  arch/arm/mach-omap2/pm.c                  |  160 +++++++++++++++++++++++++++++
>>  arch/arm/mach-omap2/pm34xx.c              |   42 ++++++++
>>  arch/arm/plat-omap/include/plat/omap-pm.h |  109 ++++++++++++++++++++
>>  4 files changed, 314 insertions(+), 37 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
>> index 42557e1..27e2ca5 100644
>> --- a/arch/arm/mach-omap2/omap3-opp.h
>> +++ b/arch/arm/mach-omap2/omap3-opp.h
>> @@ -21,42 +21,8 @@
>>  #define S83M    83000000
>>  #define S166M   166000000
>>  
>> -static struct omap_opp omap3_mpu_rate_table[] = {
>> -	{0, 0, 0, 0},
>> -	/*OPP1*/
>> -	{true, S125M, VDD1_OPP1, 0x1E},
>> -	/*OPP2*/
>> -	{true, S250M, VDD1_OPP2, 0x26},
>> -	/*OPP3*/
>> -	{true, S500M, VDD1_OPP3, 0x30},
>> -	/*OPP4*/
>> -	{true, S550M, VDD1_OPP4, 0x36},
>> -	/*OPP5*/
>> -	{true, S600M, VDD1_OPP5, 0x3C},
>> -};
>> -
>> -static struct omap_opp omap3_l3_rate_table[] = {
>> -	{0, 0, 0, 0},
>> -	/*OPP1*/
>> -	{false, 0, VDD2_OPP1, 0x1E},
>> -	/*OPP2*/
>> -	{true, S83M, VDD2_OPP2, 0x24},
>> -	/*OPP3*/
>> -	{true, S166M, VDD2_OPP3, 0x2C},
>> -};
>> -
>> -static struct omap_opp omap3_dsp_rate_table[] = {
>> -	{0, 0, 0, 0},
>> -	/*OPP1*/
>> -	{true, S90M, VDD1_OPP1, 0x1E},
>> -	/*OPP2*/
>> -	{true, S180M, VDD1_OPP2, 0x26},
>> -	/*OPP3*/
>> -	{true, S360M, VDD1_OPP3, 0x30},
>> -	/*OPP4*/
>> -	{true, S400M, VDD1_OPP4, 0x36},
>> -	/*OPP5*/
>> -	{true, S430M, VDD1_OPP5, 0x3C},
>> -};
>> +extern struct omap_opp omap3_mpu_rate_table[];
>> +extern struct omap_opp omap3_dsp_rate_table[];
>> +extern struct omap_opp omap3_l3_rate_table[];
>>  
>>  #endif
>> diff --git a/arch/arm/mach-omap2/pm.c b/arch/arm/mach-omap2/pm.c
>> index f50e93d..b2cd30c 100644
>> --- a/arch/arm/mach-omap2/pm.c
>> +++ b/arch/arm/mach-omap2/pm.c
>> @@ -33,6 +33,7 @@
>>  #include <plat/powerdomain.h>
>>  #include <plat/resource.h>
>>  #include <plat/omap34xx.h>
>> +#include <plat/omap-pm.h>
>>  
>>  #include "prm-regbits-34xx.h"
>>  #include "pm.h"
>> @@ -203,3 +204,162 @@ static int __init omap_pm_init(void)
>>  	return error;
>>  }
>>  late_initcall(omap_pm_init);
>> +
>> +int opp_to_freq(unsigned long *freq, const struct omap_opp *opps, u8 opp_id)
>> +{
>> +	int i = 1;
>> +
>> +	BUG_ON(!freq || !opps);
>> +
>> +	/* The first entry is a dummy one, loop till we hit terminator */
>> +	while (!IS_OPP_TERMINATOR(opps, i)) {
>> +		if (opps[i].enabled && (opps[i].opp_id == opp_id)) {
>> +			*freq = opps[i].rate;
>> +			return 0;
>> +		}
>> +		i++;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(opp_to_freq);
>>     
>
> This one is only ever used in SRF, since SRF assumes the existence of
> OPP IDs.
>   
yeah I know.. so far there is no replacement for SRF, so even if I dont
like the code, I need to keep it alive.. till an alternative such as
hwmod becomes reality.

> I still think we need to drop the concept of OPP IDs all together.
>
> In setting an OPP through SRF, we turn a frequency into an OPP id,
> then pass the OPP id through the multiple layers of SRF only to turn
> it back into a frequency and then call the clk API.  This is along
> path with little benefit that I can see.
>   
I agree, that was the reason why I wanted to rewrite the darn SRF
altogether.. that was not the intention of this series.. it is just a
start of cleanup of opp concept cleanup and moving to using frequencies
all over.

>   
>> +int freq_to_vsel(u8 *vsel, const struct omap_opp *opps, unsigned long freq)
>> +{
>> +	int i = 1;
>> +
>> +	BUG_ON(!vsel || !opps);
>> +
>> +	/* The first entry is a dummy one, loop till we hit terminator */
>> +	while (!IS_OPP_TERMINATOR(opps, i)) {
>> +		if (opps[i].enabled && (opps[i].rate == freq)) {
>> +			*vsel = opps[i].vsel;
>> +			return 0;
>> +		}
>> +		i++;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(freq_to_vsel);
>>     
>
> I don't see the need for this function.  In the places where it's
> used, the OPP is already known so another table walk isn't required.
> All that's needed is something like opp_get_vsel() which just does a
> 'return opp->vsel'.
>   
voltage is linked to frequency - it is simple as that. the caller should
NOT know the implementation of omap_opp.
So, you need this function, the implementation, I agree is crazy and can
be improved as discussed above.

> That being said, another thing that is needed (not necessarily this
> round) is getting rid of VSEL values from the OPPs all together in
> favor of just the voltages.  Then, a VC layer would convert the
> voltage into the HW specific value.
>   
ACK.
>   
>> +int freq_to_opp(u8 *opp_id, const struct omap_opp *opps, unsigned long freq)
>> +{
>> +	int i = 1;
>> +
>> +	BUG_ON(!opp_id || !opps);
>> +
>> +	/* The first entry is a dummy one, loop till we hit terminator */
>> +	while (!IS_OPP_TERMINATOR(opps, i)) {
>> +		if (opps[i].enabled && (opps[i].rate == freq)) {
>> +			*opp_id = opps[i].opp_id;
>> +			return 0;
>> +		}
>> +		i++;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(freq_to_opp);
>>     
>
> This one is also built on the assumption of OPP IDs. being passed
> around instead of frequencies.
>   
As you see, this is the ONLY function which uses opp IDs. the idea was
using this as a stand-in while either SRF gets cleaned up OR some other
alternative comes along.. but without intrusive change SRF will need
this. I am unable to do this in reasonable timeframe..
>   
>> +int opp_enable(struct omap_opp *opps, unsigned long freq, bool enable)
>> +{
>> +	int i = 1;
>> +
>> +	BUG_ON(!opps);
>> +
>> +	/* The first entry is a dummy one, loop till we hit terminator */
>> +	while (!IS_OPP_TERMINATOR(opps, i)) {
>> +		if (opps[i].rate == freq) {
>> +			opps[i].enabled = enable;
>> +			return 0;
>> +		}
>> +		i++;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(opp_enable);
>>     
>
> Don't see any users of this one.
>   
yep, that is because you dont need users of this one so far (37xx series
OR other 35xx series of chips) have not come in yet.. ;).. we will have
all kind of users soon, but as discussed previously, there is a real
need for this.
>   
>> +int get_next_freq(unsigned long *freq, const struct omap_opp *opps,
>> +		bool search_higher, bool search_enabled_only, bool exact_search)
>> +{
>> +	int i = 1, inc = 1;
>> +	bool found = false;
>> +
>> +	BUG_ON(!opps || !freq || !(*freq));
>> +
>> +	/* The first entry is a dummy one, loop till we hit terminator
>> +	 * XXX: The following algorithm works only on a presorted
>> +	 * list of OPPs
>> +	 */
>> +	while (!IS_OPP_TERMINATOR(opps, i)) {
>> +		/* if we found the original freq, then
>> +		 * case 1: enabled search ONLY, check opp is enabled or not
>> +		 * case 2: the next available freq if enabled is not searched
>> +		 */
>> +		if ((found && search_enabled_only && opps[i].enabled) ||
>> +		    (found && !search_enabled_only)) {
>> +			*freq = opps[i].rate;
>> +			return 0;
>> +		}
>> +
>> +		/* Find the baseline freq first.. */
>> +		if (!found && ((exact_search && opps[i].rate == *freq) ||
>> +				(!exact_search && opps[i].rate >= *freq))) {
>> +			/* found.. next decide direction */
>> +			inc = search_higher ? 1 : -1;
>> +			found = true;
>> +			/* handle an exception case for exact search.. */
>> +			if (exact_search || !search_higher)
>> +				i += inc;
>> +			/* fall thru to search for the right match */
>> +		} else {
>> +			i += inc;
>> +		}
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>> +EXPORT_SYMBOL(get_next_freq);
>>     
>
> I think all the options to this function make it not terribly
> readable.  I see two users of this:
>
> 1) populating CPUfreq table.  
>
> For this, using a list implmentation, something like this would suffice
>
>   list_for_each(opp, &opp_list, node) {
>       if (opp->enabled)
>          freq_table[i].frequency = opp->freq;
>   }
>   
you are exposing the structer of omap_opp to cpu_freq - this was the
kind of thing that got us into this mess. what cpu_freq needs are three
functions:
get_num_opp()
get_highest_opp()
get_higher_opp(freq).

> 2) rounding up in resource34xx.c
>
> Rather than having all the various options, I think a single function
>   
Yep.. I can go all out for flexbility OR constraint.. the searcher
accessor functions can be written better- yes.
> for finding an OPP given a frequency  is needed.  Something like this:
>
> #define OMAP_OPP_ROUND_NONE  0x0
> #define OMAP_OPP_ROUND_UP    BIT(1)
> #define OMAP_OPP_ROUND_DOWN  BIT(2)
>
> /**
>  * omap_opp_find_by_freq()
>  * @opps:        Pointer to OPP table
>  * @freq:        Frequency to look for in OPP table
>  * @rounding:    Rounding option: NONE, UP, DOWN
>  *
>  * Look for an OPP with a frequency value matching @freq.  If
>  * @rounding != ROUND_NONE, find closest match using rounding.
>  * 
>  * Returns pointer to OPP entry if match is found, or NULL if no match
>  * found (only possible when no rounding is used)
>  */
> struct omap_opp *opp_find_by_freq(struct omap_opp *opps,
> 				  u32 freq, u32 rounding);
>
> This could then be used for any lookup of OPP based on frequency.
> For exact matches, use OMAP_OPP_ROUND_NONE, otherwise the other
> rounding options can be used to find closest matches.
>   
NAK: simply because, I do not agree that we should expose omap_opp to
caller. they need to know freq. this allows
OPP logic to change in the future and allow omap_opp structure itself to
be modified without any impact to callers.

on specific OMAP_OPP_ROUND_NONE etc, I think it is better to split up my
initial single accessor function into multiple ones -> allows better
readability.

>   
>> +int get_limit_freq(unsigned long *freq, const struct omap_opp *opps,
>> +		bool search_highest, bool search_enabled_only)
>> +{
>> +	int i = 1;
>> +	unsigned long cur_freq_match = search_highest ? 0 : -1;
>> +	bool found = false;
>> +
>> +	BUG_ON(!opps || !freq);
>> +
>> +	/* The first entry is a dummy one, loop till we hit terminator
>> +	 * XXX: The following algorithm works only on a presorted
>> +	 * list of OPPs
>> +	 * We could use get_next_freq to search, but that will tend
>> +	 * to be inefficient
>> +	 */
>> +	while (!IS_OPP_TERMINATOR(opps, i)) {
>> +		/* match condition:
>> +		 * check if the enabled cases match (only invalid case is:
>> +		 * search_enabled=1,enabled=0)
>> +		 * then we look for comparison condition, based on direction
>> +		 */
>> +		if (!(search_enabled_only && !opps[i].enabled) &&
>> +		     ((search_highest && (opps[i].rate > cur_freq_match)) ||
>> +		     (!search_highest && (opps[i].rate < cur_freq_match)))) {
>> +			cur_freq_match = opps[i].rate;
>> +			found = true;
>> +			/* if we are searching for least, the first match
>> +			 * is the right one, look no further.
>> +			 */
>> +			if (!search_highest)
>> +				break;
>> +		}
>> +		i++;
>> +	}
>> +	if (!found)
>> +		return -EINVAL;
>> +	*freq = cur_freq_match;
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(get_limit_freq);
>>     
>
> Again, based on the users of this function, I don't see the reasons
> for the various options.
>
> 1) populating CPUfreq table at init
>
> This function is used to get the number of OPPs to allocate the
> CPUfreq table.  Instead, I'd rather see this count calculated on
> init when the OPP tables are initialized, and a single function
> to get that value.   No need to do table walking when this
> number can be updated whenever OPPs are enabled/disabled.
>
> Something like this:
>
> /**
>  * opp_get_opp_count()
>  * @opps: Pointer to OPP table
>  *
>  * Returns number of enabled OPPs.
>  */
> size_t opp_get_opp_count(struct omap_opp *opps);
>   
ACK. (similar to see prev)
> 2) Getting max value in SRF for L3 Freq.
>
> If using the above 'find_opp_by_freq' function with rounding, there
> should be no need to find a 'max' value.
>   
yes, if you want to organize the frequencies in a certain order. (e.g.
for the interest of search algos elsewhere improving due to this..).


Overall:
a) You think SRF needs to change a lot, I agree, but this series is not
the place for it..
b) Improvements of the omap accessor functions, I will send out few of
the proposals I can think of later when I get some free time.. we can
discuss that too.
c) You think omap_opp structure should be used by callers, I dont agree
to the same - as I think this is a recipe for future mess (like the
current one).
d) We both agree that the current implementation of search sucks and
there are various approaches we could take.

Regards,
Nishanth Menon

Re: [PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Nishanth Menon]

Menon, Nishanth had written, on 11/15/2009 08:54 AM, the following:
[...]
> b) Improvements of the omap accessor functions, I will send out few of
> the proposals I can think of later when I get some free time.. we can
> discuss that too.
Here is my initial proposal, do feel free to comment/add/modify etc:

In the accessor functions listed here, I have followed:
a) The function will return 0 if success else appropriate error values, 
all values are passed by pointer params.
b) Overall, the idea is to transition from exposing omap_opp to users 
currently to a system where opp handling logic will be blackboxed from 
the rest of the system - giving us options to optimize and scale 
internal logic and storage mechanism without impacting the rest of the 
system.
c) resultant code should be readable - this is one of the issues I see 
with my previous implementation.

Files: Introduce opp.c and opp.h

A) Data Structures:
Interim:
opp.h
struct omap_opp {
	bool enabled;
	unsigned long rate; /* in hz */
	u8 opp_id; /* int */
	u16 vsel; /* T2 voltage value */
};
typedef struct omap_opp omap_opp_def;

Final, once resource34xx.c and all direct accesses are cleaned up:

opp.h:
struct omap_opp;

/* initial definitions of OPP, will be used by all */
struct omap_opp_def {
	bool enabled; /* true/false */
	unsigned long rate; /* in hz */
	u16 vsel; /* in millivolts */
};

opp.c:
struct omap_opp {
	bool enabled;
	unsigned long rate;
	u16 vsel;
};

or what ever implementation it needs. can be array, list, hash 
implementation or what ever we choose it to be.

B) Accessor functions to be used:

B.1) These functions should be removed once SRF is replaced/cleaned up:

int opp_id_to_freq(unsigned long *freq, const struct omap_opp *opps, u8 
opp_id);
int opp_freq_to_id(u8 *opp_id, const struct omap_opp *opps, unsigned 
long freq);


B.2) initialization functions:

/* Register the OPP list for the silicon */
int opp_register(struct omap_opp *opp_list,
		 const struct omap_opp_def *opp_reg);


/* Enable a specific frequency */
int opp_enable(struct omap_opp *opp_list,
		unsigned long freq, bool enable);

Users:
opp_register will be called by pm34xx.c based on silicon type.
opp_enable will be called by board file/ has_feature based logic etc.. 
on a need basis

B.3) verification functions:

/* Check if we have a  frequency in the list(enabled/disabled)
  * this is needed by users who would like to enable a disabled
  * frequency
  */
int opp_has_freq(struct omap_opp *opp_list, unsigned long freq);

/* Check if we have a enabled frequency - this is what most users
  * will need
  */
int opp_is_freq_valid(struct omap_opp *opp_list, unsigned long freq);

Users:
	files who'd want to use opp_enable,
	validation paths.

B.4) Limit functions:

/* To get a number of frequencies enabled */
int opp_get_freq_count(const struct omap_opp *opp_list, u8 *num);

/* Get highest enabled frequency */
int opp_get_highest_freq(struct omap_opp *opp_list,
	unsigned long *freq);

/* Get lowest enabled frequency */
int opp_get_lowest_freq(struct omap_opp *opp_list, unsigned long *freq);

Users:
	Obvious user is the current cpufreq

B.5) Search functions -> these will check only enabled frequencies. This 
will not check if the start freq provided in *freq is an enabled 
frequency or not. Assumption I am making is: if you are searching for a 
frequency that is disabled in the opp list, you are not trying something 
generic -hence wont support.

/* Get higher enabled frequency than the provided one */
int opp_get_higher_freq(struct omap_opp *opp_list, unsigned long *freq);

/* Get lower enabled frequency than the provided one */
int opp_get_lower_freq(struct omap_opp *opp_list, unsigned long *freq);

Users:
	current srf, future scale logic

B.6) voltage functions - opp layer will not trigger voltage transition, 
it is upto other users of opp layer to make a decision.

/* get voltage corresponding to a frequency */
int opp_freq_to_voltage(u16 *voltage, struct omap_opp *opp_list,
			unsigned long *freq);

Users:
	current SR/SRF, future voltage framework.

-- 
Regards,
Nishanth Menon

Re: [PATCH 9/9 v2] omap3: pm: introduce 3630 opps

[Jon Hunter]

Menon, Nishanth wrote:
> Introduce the OMAP3630 OPPs including the defined OPP tuples.
> 
> Further information on OMAP3630 can be found here:
> http://focus.ti.com/general/docs/wtbu/wtbuproductcontent.tsp?templateId=6123&navigationId=12836&contentId=52606
> 
> OMAP36xx family introduces:
> VDD1 with 4 OPPs, of which OPP3 & 4 are available only on devices yet
> to be introduced in 36xx family. Meanwhile, VDD2 has 2 opps.
> 
> Range of OPPs supported by Devices(tentative)->
>            |<-3630-600->| (default)
>            |<-      3630-800  ->| (device: TBD)
>            |<-      3630-1000             ->| (device: TBD)
> H/w OPP-> OPP50 OPP100       OPP-Turbo   OPP1G-SB
> VDD1      OPP1  OPP2         OPP3        OPP4
> VDD2      OPP1  OPP2         OPP2        OPP2
> 
> Note:
> a) TI h/w naming for OPPs are now standardized in terms of OPP50, 100,
>    Turbo and SB. This maps as shown above to the opp IDs (s/w term).
> b) For boards which need custom VDD1/2 OPPs, the opp table can be
>    updated by the board file on a need basis after the
>    omap3_pm_init_opp_table call. The OPPs introduced here are the
>    official OPP table at this point in time.
> 
> Tested on: SDP3430, SDP3630
> 
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Jon Hunter <jon-hunter@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Romit Dasgupta <romit@ti.com>
> Cc: Sanjeev Premi <premi@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
> Cc: SuiLun Lam <s-lam@ti.com>
> Cc: Thara Gopinath <thara@ti.com>
> Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  arch/arm/mach-omap2/omap3-opp.h |    9 ++++
>  arch/arm/mach-omap2/pm34xx.c    |   87 +++++++++++++++++++++++++++++++-------
>  2 files changed, 80 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-opp.h
> index 7f27f44..a5880b8 100644
> --- a/arch/arm/mach-omap2/omap3-opp.h
> +++ b/arch/arm/mach-omap2/omap3-opp.h
> @@ -4,22 +4,31 @@
>  #include <plat/omap-pm.h>
>  
>  /* MPU speeds */
> +#define S1000M  1000000000
> +#define S800M   800000000
>  #define S600M   600000000
>  #define S550M   550000000
>  #define S500M   500000000
> +#define S300M   300000000
>  #define S250M   250000000
>  #define S125M   125000000
>  
>  /* DSP speeds */
> +#define S875M   875000000
> +#define S660M   660000000
> +#define S520M   520000000
>  #define S430M   430000000
>  #define S400M   400000000
>  #define S360M   360000000
> +#define S260M   260000000
>  #define S180M   180000000
>  #define S90M    90000000
>  
>  /* L3 speeds */
>  #define S83M    83000000
> +#define S100M   100000000
>  #define S166M   166000000
> +#define S200M   200000000
>  
>  extern struct omap_opp *omap3_mpu_rate_table;
>  extern struct omap_opp *omap3_dsp_rate_table;
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 14131f8..86137bb 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -141,6 +141,41 @@ static __initdata struct omap_opp omap34xx_dsp_rate_table[] = {
>  	{0, 0, 0, 0},
>  };
>  
> +static __initdata struct omap_opp omap36xx_mpu_rate_table[] = {
> +	{0, 0, 0, 0},
> +	/*OPP1 - 930mV - OPP50*/
> +	{true, S300M, VDD1_OPP1, 0x1a},
> +	/*OPP2 - 1.100V - OPP100*/
> +	{true, S600M, VDD1_OPP2, 0x28},
> +	/*OPP3 - 1.260V - OPP-Turbo*/
> +	{false, S800M, VDD1_OPP3, 0x34},
> +	/*OPP4 - 1.310V - OPP-SB*/
> +	{false, S1000M, VDD1_OPP4, 0x38},
> +	{0, 0, 0, 0},
> +};
> +
> +static __initdata struct omap_opp omap36xx_l3_rate_table[] = {
> +	{0, 0, 0, 0},
> +	/*OPP1 - 930mV - OPP50 */
> +	{true, S100M, VDD2_OPP1, 0x1a},
> +	/*OPP2 - 1.375V - OPP100, OPP-Turbo, OPP-SB*/


Nishanth, can you correct the above comment? This should be 1.1375V and 
not 1.375V.

Cheers
Jon

RE: [PATCH 9/9 v2] omap3: pm: introduce 3630 opps

[Sripathy, Vishwanath]

> -----Original Message-----
> From: Menon, Nishanth
> Sent: Friday, November 13, 2009 11:35 AM
> To: linux-omap
> Cc: Menon, Nishanth; Cousson, Benoit; Hunter, Jon; Kevin Hilman; Chikkature
> Rajashekar, Madhusudhan; Paul Walmsley; Dasgupta, Romit; Premi, Sanjeev;
> Shilimkar, Santosh; Aguirre, Sergio; Lam, SuiLun; Gopinath, Thara; Sripathy,
> Vishwanath
> Subject: [PATCH 9/9 v2] omap3: pm: introduce 3630 opps
> 
> Introduce the OMAP3630 OPPs including the defined OPP tuples.
> 
> Further information on OMAP3630 can be found here:
> http://focus.ti.com/general/docs/wtbu/wtbuproductcontent.tsp?templateId=6123&nav
> igationId=12836&contentId=52606
> 
> OMAP36xx family introduces:
> VDD1 with 4 OPPs, of which OPP3 & 4 are available only on devices yet
> to be introduced in 36xx family. Meanwhile, VDD2 has 2 opps.
> 
> Range of OPPs supported by Devices(tentative)->
>            |<-3630-600->| (default)
>            |<-      3630-800  ->| (device: TBD)
>            |<-      3630-1000             ->| (device: TBD)
> H/w OPP-> OPP50 OPP100       OPP-Turbo   OPP1G-SB
> VDD1      OPP1  OPP2         OPP3        OPP4
> VDD2      OPP1  OPP2         OPP2        OPP2
> 
> Note:
> a) TI h/w naming for OPPs are now standardized in terms of OPP50, 100,
>    Turbo and SB. This maps as shown above to the opp IDs (s/w term).
> b) For boards which need custom VDD1/2 OPPs, the opp table can be
>    updated by the board file on a need basis after the
>    omap3_pm_init_opp_table call. The OPPs introduced here are the
>    official OPP table at this point in time.
> 
> Tested on: SDP3430, SDP3630
> 
> Cc: Benoit Cousson <b-cousson@ti.com>
> Cc: Jon Hunter <jon-hunter@ti.com>
> Cc: Kevin Hilman <khilman@deeprootsystems.com>
> Cc: Madhusudhan Chikkature Rajashekar <madhu.cr@ti.com>
> Cc: Paul Walmsley <paul@pwsan.com>
> Cc: Romit Dasgupta <romit@ti.com>
> Cc: Sanjeev Premi <premi@ti.com>
> Cc: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Sergio Alberto Aguirre Rodriguez <saaguirre@ti.com>
> Cc: SuiLun Lam <s-lam@ti.com>
> Cc: Thara Gopinath <thara@ti.com>
> Cc: Vishwanath Sripathy <vishwanath.bs@ti.com>
> 
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  arch/arm/mach-omap2/omap3-opp.h |    9 ++++
>  arch/arm/mach-omap2/pm34xx.c    |   87
> +++++++++++++++++++++++++++++++-------
>  2 files changed, 80 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap3-opp.h b/arch/arm/mach-omap2/omap3-
> opp.h
> index 7f27f44..a5880b8 100644
> --- a/arch/arm/mach-omap2/omap3-opp.h
> +++ b/arch/arm/mach-omap2/omap3-opp.h
> @@ -4,22 +4,31 @@
>  #include <plat/omap-pm.h>
> 
>  /* MPU speeds */
> +#define S1000M  1000000000
> +#define S800M   800000000
>  #define S600M   600000000
>  #define S550M   550000000
>  #define S500M   500000000
> +#define S300M   300000000
>  #define S250M   250000000
>  #define S125M   125000000
> 
>  /* DSP speeds */
> +#define S875M   875000000
> +#define S660M   660000000
> +#define S520M   520000000
>  #define S430M   430000000
>  #define S400M   400000000
>  #define S360M   360000000
> +#define S260M   260000000
>  #define S180M   180000000
>  #define S90M    90000000
> 
>  /* L3 speeds */
>  #define S83M    83000000
> +#define S100M   100000000
>  #define S166M   166000000
> +#define S200M   200000000
> 
>  extern struct omap_opp *omap3_mpu_rate_table;
>  extern struct omap_opp *omap3_dsp_rate_table;
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 14131f8..86137bb 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -141,6 +141,41 @@ static __initdata struct omap_opp
> omap34xx_dsp_rate_table[] = {
>  	{0, 0, 0, 0},
>  };
> 
> +static __initdata struct omap_opp omap36xx_mpu_rate_table[] = {
> +	{0, 0, 0, 0},
> +	/*OPP1 - 930mV - OPP50*/
> +	{true, S300M, VDD1_OPP1, 0x1a},
> +	/*OPP2 - 1.100V - OPP100*/
> +	{true, S600M, VDD1_OPP2, 0x28},
> +	/*OPP3 - 1.260V - OPP-Turbo*/
> +	{false, S800M, VDD1_OPP3, 0x34},
> +	/*OPP4 - 1.310V - OPP-SB*/
> +	{false, S1000M, VDD1_OPP4, 0x38},
> +	{0, 0, 0, 0},
> +};

OPP Table vsel values seem to be wrong. Eg: for OPP1 to get 930mV, vsel should be 0x1b. vsel=Ceiling((930-600)/12.5)

> +
> +static __initdata struct omap_opp omap36xx_l3_rate_table[] = {
> +	{0, 0, 0, 0},
> +	/*OPP1 - 930mV - OPP50 */
> +	{true, S100M, VDD2_OPP1, 0x1a},
> +	/*OPP2 - 1.375V - OPP100, OPP-Turbo, OPP-SB*/
> +	{true, S200M, VDD2_OPP2, 0x2b},
> +	{0, 0, 0, 0},
> +};
> +


Same case

> +static __initdata struct omap_opp omap36xx_dsp_rate_table[] = {
> +	{0, 0, 0, 0},
> +	/*OPP1 - OPP50*/
> +	{true, S260M, VDD1_OPP1, 0x1a},
> +	/*OPP2 - OPP100*/
> +	{true, S520M, VDD1_OPP2, 0x28},
> +	/*OPP3 - OPP-Turbo*/
> +	{false, S660M, VDD1_OPP3, 0x34},
> +	/*OPP4 - OPP-SB*/
> +	{false, S875M, VDD1_OPP4, 0x38},
> +	{0, 0, 0, 0},
> +};
> +

Same case

>  struct omap_opp *omap3_mpu_rate_table;
>  struct omap_opp *omap3_dsp_rate_table;
>  struct omap_opp *omap3_l3_rate_table;
> @@ -1287,22 +1322,42 @@ static void __init configure_vc(void)
>  void __init omap3_pm_init_opp_table(void)
>  {
>  	/* Populate the base CPU rate tables here */
> -	omap3_mpu_rate_table = kmalloc(sizeof(omap34xx_mpu_rate_table),
> -			GFP_KERNEL);
> -	omap3_dsp_rate_table = kmalloc(sizeof(omap34xx_dsp_rate_table),
> -			GFP_KERNEL);
> -	omap3_l3_rate_table = kmalloc(sizeof(omap34xx_l3_rate_table),
> -			GFP_KERNEL);
> -
> -	BUG_ON(!omap3_mpu_rate_table || !omap3_dsp_rate_table ||
> -		!omap3_l3_rate_table);
> -
> -	memcpy(omap3_mpu_rate_table, omap34xx_mpu_rate_table,
> -			sizeof(omap34xx_mpu_rate_table));
> -	memcpy(omap3_dsp_rate_table, omap34xx_dsp_rate_table,
> -			sizeof(omap34xx_dsp_rate_table));
> -	memcpy(omap3_l3_rate_table, omap34xx_l3_rate_table,
> -			sizeof(omap34xx_l3_rate_table));
> +	if (cpu_is_omap3630()) {
> +		omap3_mpu_rate_table = kmalloc(sizeof(omap36xx_mpu_rate_table),
> +				GFP_KERNEL);
> +		omap3_dsp_rate_table = kmalloc(sizeof(omap36xx_dsp_rate_table),
> +				GFP_KERNEL);
> +		omap3_l3_rate_table = kmalloc(sizeof(omap36xx_l3_rate_table),
> +				GFP_KERNEL);
> +
> +		BUG_ON(!omap3_mpu_rate_table || !omap3_dsp_rate_table ||
> +			!omap3_l3_rate_table);
> +
> +		memcpy(omap3_mpu_rate_table, omap36xx_mpu_rate_table,
> +				sizeof(omap36xx_mpu_rate_table));
> +		memcpy(omap3_dsp_rate_table, omap36xx_dsp_rate_table,
> +				sizeof(omap36xx_dsp_rate_table));
> +		memcpy(omap3_l3_rate_table, omap36xx_l3_rate_table,
> +				sizeof(omap36xx_l3_rate_table));
> +	} else {
> +		/* Default to 34xx devices */
> +		omap3_mpu_rate_table = kmalloc(sizeof(omap34xx_mpu_rate_table),
> +				GFP_KERNEL);
> +		omap3_dsp_rate_table = kmalloc(sizeof(omap34xx_dsp_rate_table),
> +				GFP_KERNEL);
> +		omap3_l3_rate_table = kmalloc(sizeof(omap34xx_l3_rate_table),
> +				GFP_KERNEL);
> +
> +		BUG_ON(!omap3_mpu_rate_table || !omap3_dsp_rate_table ||
> +			!omap3_l3_rate_table);
> +
> +		memcpy(omap3_mpu_rate_table, omap34xx_mpu_rate_table,
> +				sizeof(omap34xx_mpu_rate_table));
> +		memcpy(omap3_dsp_rate_table, omap34xx_dsp_rate_table,
> +				sizeof(omap34xx_dsp_rate_table));
> +		memcpy(omap3_l3_rate_table, omap34xx_l3_rate_table,
> +				sizeof(omap34xx_l3_rate_table));
> +	}
>  }
> 
>  static int __init omap3_pm_early_init(void)
> --
> 1.6.3.3

Re: [PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Kevin Hilman]

Nishanth Menon <nm@ti.com> writes:

> Menon, Nishanth had written, on 11/15/2009 08:54 AM, the following:
> [...]
>> b) Improvements of the omap accessor functions, I will send out few of
>> the proposals I can think of later when I get some free time.. we can
>> discuss that too.
> Here is my initial proposal, do feel free to comment/add/modify etc:

Thanks for this.  Some comments on your proposal, and a proposed
header from me below.

> In the accessor functions listed here, I have followed:
> a) The function will return 0 if success else appropriate error
> values, all values are passed by pointer params.
> b) Overall, the idea is to transition from exposing omap_opp to users
> currently to a system where opp handling logic will be blackboxed from
> the rest of the system - giving us options to optimize and scale
> internal logic and storage mechanism without impacting the rest of the
> system.

completely agree here.

> c) resultant code should be readable - this is one of the issues I see
> with my previous implementation.
>
> Files: Introduce opp.c and opp.h
>
> A) Data Structures:
> Interim:
> opp.h
> struct omap_opp {
> 	bool enabled;
> 	unsigned long rate; /* in hz */
> 	u8 opp_id; /* int */
> 	u16 vsel; /* T2 voltage value */
> };
> typedef struct omap_opp omap_opp_def;
>
> Final, once resource34xx.c and all direct accesses are cleaned up:
>
> opp.h:
> struct omap_opp;
>
> /* initial definitions of OPP, will be used by all */
> struct omap_opp_def {
> 	bool enabled; /* true/false */
> 	unsigned long rate; /* in hz */
> 	u16 vsel; /* in millivolts */
> };
>
> opp.c:
> struct omap_opp {
> 	bool enabled;
> 	unsigned long rate;
> 	u16 vsel;
> };
>
> or what ever implementation it needs. can be array, list, hash
> implementation or what ever we choose it to be.

After SRF is gone, the header can simply have a 'struct omap_opp' and
the actual definition can be in opp.c

> B) Accessor functions to be used:
>
> B.1) These functions should be removed once SRF is replaced/cleaned up:
>
> int opp_id_to_freq(unsigned long *freq, const struct omap_opp *opps,
> u8 opp_id);
> int opp_freq_to_id(u8 *opp_id, const struct omap_opp *opps, unsigned
> long freq);

agreed, could flag these as  __deprecated to be clear they are going away,
or define them local to SRF.

> B.2) initialization functions:
>
> /* Register the OPP list for the silicon */
> int opp_register(struct omap_opp *opp_list,
> 		 const struct omap_opp_def *opp_reg);

ok, I was thinking opp_add(), but had the same idea.

Rather than having another omap_opp_def, could also add with just a
'u32 freq' and 'u32 voltage'.

>
> /* Enable a specific frequency */
> int opp_enable(struct omap_opp *opp_list,
> 		unsigned long freq, bool enable);

I was thinking opp_enable() would take a 'struct omap_opp' pointer
rather than a frequency, but same idea.


> Users:
> opp_register will be called by pm34xx.c based on silicon type.
> opp_enable will be called by board file/ has_feature based logic
> etc.. on a need basis
>
> B.3) verification functions:
>
> /* Check if we have a  frequency in the list(enabled/disabled)
>  * this is needed by users who would like to enable a disabled
>  * frequency
>  */
> int opp_has_freq(struct omap_opp *opp_list, unsigned long freq);
>
> /* Check if we have a enabled frequency - this is what most users
>  * will need
>  */
> int opp_is_freq_valid(struct omap_opp *opp_list, unsigned long freq);
>
> Users:
> 	files who'd want to use opp_enable,
> 	validation paths.
>
> B.4) Limit functions:
>
> /* To get a number of frequencies enabled */
> int opp_get_freq_count(const struct omap_opp *opp_list, u8 *num);

ok

> /* Get highest enabled frequency */
> int opp_get_highest_freq(struct omap_opp *opp_list,
> 	unsigned long *freq);
>
> /* Get lowest enabled frequency */
> int opp_get_lowest_freq(struct omap_opp *opp_list, unsigned long *freq);
>
> Users:
> 	Obvious user is the current cpufreq

I assume you mean the CPUfreq table init.  For that, I'd prefer to use
a generic iterator for walking the list to initialize CPUfreq.

> B.5) Search functions -> these will check only enabled
> frequencies. This will not check if the start freq provided in *freq
> is an enabled frequency or not. Assumption I am making is: if you are
> searching for a frequency that is disabled in the opp list, you are
> not trying something generic -hence wont support.
>
> /* Get higher enabled frequency than the provided one */
> int opp_get_higher_freq(struct omap_opp *opp_list, unsigned long *freq);
>
> /* Get lower enabled frequency than the provided one */
> int opp_get_lower_freq(struct omap_opp *opp_list, unsigned long *freq);

What about finding an exact match?

My proposal is to have a single 'find by freq' function with rounding
options, but whether there is one function w/options or 3 dedicated
functions, it doesn't matter to me.  

> Users:
> 	current srf, future scale logic
>
> B.6) voltage functions - opp layer will not trigger voltage
> transition, it is upto other users of opp layer to make a decision.
>
> /* get voltage corresponding to a frequency */
> int opp_freq_to_voltage(u16 *voltage, struct omap_opp *opp_list,
> 			unsigned long *freq);
>
> Users:
> 	current SR/SRF, future voltage framework.

Agree on the need for this, but had a slightly different design in
mind.  I think this should be done by something like:

  opp = opp_find_by_freq(opps, <freq>)
  voltage = opp_get_voltage(opp);

Anyways, below is the API header that I worked on last week after
looking at your initial proposal.  It's still rough around the edges,
but shows that your proposal above and mine are heading in basically
the same direction.

Kevin

/*
 * OMAP OPP interface
 *
 * Author: Kevin Hilman, Deep Root Systems, LLC
 *
 * 2009 (c) Deep Root Systems, LLC. This file is licensed under
 * the terms of the GNU General Public License version 2. This program
 * is licensed "as is" without any warranty of any kind, whether express
 * or implied.
 */

/**
 * struct omap_opp - clock frequency-to-OPP ID table for DSP, MPU, L3
 * @enabled: enabled if true, disabled if false
 * @rate: target clock rate
 * @min_vdd: minimum VDD1 voltage (in millivolts) for this OPP
 *
 * Operating performance point data.  Can vary by OMAP chip and board.
 */
struct omap_opp {
	bool enabled;
	u32 freq;
	u32 voltage;
};

#define OMAP_OPP_ROUND_NONE  0x0
#define OMAP_OPP_ROUND_UP    BIT(1)
#define OMAP_OPP_ROUND_DOWN  BIT(2)

/**
 * opp_find_by_freq()
 * @opps:        Pointer to OPP table
 * @freq:        Frequency to look for in OPP table
 * @rounding:    Rounding option: NONE, UP, DOWN
 *
 * Look for an OPP with a frequency value matching @freq.  If
 * @rounding != ROUND_NONE, find closest match using rounding.
 * 
 * Can be used to find exact OPP, or closest match using given rounding.
 *
 * Returns pointer to OPP entry if match is found, or NULL if no match
 * found (only possible when no rounding is used)
 */
struct omap_opp *opp_find_by_freq(struct omap_opp *opps,
				  u32 freq, u32 rounding);

/**
 * opp_add()
 * @opp:     pointer to list of OPPs to be added to
 * @freq:    frequency value of new OPP
 * @voltage: voltage value of new OPP
 * 
 * Add new OPP to list.  New OPP is enabled by default 
 *
 * Returns pointer to new OPP, or NULL upon failure.
 *
 * Implementation (option): list_add()
 */
struct omap_opp *opp_add(struct omap_opp *opps, u32 freq, u32 voltage);

/**
 * opp_get_freq()
 * @opp: pointer to OPP
 *
 * Returns frequency of given OPP, or -EINVAL if OPP not valid
 *
 * Implementation: return opp->freq
 */
int opp_get_freq(struct omap_opp *opp);

/**
 * opp_get_voltage()
 * @opp: pointer to OPP
 *
 * Returns voltage of given OPP, or -EINVAL if OPP not valid
 *
 * Implementation: return opp->freq
 */
int opp_get_voltage(struct omap_opp *opp);

/**
 * opp_get_opp_count()
 * @opps: Pointer to OPP table
 *
 * Returns number of (enabled) OPPs, or -EINVAL if invalid OPP list
 */
int opp_get_opp_count(struct omap_opp *opps);


/* Generalized iterator: list_for_each() */
int pwrdm_for_each(int (*fn)(struct omap_opp *opp, void *user),
			void *user);

Re: [PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Nishanth Menon]

Kevin Hilman had written, on 11/19/2009 07:31 PM, the following:
> Nishanth Menon <nm@ti.com> writes:
> 
>> Menon, Nishanth had written, on 11/15/2009 08:54 AM, the following:
>> [...]
>>> b) Improvements of the omap accessor functions, I will send out few of
>>> the proposals I can think of later when I get some free time.. we can
>>> discuss that too.
>> Here is my initial proposal, do feel free to comment/add/modify etc:
> 
> Thanks for this.  Some comments on your proposal, and a proposed
> header from me below.
Thanks for a thorough review and the new ideas. I will do a proposal 
later tomorrow to aggregate and provide a common solution I hope. some 
views follow.

> 
>> In the accessor functions listed here, I have followed:
>> a) The function will return 0 if success else appropriate error
>> values, all values are passed by pointer params.
>> b) Overall, the idea is to transition from exposing omap_opp to users
>> currently to a system where opp handling logic will be blackboxed from
>> the rest of the system - giving us options to optimize and scale
>> internal logic and storage mechanism without impacting the rest of the
>> system.
> 
> completely agree here.
> 
>> c) resultant code should be readable - this is one of the issues I see
>> with my previous implementation.
>>
>> Files: Introduce opp.c and opp.h
>>
>> A) Data Structures:
>> Interim:
>> opp.h
>> struct omap_opp {
>> 	bool enabled;
>> 	unsigned long rate; /* in hz */
>> 	u8 opp_id; /* int */
>> 	u16 vsel; /* T2 voltage value */
>> };
>> typedef struct omap_opp omap_opp_def;
>>
>> Final, once resource34xx.c and all direct accesses are cleaned up:
>>
>> opp.h:
>> struct omap_opp;
>>
>> /* initial definitions of OPP, will be used by all */
>> struct omap_opp_def {
>> 	bool enabled; /* true/false */
>> 	unsigned long rate; /* in hz */
>> 	u16 vsel; /* in millivolts */
>> };
>>
>> opp.c:
>> struct omap_opp {
>> 	bool enabled;
>> 	unsigned long rate;
>> 	u16 vsel;
>> };
>>
>> or what ever implementation it needs. can be array, list, hash
>> implementation or what ever we choose it to be.
> 
> After SRF is gone, the header can simply have a 'struct omap_opp' and
> the actual definition can be in opp.c

The the functions will need the handle for it to know which opp table is 
referenced to. so we need the prototype in header, though the definition 
has to be in .c file

> 
>> B) Accessor functions to be used:
>>
>> B.1) These functions should be removed once SRF is replaced/cleaned up:
>>
>> int opp_id_to_freq(unsigned long *freq, const struct omap_opp *opps,
>> u8 opp_id);
>> int opp_freq_to_id(u8 *opp_id, const struct omap_opp *opps, unsigned
>> long freq);
> 
> agreed, could flag these as  __deprecated to be clear they are going away,
> or define them local to SRF.
Ack, was actually wondering if I can add __must_check instead to make it 
noisy as hell and trouble all of us into fixing it eventually.

> 
>> B.2) initialization functions:
>>
>> /* Register the OPP list for the silicon */
>> int opp_register(struct omap_opp *opp_list,
>> 		 const struct omap_opp_def *opp_reg);
> 
> ok, I was thinking opp_add(), but had the same idea.
> 
> Rather than having another omap_opp_def, could also add with just a
> 'u32 freq' and 'u32 voltage'.

The idea was ability to register an initial table with certain opps 
disabled. init code will be simpler that way.

> 
>> /* Enable a specific frequency */
>> int opp_enable(struct omap_opp *opp_list,
>> 		unsigned long freq, bool enable);
> 
> I was thinking opp_enable() would take a 'struct omap_opp' pointer
> rather than a frequency, but same idea.
as I point below, yep, that might actually be a improvement in 
performance doing that. I think I can generate another rule to my 
guiding principles: "initializers, validators and search basics should 
use freq, rest should use omap_opp pointers".

> 
> 
>> Users:
>> opp_register will be called by pm34xx.c based on silicon type.
>> opp_enable will be called by board file/ has_feature based logic
>> etc.. on a need basis
>>
>> B.3) verification functions:
>>
>> /* Check if we have a  frequency in the list(enabled/disabled)
>>  * this is needed by users who would like to enable a disabled
>>  * frequency
>>  */
>> int opp_has_freq(struct omap_opp *opp_list, unsigned long freq);
>>
>> /* Check if we have a enabled frequency - this is what most users
>>  * will need
>>  */
>> int opp_is_freq_valid(struct omap_opp *opp_list, unsigned long freq);
>>
>> Users:
>> 	files who'd want to use opp_enable,
>> 	validation paths.
>>
>> B.4) Limit functions:
>>
>> /* To get a number of frequencies enabled */
>> int opp_get_freq_count(const struct omap_opp *opp_list, u8 *num);
> 
> ok
> 
>> /* Get highest enabled frequency */
>> int opp_get_highest_freq(struct omap_opp *opp_list,
>> 	unsigned long *freq);
>>
>> /* Get lowest enabled frequency */
>> int opp_get_lowest_freq(struct omap_opp *opp_list, unsigned long *freq);
>>
>> Users:
>> 	Obvious user is the current cpufreq
> 
> I assume you mean the CPUfreq table init.  For that, I'd prefer to use
> a generic iterator for walking the list to initialize CPUfreq.

you will still need a start point as cpufreq does not even know which 
processor it is running on and whether some freq is disabled or not.

> 
>> B.5) Search functions -> these will check only enabled
>> frequencies. This will not check if the start freq provided in *freq
>> is an enabled frequency or not. Assumption I am making is: if you are
>> searching for a frequency that is disabled in the opp list, you are
>> not trying something generic -hence wont support.
>>
>> /* Get higher enabled frequency than the provided one */
>> int opp_get_higher_freq(struct omap_opp *opp_list, unsigned long *freq);
>>
>> /* Get lower enabled frequency than the provided one */
>> int opp_get_lower_freq(struct omap_opp *opp_list, unsigned long *freq);
> 
> What about finding an exact match?
> 
> My proposal is to have a single 'find by freq' function with rounding
> options, but whether there is one function w/options or 3 dedicated
> functions, it doesn't matter to me.  

opp_is_freq_valid already does the exact match, so a generic function is 
redundant and makes code a mite difficult to read + allowing scope for 
misuse in terms of extending the definitions. we can do it if there is a 
need for the same in the future.

> 
>> Users:
>> 	current srf, future scale logic
>>
>> B.6) voltage functions - opp layer will not trigger voltage
>> transition, it is upto other users of opp layer to make a decision.
>>
>> /* get voltage corresponding to a frequency */
>> int opp_freq_to_voltage(u16 *voltage, struct omap_opp *opp_list,
>> 			unsigned long *freq);
>>
>> Users:
>> 	current SR/SRF, future voltage framework.
> 
> Agree on the need for this, but had a slightly different design in
> mind.  I think this should be done by something like:
> 
>   opp = opp_find_by_freq(opps, <freq>)
>   voltage = opp_get_voltage(opp);
yes, this is one way to do it, this assumes that opp is the handle to 
specific opp -> might actually be a better approach than mine in terms 
of sticking with the rule that freq is only used, causing a research 
overheads every time.

I dont however like the idea of function returning value -> if the opp 
functions were always to return error/success, it is easier to review 
and maintain further developments on it.

> 
> Anyways, below is the API header that I worked on last week after
> looking at your initial proposal.  It's still rough around the edges,
> but shows that your proposal above and mine are heading in basically
> the same direction.
> 
> Kevin
> 
> /*
>  * OMAP OPP interface
>  *
>  * Author: Kevin Hilman, Deep Root Systems, LLC
>  *
>  * 2009 (c) Deep Root Systems, LLC. This file is licensed under
>  * the terms of the GNU General Public License version 2. This program
>  * is licensed "as is" without any warranty of any kind, whether express
>  * or implied.
>  */
> 
> /**
>  * struct omap_opp - clock frequency-to-OPP ID table for DSP, MPU, L3
>  * @enabled: enabled if true, disabled if false
>  * @rate: target clock rate
>  * @min_vdd: minimum VDD1 voltage (in millivolts) for this OPP
>  *
>  * Operating performance point data.  Can vary by OMAP chip and board.
>  */
> struct omap_opp {
> 	bool enabled;
> 	u32 freq;
> 	u32 voltage;
> };
> 
> #define OMAP_OPP_ROUND_NONE  0x0
> #define OMAP_OPP_ROUND_UP    BIT(1)
> #define OMAP_OPP_ROUND_DOWN  BIT(2)
> 
> /**
>  * opp_find_by_freq()
>  * @opps:        Pointer to OPP table
>  * @freq:        Frequency to look for in OPP table
>  * @rounding:    Rounding option: NONE, UP, DOWN
>  *
>  * Look for an OPP with a frequency value matching @freq.  If
>  * @rounding != ROUND_NONE, find closest match using rounding.
>  * 
>  * Can be used to find exact OPP, or closest match using given rounding.
>  *
>  * Returns pointer to OPP entry if match is found, or NULL if no match
>  * found (only possible when no rounding is used)
>  */
> struct omap_opp *opp_find_by_freq(struct omap_opp *opps,
> 				  u32 freq, u32 rounding);
> 
> /**
>  * opp_add()
>  * @opp:     pointer to list of OPPs to be added to
>  * @freq:    frequency value of new OPP
>  * @voltage: voltage value of new OPP
>  * 
>  * Add new OPP to list.  New OPP is enabled by default 
>  *
>  * Returns pointer to new OPP, or NULL upon failure.
>  *
>  * Implementation (option): list_add()
>  */
> struct omap_opp *opp_add(struct omap_opp *opps, u32 freq, u32 voltage);
> 
> /**
>  * opp_get_freq()
>  * @opp: pointer to OPP
>  *
>  * Returns frequency of given OPP, or -EINVAL if OPP not valid
>  *
>  * Implementation: return opp->freq
>  */
> int opp_get_freq(struct omap_opp *opp);
> 
> /**
>  * opp_get_voltage()
>  * @opp: pointer to OPP
>  *
>  * Returns voltage of given OPP, or -EINVAL if OPP not valid
>  *
>  * Implementation: return opp->freq
>  */
> int opp_get_voltage(struct omap_opp *opp);
> 
> /**
>  * opp_get_opp_count()
>  * @opps: Pointer to OPP table
>  *
>  * Returns number of (enabled) OPPs, or -EINVAL if invalid OPP list
>  */
> int opp_get_opp_count(struct omap_opp *opps);
> 
> 
> /* Generalized iterator: list_for_each() */
> int pwrdm_for_each(int (*fn)(struct omap_opp *opp, void *user),
> 			void *user);
> 
> 
> 
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Nishanth Menon]

Menon, Nishanth had written, on 11/19/2009 08:16 PM, the following:
> Kevin Hilman had written, on 11/19/2009 07:31 PM, the following:
>> Nishanth Menon <nm@ti.com> writes:
>>
>>> Menon, Nishanth had written, on 11/15/2009 08:54 AM, the following:

[...]

> Thanks for a thorough review and the new ideas. I will do a proposal
> later tomorrow to aggregate and provide a common solution I hope. some
> views follow.
Here it is inlined:
/*
  * OMAP OPP Interface
  *
  * Copyright (C) 2009 Texas Instruments Incorporated.
  *	Nishanth Menon
  * Copyright (C) 2009 Deep Root Systems, LLC.
  *	Kevin Hilman
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
#ifndef __ASM_ARM_OMAP_OPP_H
#ifndef __ASM_ARM_OMAP_OPP_H

/**
  * struct omap_opp - OMAP OPP description structure
  * @enabled:	true/false - marking this OPP as enabled/disabled
  * @rate:	Frequency in hertz
  * @opp_id:	(DEPRECATED) opp identifier
  * @vsel:	Voltage in volt processor level(this usage is
  *		DEPRECATED to use Voltage in millivolts in future)
  *		mV= (vsel * 12.5) + 600
  *
  * This structure stores the OPP information for a given domain.
  * Due to legacy reasons, this structure is currently exposed and
  * will soon be removed elsewhere and will only be used as a handle
  * from the OPP internal referencing mechanism
  */
struct omap_opp {
	bool enabled;
	unsigned long rate;
	u8 opp_id;
	u16 vsel;
};

/**
  * opp_get_voltage - Gets the voltage corresponding to an opp
  * @m_volt:	Voltage in millivolts corresponding to an opp
  * @opp:	opp for which voltage has to be returned for
  *
  * Return 0 and the voltage in milli volt corresponding to the opp,
  * else return the corresponding error value.
  */
int opp_get_voltage(u16 *m_volt, const struct omap_opp *opp);

/**
  * opp_get_freq - Gets the frequency corresponding to an opp
  * @freq:	Frequency in hertz corresponding to an opp
  * @opp:	opp for which frequency has to be returned for
  *
  * Return 0 and the frequency in hertz corresponding to the opp,
  * else return the corresponding error value.
  */
int opp_get_freq(unsigned long *freq, const struct omap_opp *opp);

/**
  * opp_is_valid - Verifies if a given frequency is enabled in the opp list
  * @opp:	Pointer to opp returned if opp match is achieved
  * @oppl:	opp list
  * @freq:	Frequency in hertz to check for
  *
  * Searches the OPP list to find if the provided frequency is an enabled
  * frequency. If a match is achieved, it returns 0 and the pointer to 
the opp
  * is returned, else a corresponding error value is returned.
  */
int opp_is_valid(struct omap_opp **opp, const struct omap_opp *oppl,
		const unsigned long freq);

/**
  * opp_has_freq - Checks if a frequency is exists(enabled/disabled) in 
opp list
  * @opp:	Pointer to opp returned if opp match is achieved
  * @oppl:	opp list
  * @freq:	Frequency in hertz to check for
  *
  * Searches the OPP list to find a frequency. This is a more generic 
function
  * than the opp_is_valid since it searches for both enabled/disabled
  * frequencies.
  *
  * This function may be used by detection logic to enable a disabled OPP as
  * all other search functions work on enabled OPPs only.
  */
int opp_has_freq(struct omap_opp **opp, const struct omap_opp *oppl,
		const unsigned long freq);

/**
  * opp_get_opp_count - Get number of opps enabled in the opp list
  * @num:	returns the number of opps
  * @oppl:	opp list
  *
  * This functions returns 0 and the number of opps are updated in num if
  * success, else returns corresponding error value.
  */
int opp_get_opp_count(u8 *num, const struct omap_opp *oppl);

/**
  * opp_get_higher_opp - search for the next highest opp in the list
  * @opp:	pointer to the opp
  * @freq:	frequency to start the search on
  * @oppl:	opp list to search on
  *
  * Searches for the higher *enabled* OPP than a starting freq/opp
  * Decision of start condition:
  *	if *opp is NULL, *freq is checked (usually the start condition)
  *	if *opp is populated, *freq is ignored
  * Returns 0 and *opp and *freq is populated with the next highest match,
  * else returns corresponding error value.
  *
  * Example usage:
  *	* print a all frequencies ascending order *
  *	unsigned long freq = 0;
  *	struct omap_opp *opp = NULL;
  *	while(!opp_get_higher_opp(&opp, &freq, oppl))
  *		pr_info("%ld ", freq);
  * NOTE: if we set freq as 0, we get the lowest enabled frequency
  */
int opp_get_higher_opp(struct omap_opp **opp, unsigned long *freq,
			const struct omap_opp *oppl);

/**
  * opp_get_lower_opp - search for the next lower opp in the list
  * @opp:	pointer to the opp
  * @freq:	frequency to start the search on
  * @oppl:	opp list to search on
  *
  * Search for the lower *enabled* OPP than a starting freq/opp
  * Decision of start condition:
  *	if *opp is NULL, *freq is checked (usually the start condition)
  *	if *opp is populated, *freq is ignored
  * Returns 0 and *opp and *freq is populated with the next lowest match,
  * else returns corresponding error value.
  *
  * Example usage:
  *	* print a all frequencies in descending order *
  *	unsigned long freq = ULONG_MAX;
  *	struct omap_opp *opp = NULL;
  *	while(!opp_get_lower_opp(&opp, &freq, oppl))
  *		pr_info("%ld ", freq);
  * NOTE: if we set freq as ULONG_MAX, we get the highest enabled frequency
  */
int opp_get_lower_opp(struct omap_opp **opp, unsigned long *freq,
			const struct omap_opp *oppl);

/**
  * struct omap_opp_def - OMAP OPP Definition
  * @enabled:	True/false - is this OPP enabled/disabled by default
  * @freq:	Frequency in hertz corresponding to this OPP
  * @m_volt:	Nominal voltage in millivolts corresponding to this OPP
  *
  * OMAP SOCs from OMAP3 onwards have a standard set of tuples consisting
  * of frequency and voltage pairs that the device will support. This is
  * Operating Point. However, the actual definitions of OMAP Operating Point
  * varies over silicon within the same family of devices. For a specific
  * domain, you can have a set of {frequency, voltage} pairs and this is 
denoted
  * by an array of omap_opp_def. As the kernel boots and more information is
  * available, a set of these are activated based on the precise nature of
  * device the kernel boots up on.
  *
  * NOTE: A disabled opp from the omap_opp_def table may be enabled as 
part of
  * runtime logic. It is discouraged to enable/disable the OPP unless 
they are
  * done as part of OPP registration sequence.
  */
struct omap_opp_def {
	bool enabled;
	unsigned long freq;
	u16 m_volt;
};

/**
  * opp_init - Initialize an OPP table from the initial table definition
  * @oppl:	Returned back to caller as the opp list to reference the OPP
  * @opp_defs:	Array of omap_opp_def to describe the OPP. This list 
should be
  *		0 terminated.
  *
  * This function creates the internal datastructure representing the 
OPP list
  * from an initial definition table. this handle is then used for further
  * validation, search, modification operations on the OPP list.
  *
  * This function returns 0 and the pointer to the allocated list 
through oppl if
  * success, else corresponding error value. Caller should NOT free the 
oppl.
  * opps_defs can be freed after use.
  */
int opp_init(struct omap_opp **oppl, const struct omap_opp_def *opp_defs);

/**
  * opp_enable - Enable or disable a specific OPP
  * @opp:	pointer to opp
  * @freq:	frequency in hertz
  * @enable:	true/false to enable/disable that specific OPP
  *
  * Enables/disables a provided freq in the opp list. If the operation 
is valid,
  * this returns 0, else the corresponding error value.
  *
  * OPP used here is from the the opp_is_valid/opp_has_freq or other search
  * functions
  */
int opp_enable(const struct omap_opp *opp, const unsigned int freq);

#endif		/* __ASM_ARM_OMAP_OPP_H */

-- 
Regards,
Nishanth Menon

RE: [PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Cousson, Benoit]

Hi Nishanth,

> From: Menon, Nishanth
> Sent: Saturday, November 21, 2009 4:01 AM

[...]

> /**
>   * struct omap_opp_def - OMAP OPP Definition
>   * @enabled: True/false - is this OPP enabled/disabled by default
>   * @freq:    Frequency in hertz corresponding to this OPP
>   * @m_volt:  Nominal voltage in millivolts corresponding to this OPP
>   *
>   * OMAP SOCs from OMAP3 onwards have a standard set of tuples consisting
>   * of frequency and voltage pairs that the device will support. This is
>   * Operating Point. However, the actual definitions of OMAP Operating
> Point
>   * varies over silicon within the same family of devices. For a specific
>   * domain, you can have a set of {frequency, voltage} pairs and this is
> denoted
>   * by an array of omap_opp_def. As the kernel boots and more information

The OPP term is not necessarily something new to OMAP3; OMAP2420/2430 already supported 2 OPPs (100% and 50%) and to some extend OMAP1610/1710 were also able to run at a lowest OPP (50%).

To be more accurate, you should explain that the OPP is relative to a voltage domain and that all IPs in that voltage domain will have potentially their own OPPs depending of the voltage.

> is
>   * available, a set of these are activated based on the precise nature of
>   * device the kernel boots up on.
>   *
>   * NOTE: A disabled opp from the omap_opp_def table may be enabled as
> part of

Are you sure you want to allow that? What if you re-enable none supported OPPs like a speed-binning OPP on a regular device?

>   * runtime logic. It is discouraged to enable/disable the OPP unless
> they are
>   * done as part of OPP registration sequence.

Why is it discouraged? We should have as well the capability to enable/disable dynamically and temporarily an OPP (thermal management, frequency avoidance, select different OPP sets...).

You should differentiate OPPs disabled at boot time because they are not supported by the platform and the ones disabled at runtime. It will avoid that you potentially re-enable a forbidden OPP.

Why don't you remove them during the opp_init phase?

>   */
> struct omap_opp_def {
>       bool enabled;
>       unsigned long freq;
>       u16 m_volt;

It might be better to use an u32 and store the voltage in microvolt considering that the T2 and Phoenix have a 12.5 mV accuracy step. Moreover, this is what the regulator framework is currently using, it will avoid the *1000 operation, which is not that a big deal, I agree...

> };
>
> /**
>   * opp_init - Initialize an OPP table from the initial table definition
>   * @oppl:    Returned back to caller as the opp list to reference the OPP
>   * @opp_defs:        Array of omap_opp_def to describe the OPP. This list
> should be
>   *           0 terminated.
>   *
>   * This function creates the internal datastructure representing the
> OPP list
>   * from an initial definition table. this handle is then used for further
>   * validation, search, modification operations on the OPP list.
>   *
>   * This function returns 0 and the pointer to the allocated list
> through oppl if
>   * success, else corresponding error value. Caller should NOT free the
> oppl.
>   * opps_defs can be freed after use.
>   */
> int opp_init(struct omap_opp **oppl, const struct omap_opp_def *opp_defs);
>
> /**
>   * opp_enable - Enable or disable a specific OPP

You'd better provide two APIs opp_enable and opp_disable or find a better name for that one, like opp_state or opp_set_state.

>   * @opp:     pointer to opp
>   * @freq:    frequency in hertz
>   * @enable:  true/false to enable/disable that specific OPP
>   *
>   * Enables/disables a provided freq in the opp list. If the operation
> is valid,
>   * this returns 0, else the corresponding error value.
>   *
>   * OPP used here is from the the opp_is_valid/opp_has_freq or other
> search
>   * functions
>   */
> int opp_enable(const struct omap_opp *opp, const unsigned int freq);
>
> #endif                /* __ASM_ARM_OMAP_OPP_H */
>

Regards,
Benoit

Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Menon, Nishanth]

Hi Benoit,

Thanks for your detailed review comments. few views follows

Cousson, Benoit said the following on 11/21/2009 10:07 AM:
[...]
>> /**
>>   * struct omap_opp_def - OMAP OPP Definition
>>   * @enabled: True/false - is this OPP enabled/disabled by default
>>   * @freq:    Frequency in hertz corresponding to this OPP
>>   * @m_volt:  Nominal voltage in millivolts corresponding to this OPP
>>   *
>>   * OMAP SOCs from OMAP3 onwards have a standard set of tuples consisting
>>   * of frequency and voltage pairs that the device will support. This is
>>   * Operating Point. However, the actual definitions of OMAP Operating
>> Point
>>   * varies over silicon within the same family of devices. For a specific
>>   * domain, you can have a set of {frequency, voltage} pairs and this is
>> denoted
>>   * by an array of omap_opp_def. As the kernel boots and more information
>>     
>
> The OPP term is not necessarily something new to OMAP3; OMAP2420/2430 already supported 2 OPPs (100% and 50%) and to some extend OMAP1610/1710 were also able to run at a lowest OPP (50%).
>
> To be more accurate, you should explain that the OPP is relative to a voltage domain and that all IPs in that voltage domain will have potentially their own OPPs depending of the voltage.
>
>   
Thanks for explaining the history. yep, will fix up.
>> is
>>   * available, a set of these are activated based on the precise nature of
>>   * device the kernel boots up on.
>>   *
>>   * NOTE: A disabled opp from the omap_opp_def table may be enabled as
>> part of
>>     
>
> Are you sure you want to allow that? What if you re-enable none supported OPPs like a speed-binning OPP on a regular device?
>   
Yes, we want to allow this. here is why: we provide a list of ALL opps
supported on that family of devices of which the common opp entries
(across the specific family of SOCs for which the table are enabled).
yes, there are multiple ways of doing this, one of which is to have
enable/disable opp OR to use a add function etc.. we can scale this
function as needed in the future as our experience with more dynamic
silicon decisions become solid. here are specific examples:
cpu_is_3630() will probably register all OPPs for 3630 including speed
binned one which will be by default disabled, on detecting the
speedbinned variants, OPP will be specifically enabled.

Now opp.[ch] is meant to be core functions with control of some of the
functions from elsewhere, in kernel space with un exported functions, I
do not expect to write "dumb proof code", instead I would expect to
write smart optimized code and allow other smart people's smart code use
it optimally ;).. if we screw up here (e.g. enable a wrong OPP), we need
code fixing and with this framwork it would now be easy to track and fix
as OPP access are centralized.


>   
>>   * runtime logic. It is discouraged to enable/disable the OPP unless
>> they are
>>   * done as part of OPP registration sequence.
>>     
>
> Why is it discouraged? We should have as well the capability to enable/disable dynamically and temporarily an OPP (thermal management, frequency avoidance, select different OPP sets...).
>
> You should differentiate OPPs disabled at boot time because they are not supported by the platform and the ones disabled at runtime. It will avoid that you potentially re-enable a forbidden OPP.
>
> Why don't you remove them during the opp_init phase?
>   
Discouraged IMHO you may impact rest of the today's system such as
cpufreq heuristics etc.. But, I agree with you, this is a current
limitation which is not what I should reflect in the comment - will
remove that statement.  btw, it is smarter for the higher layers to make
dynamic decisions of temp frequency avoidance without having to resort
to enable/disable OPP IMHO.
>   
>>   */
>> struct omap_opp_def {
>>       bool enabled;
>>       unsigned long freq;
>>       u16 m_volt;
>>     
>
> It might be better to use an u32 and store the voltage in microvolt considering that the T2 and Phoenix have a 12.5 mV accuracy step. Moreover, this is what the regulator framework is currently using, it will avoid the *1000 operation, which is not that a big deal, I agree...
>   
Thanks for pointing this out.. yep uV makes more sense. will switch.

>   
>> };
>>
>> /**
>>   * opp_init - Initialize an OPP table from the initial table definition
>>   * @oppl:    Returned back to caller as the opp list to reference the OPP
>>   * @opp_defs:        Array of omap_opp_def to describe the OPP. This list
>> should be
>>   *           0 terminated.
>>   *
>>   * This function creates the internal datastructure representing the
>> OPP list
>>   * from an initial definition table. this handle is then used for further
>>   * validation, search, modification operations on the OPP list.
>>   *
>>   * This function returns 0 and the pointer to the allocated list
>> through oppl if
>>   * success, else corresponding error value. Caller should NOT free the
>> oppl.
>>   * opps_defs can be freed after use.
>>   */
>> int opp_init(struct omap_opp **oppl, const struct omap_opp_def *opp_defs);
>>
>> /**
>>   * opp_enable - Enable or disable a specific OPP
>>     
>
> You'd better provide two APIs opp_enable and opp_disable or find a better name for that one, like opp_state or opp_set_state.
>   
the kernel has other xyz_enable function which does disable also. yes,
this had been debated internally, and  Kevin pointed this out.

example:
include/linux/pci.h:int pci_enable_wake(struct pci_dev *dev, pci_power_t
state, bool enable);
others: grep -R ".*enable.*(" include/|grep -v define

If someone has strong opinions on this and find this function extremely
obscure, I don't mind creating wrappers around an internal __opp_onoff
and provide and opp_enable and opp_disable. it is just a one liner anyways..
>   
>>   * @opp:     pointer to opp
>>   * @freq:    frequency in hertz
>>   * @enable:  true/false to enable/disable that specific OPP
>>   *
>>   * Enables/disables a provided freq in the opp list. If the operation
>> is valid,
>>   * this returns 0, else the corresponding error value.
>>   *
>>   * OPP used here is from the the opp_is_valid/opp_has_freq or other
>> search
>>   * functions
>>   */
>> int opp_enable(const struct omap_opp *opp, const unsigned int freq);
>>
>> #endif                /* __ASM_ARM_OMAP_OPP_H */
>>
>>     
Regards,
Nishanth Menon

RE: [PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Cousson, Benoit]

>From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
>
>Hi Benoit,
>
>Thanks for your detailed review comments. few views follows
>
>Cousson, Benoit said the following on 11/21/2009 10:07 AM:
>[...]
>>> /**
>>>   * struct omap_opp_def - OMAP OPP Definition
>>>   * @enabled: True/false - is this OPP enabled/disabled by default
>>>   * @freq:    Frequency in hertz corresponding to this OPP
>>>   * @m_volt:  Nominal voltage in millivolts corresponding to this OPP
>>>   *
>>>   * OMAP SOCs from OMAP3 onwards have a standard set of tuples
>consisting
>>>   * of frequency and voltage pairs that the device will support. This is
>>>   * Operating Point. However, the actual definitions of OMAP Operating
>>> Point
>>>   * varies over silicon within the same family of devices. For a
>specific
>>>   * domain, you can have a set of {frequency, voltage} pairs and this is
>>> denoted
>>>   * by an array of omap_opp_def. As the kernel boots and more
>information
>>>
>>
>> The OPP term is not necessarily something new to OMAP3; OMAP2420/2430
>already supported 2 OPPs (100% and 50%) and to some extend OMAP1610/1710
>were also able to run at a lowest OPP (50%).
>>
>> To be more accurate, you should explain that the OPP is relative to a
>voltage domain and that all IPs in that voltage domain will have
>potentially their own OPPs depending of the voltage.
>>
>>
>Thanks for explaining the history. yep, will fix up.
>>> is
>>>   * available, a set of these are activated based on the precise nature
>of
>>>   * device the kernel boots up on.
>>>   *
>>>   * NOTE: A disabled opp from the omap_opp_def table may be enabled as
>>> part of
>>>
>>
>> Are you sure you want to allow that? What if you re-enable none supported
>OPPs like a speed-binning OPP on a regular device?
>>
>Yes, we want to allow this. here is why: we provide a list of ALL opps
>supported on that family of devices of which the common opp entries
>(across the specific family of SOCs for which the table are enabled).
>yes, there are multiple ways of doing this, one of which is to have
>enable/disable opp OR to use a add function etc.. we can scale this
>function as needed in the future as our experience with more dynamic
>silicon decisions become solid. here are specific examples:
>cpu_is_3630() will probably register all OPPs for 3630 including speed
>binned one which will be by default disabled, on detecting the
>speedbinned variants, OPP will be specifically enabled.

OK, I still had in mind the proposal to copy the relevant OPP from a const
table in initdata to the actual omap_opp_def table. In that case only the
valid OPPs will be in the table. It will prevent anybody to reactivate a speed-binned OPP.
BTW, since you build the omap_opp_def at boot time, why do you want to keep
all possible OPPs for the SoC family an not keep only the relevant ones for
the specific SoC?


>Now opp.[ch] is meant to be core functions with control of some of the
>functions from elsewhere, in kernel space with un exported functions, I
>do not expect to write "dumb proof code", instead I would expect to
>write smart optimized code and allow other smart people's smart code use
>it optimally ;).. if we screw up here (e.g. enable a wrong OPP), we need
>code fixing and with this framwork it would now be easy to track and fix
>as OPP access are centralized.
>
>
>>
>>>   * runtime logic. It is discouraged to enable/disable the OPP unless
>>> they are
>>>   * done as part of OPP registration sequence.
>>>
>>
>> Why is it discouraged? We should have as well the capability to
>enable/disable dynamically and temporarily an OPP (thermal management,
>frequency avoidance, select different OPP sets...).
>>
>> You should differentiate OPPs disabled at boot time because they are not
>supported by the platform and the ones disabled at runtime. It will avoid
>that you potentially re-enable a forbidden OPP.
>>
>> Why don't you remove them during the opp_init phase?
>>
>Discouraged IMHO you may impact rest of the today's system such as
>cpufreq heuristics etc.. But, I agree with you, this is a current
>limitation which is not what I should reflect in the comment - will
>remove that statement.  btw, it is smarter for the higher layers to make
>dynamic decisions of temp frequency avoidance without having to resort
>to enable/disable OPP IMHO.

I do agree, but in that case the opp_enable API should be limited to the opp_init phase and thus not expose at all.
You should then clarify the scope of that API and explain what must be done
by the CPUFreq layer.


>>>   */
>>> struct omap_opp_def {
>>>       bool enabled;
>>>       unsigned long freq;
>>>       u16 m_volt;
>>>
>>
>> It might be better to use an u32 and store the voltage in microvolt
>considering that the T2 and Phoenix have a 12.5 mV accuracy step. Moreover,
>this is what the regulator framework is currently using, it will avoid the
>*1000 operation, which is not that a big deal, I agree...
>>
>Thanks for pointing this out.. yep uV makes more sense. will switch.
>
>>
>>> };
>>>
>>> /**
>>>   * opp_init - Initialize an OPP table from the initial table definition
>>>   * @oppl:    Returned back to caller as the opp list to reference the
>OPP
>>>   * @opp_defs:        Array of omap_opp_def to describe the OPP. This
>list
>>> should be
>>>   *           0 terminated.
>>>   *
>>>   * This function creates the internal datastructure representing the
>>> OPP list
>>>   * from an initial definition table. this handle is then used for
>further
>>>   * validation, search, modification operations on the OPP list.
>>>   *
>>>   * This function returns 0 and the pointer to the allocated list
>>> through oppl if
>>>   * success, else corresponding error value. Caller should NOT free the
>>> oppl.
>>>   * opps_defs can be freed after use.
>>>   */
>>> int opp_init(struct omap_opp **oppl, const struct omap_opp_def
>*opp_defs);
>>>
>>> /**
>>>   * opp_enable - Enable or disable a specific OPP
>>>
>>
>> You'd better provide two APIs opp_enable and opp_disable or find a better
>name for that one, like opp_state or opp_set_state.
>>
>the kernel has other xyz_enable function which does disable also. yes,
>this had been debated internally, and  Kevin pointed this out.
>
>example:
>include/linux/pci.h:int pci_enable_wake(struct pci_dev *dev, pci_power_t
>state, bool enable);
>others: grep -R ".*enable.*(" include/|grep -v define

Nobody is perfect... even the current Linux code :-)


>If someone has strong opinions on this and find this function extremely
>obscure, I don't mind creating wrappers around an internal __opp_onoff
>and provide and opp_enable and opp_disable. it is just a one liner anyways..

If it is not a big deal to fix that, then it might be good to do it. It will
avoid further debate.

Regards,
Benoit


Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920

Re: [PATCH 2/9 v2] omap3: pm: introduce opp accessor functions

[Menon, Nishanth]

Cousson, Benoit said the following on 11/21/2009 04:22 PM:
>> From: Nishanth Menon [mailto:menon.nishanth@gmail.com]
>>
>> Hi Benoit,
>>
>> Thanks for your detailed review comments. few views follows
>>
>> Cousson, Benoit said the following on 11/21/2009 10:07 AM:
>> [...]
>>     
>>>> /**
>>>>   * struct omap_opp_def - OMAP OPP Definition
>>>>   * @enabled: True/false - is this OPP enabled/disabled by default
>>>>   * @freq:    Frequency in hertz corresponding to this OPP
>>>>   * @m_volt:  Nominal voltage in millivolts corresponding to this OPP
>>>>   *
>>>>   * OMAP SOCs from OMAP3 onwards have a standard set of tuples
>>>>         
>> consisting
>>     
>>>>   * of frequency and voltage pairs that the device will support. This is
>>>>   * Operating Point. However, the actual definitions of OMAP Operating
>>>> Point
>>>>   * varies over silicon within the same family of devices. For a
>>>>         
>> specific
>>     
>>>>   * domain, you can have a set of {frequency, voltage} pairs and this is
>>>> denoted
>>>>   * by an array of omap_opp_def. As the kernel boots and more
>>>>         
>> information
>>     
>>> The OPP term is not necessarily something new to OMAP3; OMAP2420/2430
>>>       
>> already supported 2 OPPs (100% and 50%) and to some extend OMAP1610/1710
>> were also able to run at a lowest OPP (50%).
>>     
>>> To be more accurate, you should explain that the OPP is relative to a
>>>       
>> voltage domain and that all IPs in that voltage domain will have
>> potentially their own OPPs depending of the voltage.
>>     
>>>       
>> Thanks for explaining the history. yep, will fix up.
>>     
>>>> is
>>>>   * available, a set of these are activated based on the precise nature
>>>>         
>> of
>>     
>>>>   * device the kernel boots up on.
>>>>   *
>>>>   * NOTE: A disabled opp from the omap_opp_def table may be enabled as
>>>> part of
>>>>
>>>>         
>>> Are you sure you want to allow that? What if you re-enable none supported
>>>       
>> OPPs like a speed-binning OPP on a regular device?
>>     
>> Yes, we want to allow this. here is why: we provide a list of ALL opps
>> supported on that family of devices of which the common opp entries
>> (across the specific family of SOCs for which the table are enabled).
>> yes, there are multiple ways of doing this, one of which is to have
>> enable/disable opp OR to use a add function etc.. we can scale this
>> function as needed in the future as our experience with more dynamic
>> silicon decisions become solid. here are specific examples:
>> cpu_is_3630() will probably register all OPPs for 3630 including speed
>> binned one which will be by default disabled, on detecting the
>> speedbinned variants, OPP will be specifically enabled.
>>     
>
> OK, I still had in mind the proposal to copy the relevant OPP from a const
> table in initdata to the actual omap_opp_def table. In that case only the
> valid OPPs will be in the table. It will prevent anybody to reactivate a speed-binned OPP.
> BTW, since you build the omap_opp_def at boot time, why do you want to keep
> all possible OPPs for the SoC family an not keep only the relevant ones for
> the specific SoC?
>
>
>   
>> Now opp.[ch] is meant to be core functions with control of some of the
>> functions from elsewhere, in kernel space with un exported functions, I
>> do not expect to write "dumb proof code", instead I would expect to
>> write smart optimized code and allow other smart people's smart code use
>> it optimally ;).. if we screw up here (e.g. enable a wrong OPP), we need
>> code fixing and with this framwork it would now be easy to track and fix
>> as OPP access are centralized.
>>
>>
>>     
>>>>   * runtime logic. It is discouraged to enable/disable the OPP unless
>>>> they are
>>>>   * done as part of OPP registration sequence.
>>>>
>>>>         
>>> Why is it discouraged? We should have as well the capability to
>>>       
>> enable/disable dynamically and temporarily an OPP (thermal management,
>> frequency avoidance, select different OPP sets...).
>>     
>>> You should differentiate OPPs disabled at boot time because they are not
>>>       
>> supported by the platform and the ones disabled at runtime. It will avoid
>> that you potentially re-enable a forbidden OPP.
>>     
>>> Why don't you remove them during the opp_init phase?
>>>
>>>       
>> Discouraged IMHO you may impact rest of the today's system such as
>> cpufreq heuristics etc.. But, I agree with you, this is a current
>> limitation which is not what I should reflect in the comment - will
>> remove that statement.  btw, it is smarter for the higher layers to make
>> dynamic decisions of temp frequency avoidance without having to resort
>> to enable/disable OPP IMHO.
>>     
>
> I do agree, but in that case the opp_enable API should be limited to the opp_init phase and thus not expose at all.
> You should then clarify the scope of that API and explain what must be done
> by the CPUFreq layer.
>   
I am not intending to describe the power management architecture here.
if you have a proposal of how it should be stated, please do share, I
dont think I get why you need this api to describe usage
in-yet-to-be-used-in-cpufreq? esp considering I have intentions of
describing OPP layer not, how rest of the blocks should use these APIs.
the actual implementation is upto the OPP.c.

I think a description from you might be easier than a bunch of back and
froth on emails.
>
>   
>>>>   */
>>>> struct omap_opp_def {
>>>>       bool enabled;
>>>>       unsigned long freq;
>>>>       u16 m_volt;
>>>>
>>>>         
>>> It might be better to use an u32 and store the voltage in microvolt
>>>       
>> considering that the T2 and Phoenix have a 12.5 mV accuracy step. Moreover,
>> this is what the regulator framework is currently using, it will avoid the
>> *1000 operation, which is not that a big deal, I agree...
>>     
>> Thanks for pointing this out.. yep uV makes more sense. will switch.
>>
>>     
>>>> };
>>>>
>>>> /**
>>>>   * opp_init - Initialize an OPP table from the initial table definition
>>>>   * @oppl:    Returned back to caller as the opp list to reference the
>>>>         
>> OPP
>>     
>>>>   * @opp_defs:        Array of omap_opp_def to describe the OPP. This
>>>>         
>> list
>>     
>>>> should be
>>>>   *           0 terminated.
>>>>   *
>>>>   * This function creates the internal datastructure representing the
>>>> OPP list
>>>>   * from an initial definition table. this handle is then used for
>>>>         
>> further
>>     
>>>>   * validation, search, modification operations on the OPP list.
>>>>   *
>>>>   * This function returns 0 and the pointer to the allocated list
>>>> through oppl if
>>>>   * success, else corresponding error value. Caller should NOT free the
>>>> oppl.
>>>>   * opps_defs can be freed after use.
>>>>   */
>>>> int opp_init(struct omap_opp **oppl, const struct omap_opp_def
>>>>         
>> *opp_defs);
>>     
>>>> /**
>>>>   * opp_enable - Enable or disable a specific OPP
>>>>
>>>>         
>>> You'd better provide two APIs opp_enable and opp_disable or find a better
>>>       
>> name for that one, like opp_state or opp_set_state.
>>     
>> the kernel has other xyz_enable function which does disable also. yes,
>> this had been debated internally, and  Kevin pointed this out.
>>
>> example:
>> include/linux/pci.h:int pci_enable_wake(struct pci_dev *dev, pci_power_t
>> state, bool enable);
>> others: grep -R ".*enable.*(" include/|grep -v define
>>     
>
> Nobody is perfect... even the current Linux code :-)
>
>
>   
>> If someone has strong opinions on this and find this function extremely
>> obscure, I don't mind creating wrappers around an internal __opp_onoff
>> and provide and opp_enable and opp_disable. it is just a one liner anyways..
>>     
>
> If it is not a big deal to fix that, then it might be good to do it. It will
> avoid further debate.
>   
Personally, I consider this a non-issue,but,  if it makes people happy,
I will add a wrapper here..

Regards,
Nishanth Menon