OPP: Introduces enum for addressing different OPP types

Introduces enum for identifying OPP types. This helps in querying the OPP layer by passing the type of OPP (enum types) and gets away from maintaining the pointer to the OPP data list outside the OPP layer.

Signed-off-by Romit Dasgupta <[email protected]>

---

 arch/arm/mach-omap2/pm34xx.c          |   15 +--
 arch/arm/mach-omap2/resource34xx.c    |   84 +++++++++------------
 arch/arm/mach-omap2/smartreflex.c     |   33 ++------
 arch/arm/plat-omap/common.c           |    6 -
 arch/arm/plat-omap/cpu-omap.c         |   12 +--
 arch/arm/plat-omap/include/plat/opp.h |   60 +++++++--------
 arch/arm/plat-omap/omap-pm-noop.c     |    4 -
 arch/arm/plat-omap/omap-pm-srf.c      |    4 -
 arch/arm/plat-omap/opp.c              |   96 +++++++++++++++---------
 9 files changed, 148 insertions(+), 166 deletions(-)

arch/arm/mach-omap2/pm34xx.c +1 -5

index 5c751c1..8c73e0e 100644
@@ -1347,7 +1347,7 @@ static void __init configure_vc(void)
 
 void __init omap3_pm_init_opp_table(void)
 {
-       int i;
+       int i, ret, entries;
        struct omap_opp_def **omap3_opp_def_list;
        struct omap_opp_def *omap34xx_opp_def_list[] = {
                omap34xx_mpu_rate_table,

@@ -1359,18 +1359,15 @@ void __init omap3_pm_init_opp_table(void)
                omap36xx_l3_rate_table,
                omap36xx_dsp_rate_table
        };
-       struct omap_opp **omap3_rate_tables[] = {
-               &mpu_opps,
-               &l3_opps,
-               &dsp_opps

arch/arm/mach-omap2/pm34xx.c +1 -1

                /* We dont want half configured system at the moment */
-               BUG_ON(IS_ERR(omap3_rate_tables[i]));
+               BUG_ON(ret);
        }
 }
 

arch/arm/mach-omap2/resource34xx.c +1 -1

index 157b38e..38c44ee 100644
@@ -161,7 +161,7 @@ static DEFINE_MUTEX(dvfs_mutex);
 /**
  * opp_to_freq - convert OPPID to frequency (DEPRECATED)
  * @freq: return frequency back to caller
- * @opps: opp list
+ * @opp_t: OPP type where we need to look.
  * @opp_id: OPP ID we are searching for
  *
  * return 0 and freq is populated if we find the opp_id, else,

arch/arm/mach-omap2/resource34xx.c +1 -1

-       opp = opp_find_by_opp_id(opps, opp_id);
+       opp = opp_find_by_opp_id(opp_t, opp_id);
        if (!opp)
                return -EINVAL;
 

@@ -188,20 +188,20 @@ static int __deprecated opp_to_freq(unsigned long *freq,
 /**
  * freq_to_opp - convert a frequency back to OPP ID (DEPRECATED)
  * @opp_id: opp ID returned back to caller

arch/arm/mach-omap2/resource34xx.c +2 -2

                unsigned long freq)
 {
        struct omap_opp *opp;
 
-       BUG_ON(!opp_id || !opps);
-       opp = opp_find_freq_ceil(opps, &freq);
+       BUG_ON(opp_t == OPP_NONE || opp_t > OPP_TYPES);
+       opp = opp_find_freq_ceil(opp_t, &freq);
        if (IS_ERR(opp))
                return -EINVAL;
        *opp_id = opp_get_opp_id(opp);

arch/arm/mach-omap2/resource34xx.c +6 -6

        /* Initialize the current level of the OPP resource
        * to the  opp set by u-boot.
        */

@@ -228,14 +225,14 @@ void init_opp(struct shared_resource *resp)
                vdd1_resp = resp;
                dpll1_clk = clk_get(NULL, "dpll1_ck");
                dpll2_clk = clk_get(NULL, "dpll2_ck");
-               ret = freq_to_opp(&opp_id, mpu_opps, dpll1_clk->rate);
+               ret = freq_to_opp(&opp_id, OPP_MPU, 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");
-               ret = freq_to_opp(&opp_id, l3_opps, l3_clk->rate);
+               ret = freq_to_opp(&opp_id, OPP_L3, l3_clk->rate);
                BUG_ON(ret); /* TBD Cleanup handling */
                curr_vdd2_opp = opp_id;
        }

@@ -288,13 +285,13 @@ static int program_opp_freq(int res, int target_level, int current_level)
 
        /* 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);
+               ret = opp_to_freq(&mpu_freq, OPP_MPU, target_level);
+               ret |= opp_to_freq(&dsp_freq, OPP_DSP, target_level);
 #ifndef CONFIG_CPU_FREQ
-               ret |= opp_to_freq(&mpu_cur_freq, mpu_opps, current_level);
+               ret |= opp_to_freq(&mpu_cur_freq, OPP_MPU, current_level);
 #endif
        } else {
-               ret = opp_to_freq(&l3_freq, l3_opps, target_level);
+               ret = opp_to_freq(&l3_freq, OPP_L3, target_level);
        }
        /* we would have caught all bad levels earlier.. */
        if (unlikely(ret))

arch/arm/mach-omap2/resource34xx.c +14 -17

                int current_level)
 {
        int i, ret = 0, raise;

@@ -342,7 +339,7 @@ static int program_opp(int res, struct omap_opp *opp, int target_level,
 #endif
 
        /* See if have a freq associated, if not, invalid opp */
-       ret = opp_to_freq(&freq, opp, target_level);
+       ret = opp_to_freq(&freq, opp_t, target_level);
        if (unlikely(ret))
                return ret;
 

@@ -365,13 +362,13 @@ static int program_opp(int res, struct omap_opp *opp, int target_level,
                         * transitioning from good to good OPP
                         * none of the following should fail..
                         */
-                       oppx = opp_find_freq_exact(opp, freq, true);
+                       oppx = opp_find_freq_exact(opp_t, freq, true);
                        BUG_ON(IS_ERR(oppx));
                        uvdc = opp_get_voltage(oppx);
                        vt = omap_twl_uv_to_vsel(uvdc);
 
-                       BUG_ON(opp_to_freq(&freq, opp, current_level));
-                       oppx = opp_find_freq_exact(opp, freq, true);
+                       BUG_ON(opp_to_freq(&freq, opp_t, current_level));
+                       oppx = opp_find_freq_exact(opp_t, freq, true);
                        BUG_ON(IS_ERR(oppx));
                        uvdc = opp_get_voltage(oppx);
                        vc = omap_twl_uv_to_vsel(uvdc);

@@ -404,15 +401,12 @@ int resource_set_opp_level(int res, u32 target_level, int flags)
        if (resp->curr_level == target_level)
                return 0;
 
-       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);
+               ret = opp_to_freq(&mpu_freq, OPP_MPU, target_level);
+               ret |= opp_to_freq(&mpu_old_freq, OPP_MPU, resp->curr_level);
        } else {
-               ret = opp_to_freq(&l3_freq, l3_opps, target_level);
+               ret = opp_to_freq(&l3_freq, OPP_L3, target_level);
        }
        if (ret)
                return ret;

@@ -431,7 +425,7 @@ int resource_set_opp_level(int res, u32 target_level, int flags)
                /* Send pre notification to CPUFreq */
                cpufreq_notify_transition(&freqs_notify, CPUFREQ_PRECHANGE);
 #endif
-               resp->curr_level = program_opp(res, mpu_opps, target_level,
+               resp->curr_level = program_opp(res, OPP_MPU, target_level,
                        resp->curr_level);
 #ifdef CONFIG_CPU_FREQ
                /* Send a post notification to CPUFreq */

@@ -442,7 +436,7 @@ int resource_set_opp_level(int res, u32 target_level, int flags)
                        mutex_unlock(&dvfs_mutex);
                        return 0;
                }
-               resp->curr_level = program_opp(res, l3_opps, target_level,
+               resp->curr_level = program_opp(res, OPP_L3, target_level,
                        resp->curr_level);
        }
        mutex_unlock(&dvfs_mutex);

@@ -474,17 +468,17 @@ int set_opp(struct shared_resource *resp, u32 target_level)
                req_l3_freq = (target_level * 1000)/4;
 
                /* Do I have a best match? */
-               oppx = opp_find_freq_ceil(l3_opps, &req_l3_freq);
+               oppx = opp_find_freq_ceil(OPP_L3, &req_l3_freq);
                if (IS_ERR(oppx)) {
                        /* Give me the best we got */
                        req_l3_freq = ULONG_MAX;
-                       oppx = opp_find_freq_floor(l3_opps, &req_l3_freq);
+                       oppx = opp_find_freq_floor(OPP_L3, &req_l3_freq);
                }
 
                /* uh uh.. no OPPs?? */
                BUG_ON(IS_ERR(oppx));
 
-               ret = freq_to_opp((u8 *)&target_level, l3_opps, req_l3_freq);
+               ret = freq_to_opp((u8 *)&target_level, OPP_L3, req_l3_freq);
                /* we dont expect this to fail */
                BUG_ON(ret);
 

@@ -503,9 +497,9 @@ 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);
+               return opp_to_freq(&x, OPP_MPU, target_level);
        else if (strcmp(resp->name, "dsp_freq") == 0)
-               return opp_to_freq(&x, dsp_opps, target_level);
+               return opp_to_freq(&x, OPP_DSP, target_level);
        return 0;
 }
 

arch/arm/mach-omap2/resource34xx.c +6 -9

        linked_res_name = (char *)resp->resource_data;
        /* Initialize the current level of the Freq resource
        * to the frequency set by u-boot.
        */
        if (strcmp(resp->name, "mpu_freq") == 0)
                /* MPU freq in Mhz */
-               ret = opp_to_freq(&freq, mpu_opps, curr_vdd1_opp);
+               ret = opp_to_freq(&freq, OPP_MPU, curr_vdd1_opp);
        else if (strcmp(resp->name, "dsp_freq") == 0)
                /* DSP freq in Mhz */
-               ret = opp_to_freq(&freq, dsp_opps, curr_vdd1_opp);
+               ret = opp_to_freq(&freq, OPP_DSP, curr_vdd1_opp);
        BUG_ON(ret);
 
        resp->curr_level = freq;

@@ -543,16 +534,13 @@ int set_freq(struct shared_resource *resp, u32 target_level)
        u8 vdd1_opp;
        int ret = -EINVAL;
 
-       if (!mpu_opps || !dsp_opps)
-               return 0;
-
        if (strcmp(resp->name, "mpu_freq") == 0) {
-               ret = freq_to_opp(&vdd1_opp, mpu_opps, target_level);
+               ret = freq_to_opp(&vdd1_opp, OPP_MPU, target_level);
                if (!ret)
                        ret = resource_request("vdd1_opp", &dummy_mpu_dev,
                                        vdd1_opp);
        } else if (strcmp(resp->name, "dsp_freq") == 0) {
-               ret = freq_to_opp(&vdd1_opp, dsp_opps, target_level);
+               ret = freq_to_opp(&vdd1_opp, OPP_DSP, target_level);
                if (!ret)
                        ret = resource_request("vdd1_opp", &dummy_dsp_dev,
                                        vdd1_opp);

@@ -566,8 +554,8 @@ 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);
+               return freq_to_opp(&x, OPP_MPU, target_level);
        else if (strcmp(resp->name, "dsp_freq") == 0)
-               return freq_to_opp(&x, dsp_opps, target_level);
+               return freq_to_opp(&x, OPP_DSP, target_level);
        return 0;
 }

arch/arm/mach-omap2/smartreflex.c +10 -17

index 3c37837..1c5ec37 100644
@@ -152,12 +152,11 @@ static u8 get_vdd1_opp(void)
        struct omap_opp *opp;
        unsigned long freq;
 
-       if (sr1.vdd_opp_clk == NULL || IS_ERR(sr1.vdd_opp_clk) ||
-                                                       mpu_opps == NULL)
+       if (sr1.vdd_opp_clk == NULL || IS_ERR(sr1.vdd_opp_clk))
                return 0;
 
        freq = sr1.vdd_opp_clk->rate;
-       opp = opp_find_freq_ceil(mpu_opps, &freq);
+       opp = opp_find_freq_ceil(OPP_MPU, &freq);
        if (IS_ERR(opp))
                return 0;
        /*

@@ -176,12 +175,11 @@ static u8 get_vdd2_opp(void)
        struct omap_opp *opp;
        unsigned long freq;
 
-       if (sr2.vdd_opp_clk == NULL || IS_ERR(sr2.vdd_opp_clk) ||
-                                                       l3_opps == NULL)
+       if (sr2.vdd_opp_clk == NULL || IS_ERR(sr2.vdd_opp_clk))
                return 0;
 
        freq = sr2.vdd_opp_clk->rate;
-       opp = opp_find_freq_ceil(l3_opps, &freq);
+       opp = opp_find_freq_ceil(OPP_L3, &freq);
        if (IS_ERR(opp))
                return 0;
 

@@ -310,7 +308,7 @@ static void sr_configure_vp(int srid)
                if (!target_opp_no)
                        target_opp_no = VDD1_OPP3;
 
-               opp = opp_find_by_opp_id(mpu_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_MPU, target_opp_no);
                BUG_ON(!opp); /* XXX ugh */
 
                uvdc = opp_get_voltage(opp);

@@ -359,7 +357,7 @@ static void sr_configure_vp(int srid)
                if (!target_opp_no)
                        target_opp_no = VDD2_OPP3;
 
-               opp = opp_find_by_opp_id(l3_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_L3, target_opp_no);
                BUG_ON(!opp); /* XXX ugh */
 
                uvdc = opp_get_voltage(opp);

@@ -472,7 +470,7 @@ static int sr_reset_voltage(int srid)
                        return 1;
                }
 
-               opp = opp_find_by_opp_id(mpu_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_MPU, target_opp_no);
                if (!opp)
                        return 1;
 

@@ -490,7 +488,7 @@ static int sr_reset_voltage(int srid)
                        return 1;
                }
 
-               opp = opp_find_by_opp_id(l3_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_L3, target_opp_no);
                if (!opp)
                        return 1;
 

@@ -546,11 +544,6 @@ static int sr_enable(struct omap_sr *sr, u32 target_opp_no)
        int uvdc;
        char vsel;
 
-       if (!(mpu_opps && l3_opps)) {
-               pr_notice("VSEL values not found\n");
-               return false;
-       }
-
        sr->req_opp_no = target_opp_no;
 
        if (sr->srid == SR1) {

@@ -575,7 +568,7 @@ static int sr_enable(struct omap_sr *sr, u32 target_opp_no)
                        break;
                }
 
-               opp = opp_find_by_opp_id(mpu_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_MPU, target_opp_no);
                if (!opp)
                        return false;
        } else {

@@ -594,7 +587,7 @@ static int sr_enable(struct omap_sr *sr, u32 target_opp_no)
                        break;
                }
 
-               opp = opp_find_by_opp_id(l3_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_L3, target_opp_no);
                if (!opp)
                        return false;
        }

arch/arm/mach-omap2/smartreflex.c +0 -0

        /* Enable SR on T2 */
        ret = twl_i2c_read_u8(TWL4030_MODULE_PM_RECEIVER, &RdReg,
                              R_DCDC_GLOBAL_CFG);

arch/arm/plat-omap/common.c +0 -6

index 6bd62ea..dddc027 100644
@@ -52,12 +52,6 @@ int omap_board_config_size;
 /* used by omap-smp.c and board-4430sdp.c */
 void __iomem *gic_cpu_base_addr;
 
-#ifdef CONFIG_OMAP_PM_NONE
-struct omap_opp *mpu_opps;
-struct omap_opp *dsp_opps;
-struct omap_opp *l3_opps;
-#endif
-
 static const void *get_config(u16 tag, size_t len, int skip, size_t *len_out)
 {
        struct omap_board_config_kernel *kinfo = NULL;

arch/arm/plat-omap/cpu-omap.c +6 -6

index 109203e..a69b557 100644
@@ -87,6 +87,9 @@ static int omap_target(struct cpufreq_policy *policy,
 #ifdef CONFIG_ARCH_OMAP1
        struct cpufreq_freqs freqs;
 #endif
+#if defined(CONFIG_ARCH_OMAP3) && !defined(CONFIG_OMAP_PM_NONE)
+       unsigned long freq = target_freq * 1000;
+#endif
        int ret = 0;
 
        /* Ensure desired rate is within allowed range.  Some govenors

@@ -111,11 +114,8 @@ static int omap_target(struct cpufreq_policy *policy,
        ret = clk_set_rate(mpu_clk, freqs.new * 1000);
        cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 #elif defined(CONFIG_ARCH_OMAP3) && !defined(CONFIG_OMAP_PM_NONE)
-       if (mpu_opps) {
-               unsigned long freq = target_freq * 1000;
-               if (!IS_ERR(opp_find_freq_ceil(mpu_opps, &freq)))
-                       omap_pm_cpu_set_freq(freq);
-       }
+       if (opp_find_freq_ceil(OPP_MPU, &freq))
+               omap_pm_cpu_set_freq(freq);
 #endif
        return ret;
 }

@@ -136,7 +136,7 @@ static int __init omap_cpu_init(struct cpufreq_policy *policy)
        if (!cpu_is_omap34xx())
                clk_init_cpufreq_table(&freq_table);
        else
-               opp_init_cpufreq_table(mpu_opps, &freq_table);
+               opp_init_cpufreq_table(OPP_MPU, &freq_table);
 
        if (freq_table) {
                result = cpufreq_frequency_table_cpuinfo(policy, freq_table);

arch/arm/plat-omap/include/plat/opp.h +0 -0

index 9f91ad3..c4d5bf9 100644
@@ -13,9 +13,18 @@
 #ifndef __ASM_ARM_OMAP_OPP_H

arch/arm/plat-omap/include/plat/opp.h +1 -0

+};
 
 struct omap_opp;
 

@@ -40,16 +49,16 @@ unsigned long opp_get_freq(const struct omap_opp *opp);
 /**

arch/arm/plat-omap/include/plat/opp.h +1 -1

 /**
  * opp_find_freq_exact() - search for an exact frequency
- * @oppl:      OPP list
+ * @opp_t:     OPP type we want to search in.
  * @freq:      frequency to search for
  * @enabled:   enabled/disabled OPP to search for
  *

arch/arm/plat-omap/include/plat/opp.h +1 -1

                                     unsigned long freq, bool enabled);
 
 /* XXX This documentation needs fixing */
 
 /**
  * opp_find_freq_floor() - Search for an rounded freq
- * @oppl:      Starting list
+ * @opp_t:     OPP type we want to search in
  * @freq:      Start frequency
  *
  * Search for the lower *enabled* OPP from a starting freq

arch/arm/plat-omap/include/plat/opp.h +1 -1

 /* XXX This documentation needs fixing */
 
 /**
  * opp_find_freq_ceil() - Search for an rounded freq
- * @oppl:      Starting list
+ * @opp_t:     OPP type where we want to search in
  * @freq:      Start frequency
  *
  * Search for the higher *enabled* OPP from a starting freq

arch/arm/plat-omap/include/plat/opp.h +1 -1

 /**

@@ -170,35 +178,27 @@ struct omap_opp_def {
 
 /**
  * opp_init_list() - Initialize an opp list from the opp definitions
+ * @opp_t:     OPP type to initialize this list for.
  * @opp_defs:  Initial opp definitions to create the list.
  *
- * This function creates a list of opp definitions and returns a handle.

arch/arm/plat-omap/include/plat/opp.h +4 -12

+                        const struct omap_opp_def *opp_defs);
 
 /**
  * opp_add()  - Add an OPP table from a table definitions
- * @oppl:      List to add the OPP to
+ * @opp_t:     OPP type under which we want to add our new OPP.
  * @opp_def:   omap_opp_def to describe the OPP which we want to add to list.
  *
- * This function adds an opp definition to the opp list and returns
- * a handle representing the new OPP list. This handle is then used for further
- * validation, search, modification operations on the OPP list.
- *
- * This function returns the pointer to the allocated list through oppl if
- * success, else corresponding ERR_PTR value. Caller should NOT free the oppl.
- * opps_defs can be freed after use.
+ * This function adds an opp definition to the opp list and returns status.
  *
- * NOTE: caller should assume that on success, oppl is probably populated with
- * a new handle and the new handle should be used for further referencing
  */
-struct omap_opp *opp_add(struct omap_opp *oppl,
-                        const struct omap_opp_def *opp_def);
+int opp_add(enum opp_t opp_t, const struct omap_opp_def *opp_def);
 
 /**
  * opp_enable() - Enable a specific OPP

arch/arm/plat-omap/include/plat/opp.h +0 -0

                            struct cpufreq_frequency_table **table);
 
 

arch/arm/plat-omap/omap-pm-noop.c +0 -4

index aeb372e..713a59f 100644
@@ -26,10 +26,6 @@
 
 #include <plat/powerdomain.h>
 
-struct omap_opp *dsp_opps;
-struct omap_opp *mpu_opps;
-struct omap_opp *l3_opps;
-
 /*
  * Device-driver-originated constraints (via board-*.c files)
  */

arch/arm/plat-omap/omap-pm-srf.c +0 -4

index f7bf353..9b4cf7f 100644
@@ -25,10 +25,6 @@
 #include <plat/resource.h>
 #include <plat/omap_device.h>
 
-struct omap_opp *dsp_opps;
-struct omap_opp *mpu_opps;
-struct omap_opp *l3_opps;
-
 #define LAT_RES_POSTAMBLE "_latency"
 #define MAX_LATENCY_RES_NAME 30
 

arch/arm/plat-omap/opp.c +0 -0

index 4fe1933..aa21fc5 100644

arch/arm/plat-omap/opp.c +6 -4

 /*
  * DEPRECATED: Meant to detect end of opp array
  * This is meant to help co-exist with current SRF etc

@@ -65,18 +67,20 @@ unsigned long opp_get_freq(const struct omap_opp *opp)
 
 /**
  * opp_find_by_opp_id - look up OPP by OPP ID (deprecated)
- * @opps: pointer to an array of struct omap_opp
+ * @opp_t: OPP type where we want the look up to happen.
  *
  * Returns the struct omap_opp pointer corresponding to the given OPP
  * ID @opp_id, or returns NULL on error.
  */
-struct omap_opp * __deprecated opp_find_by_opp_id(struct omap_opp *opps,
+struct omap_opp * __deprecated opp_find_by_opp_id(enum opp_t opp_t,
                                                  u8 opp_id)
 {
+       struct omap_opp *opps;
        int i = 0;
 
-       if (!opps || !opp_id)
+       if (opp_t == OPP_NONE || opp_t > OPP_TYPES || !opp_id)
                return NULL;
+       opps = _opp_list[opp_t - 1];
 
        while (!OPP_TERM(&opps[i])) {
                if (opps[i].enabled && (opps[i].opp_id == opp_id))

@@ -93,14 +97,17 @@ u8 __deprecated opp_get_opp_id(struct omap_opp *opp)
        return opp->opp_id;
 }
 
-int opp_get_opp_count(struct omap_opp *oppl)
+int opp_get_opp_count(enum opp_t opp_t)
 {

arch/arm/plat-omap/opp.c +48 -31

        while (!OPP_TERM(oppl)) {
                if (oppl->enabled)
                        n++;

@@ -109,14 +116,18 @@ int opp_get_opp_count(struct omap_opp *oppl)
        return n;
 }
 
-struct omap_opp *opp_find_freq_exact(struct omap_opp *oppl,
+struct omap_opp *opp_find_freq_exact(enum opp_t opp_t,
                                     unsigned long freq, bool enabled)
 {
-       if (unlikely(!oppl || IS_ERR(oppl))) {
+       struct omap_opp *oppl;
+
+       if (unlikely(opp_t == OPP_NONE || opp_t > OPP_TYPES)) {
                pr_err("%s: Invalid parameters being passed\n", __func__);
                return ERR_PTR(-EINVAL);
        }
 
+       oppl = _opp_list[opp_t - 1];
+
        while (!OPP_TERM(oppl)) {
                if ((oppl->rate == freq) && (oppl->enabled == enabled))
                        break;

@@ -126,13 +137,18 @@ struct omap_opp *opp_find_freq_exact(struct omap_opp *oppl,
        return OPP_TERM(oppl) ? ERR_PTR(-ENOENT) : oppl;
 }
 
-struct omap_opp *opp_find_freq_ceil(struct omap_opp *oppl, unsigned long *freq)
+struct omap_opp *opp_find_freq_ceil(enum opp_t opp_t, unsigned long *freq)
 {
-       if (unlikely(!oppl || IS_ERR(oppl) || !freq || IS_ERR(freq))) {
+       struct omap_opp *oppl;
+
+       if (unlikely(opp_t == OPP_NONE || opp_t > OPP_TYPES || !freq ||
+                IS_ERR(freq))) {
                pr_err("%s: Invalid parameters being passed\n", __func__);
                return ERR_PTR(-EINVAL);
        }
 
+       oppl = _opp_list[opp_t - 1];
+
        while (!OPP_TERM(oppl)) {
                if (oppl->enabled && oppl->rate >= *freq)
                        break;

@@ -148,14 +164,16 @@ struct omap_opp *opp_find_freq_ceil(struct omap_opp *oppl, unsigned long *freq)
        return oppl;
 }
 
-struct omap_opp *opp_find_freq_floor(struct omap_opp *oppl, unsigned long *freq)
+struct omap_opp *opp_find_freq_floor(enum opp_t opp_t, unsigned long *freq)
 {
-       struct omap_opp *prev_opp = oppl;
+       struct omap_opp *prev_opp, *oppl;
 
-       if (unlikely(!oppl || IS_ERR(oppl) || !freq || IS_ERR(freq))) {
+       if (unlikely(opp_t == OPP_NONE || opp_t > OPP_TYPES || !freq ||
+                IS_ERR(freq))) {
                pr_err("%s: Invalid parameters being passed\n", __func__);
                return ERR_PTR(-EINVAL);
        }
+       oppl = prev_opp = _opp_list[opp_t - 1];
 
        while (!OPP_TERM(oppl)) {
                if (oppl->enabled) {

@@ -185,19 +203,19 @@ static void omap_opp_populate(struct omap_opp *opp,
        opp->u_volt = opp_def->u_volt;
 }
 
-struct omap_opp *opp_add(struct omap_opp *oppl,
-                        const struct omap_opp_def *opp_def)
+int opp_add(enum opp_t opp_t, const struct omap_opp_def *opp_def)
 {
-       struct omap_opp *opp, *oppt, *oppr;
+       struct omap_opp *opp, *oppt, *oppr, *oppl;
        u8 n, i, ins;
 
-       if (unlikely(!oppl || IS_ERR(oppl) || !opp_def || IS_ERR(opp_def))) {
+       if (unlikely(opp_t == OPP_NONE || opp_t > OPP_TYPES || !opp_def ||
+                        IS_ERR(opp_def))) {
                pr_err("%s: Invalid params being passed\n", __func__);
-               return ERR_PTR(-EINVAL);
+               return -EINVAL;
        }
 
        n = 0;
-       opp = oppl;
+       oppl = opp = _opp_list[opp_t - 1];
        while (!OPP_TERM(opp)) {
                n++;
                opp++;

@@ -210,11 +228,11 @@ struct omap_opp *opp_add(struct omap_opp *oppl,
        oppr = kzalloc(sizeof(struct omap_opp) * (n + 2), GFP_KERNEL);
        if (!oppr) {
                pr_err("%s: No memory for new opp array\n", __func__);
-               return ERR_PTR(-ENOMEM);
+               return -ENOMEM;
        }
 
        /* Simple insertion sort */
-       opp = oppl;
+       opp = _opp_list[opp_t - 1];
        oppt = oppr;
        ins = 0;
        i = 1;

@@ -238,23 +256,27 @@ struct omap_opp *opp_add(struct omap_opp *oppl,
                oppt++;
        }
 
+       _opp_list[opp_t - 1] = oppr;
+
        /* Terminator implicitly added by kzalloc() */
 
        /* Free the old list */
        kfree(oppl);
 
-       return oppr;
+       return 0;
 }
 
-struct omap_opp __init *opp_init_list(const struct omap_opp_def *opp_defs)
+int __init opp_init_list(enum opp_t opp_t,
+                                const struct omap_opp_def *opp_defs)
 {
        struct omap_opp_def *t = (struct omap_opp_def *)opp_defs;
-       struct omap_opp *opp, *oppl;
+       struct omap_opp *oppl;
        u8 n = 0, i = 1;
 
-       if (unlikely(!opp_defs || IS_ERR(opp_defs))) {
+       if (unlikely(opp_t == OPP_NONE || opp_t > OPP_TYPES || !opp_defs ||
+                        IS_ERR(opp_defs))) {
                pr_err("%s: Invalid params being passed\n", __func__);
-               return ERR_PTR(-EINVAL);
+               return -EINVAL;
        }
        /* Grab a count */
        while (t->enabled || t->freq || t->u_volt) {

@@ -269,22 +291,23 @@ struct omap_opp __init *opp_init_list(const struct omap_opp_def *opp_defs)
        oppl = kzalloc(sizeof(struct omap_opp) * (n + 1), GFP_KERNEL);
        if (!oppl) {
                pr_err("%s: No memory for opp array\n", __func__);
-               return ERR_PTR(-ENOMEM);
+               return -ENOMEM;
        }
 
-       opp = oppl;
+
+       _opp_list[opp_t - 1] = oppl;
        while (n) {
-               omap_opp_populate(opp, opp_defs);
-               opp->opp_id = i;
+               omap_opp_populate(oppl, opp_defs);
+               oppl->opp_id = i;
                n--;
-               opp++;
+               oppl++;
                opp_defs++;
                i++;
        }
 
        /* Terminator implicitly added by kzalloc() */
 
-       return oppl;
+       return 0;
 }
 
 int opp_enable(struct omap_opp *opp)

@@ -308,20 +331,21 @@ int opp_disable(struct omap_opp *opp)
 }
 
 /* XXX document */
-void opp_init_cpufreq_table(struct omap_opp *opps,
+void opp_init_cpufreq_table(enum opp_t opp_t,
                            struct cpufreq_frequency_table **table)
 {
        int i = 0, j;
        int opp_num;
        struct cpufreq_frequency_table *freq_table;
+       struct omap_opp *opp;
 
-       if (!opps) {
+       if (opp_t == OPP_NONE || opp_t > OPP_TYPES) {
                pr_warning("%s: failed to initialize frequency"
                                "table\n", __func__);
                return;
        }
 
-       opp_num = opp_get_opp_count(opps);
+       opp_num = opp_get_opp_count(opp_t);
        if (opp_num < 0) {
                pr_err("%s: no opp table?\n", __func__);
                return;

@@ -335,14 +359,14 @@ void opp_init_cpufreq_table(struct omap_opp *opps,
                return;
        }
 
+       opp = _opp_list[opp_t - 1];
        for (j = opp_num; j >= 0; j--) {
-               struct omap_opp *opp = &opps[j];
-
                if (opp->enabled) {
                        freq_table[i].index = i;
                        freq_table[i].frequency = opp->rate / 1000;
                        i++;
                }
+               opp++;
        }
 
        freq_table[i].index = i;

Introduces enum for identifying OPP types. This helps in querying the OPP layer by passing the type of OPP (enum types) and gets away from maintaining the pointer to the OPP data list outside the OPP layer.

Signed-off-by Romit Dasgupta <[email protected]>

arch/arm/plat-omap/include/plat/opp.h +0 -0

index 9f91ad3..c4d5bf9 100644
@@ -13,9 +13,18 @@
 #ifndef __ASM_ARM_OMAP_OPP_H

From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Kevin Hilman
Sent: Tuesday, January 12, 2010 6:19 PM
To: Dasgupta, Romit
Cc: Menon, Nishanth; [email protected]
Subject: Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types

Quoted 2 lines · Romit Dasgupta

Introduces enum for identifying OPP types. This helps in querying the OPP layer by passing the type of OPP (enum types) and gets away from

maintaining

Quoted 3 lines

the pointer to the OPP data list outside the OPP layer.

Signed-off-by Romit Dasgupta <[email protected]>

I like this idea... but I have some questions about how we should cleanly handle SMP and future SoCs.

arch/arm/plat-

Quoted 1 line

arch/arm/plat- renamed from arch/arm/plat-omap/include/plat/opp.h +0 -0

omap/include/plat/opp.h

Quoted 5 lines

index 9f91ad3..c4d5bf9 100644

arch/arm/plat-omap/include/plat/opp.h +0 -0

@@ -13,9 +13,18 @@
 #ifndef __ASM_ARM_OMAP_OPP_H

arch/arm/plat-omap/include/plat/opp.h line 20

Quoted 10 lines

arch/arm/plat-omap/include/plat/opp.h +5 -3

 #define __ASM_ARM_OMAP_OPP_H
 
-extern struct omap_opp *mpu_opps;
-extern struct omap_opp *dsp_opps;
-extern struct omap_opp *l3_opps;
+#ifdef CONFIG_ARCH_OMAP3
+#define OPP_TYPES 3
+#else
+#error "You need to put the number of OPP types for OMAP chip type."
+#endif

Rather than the #ifdef...

arch/arm/plat-omap/include/plat/opp.h line 26

Quoted 5 lines

arch/arm/plat-omap/include/plat/opp.h +5 -0

+enum opp_t {
+    OPP_NONE,
+    OPP_MPU,
+    OPP_L3,
+    OPP_DSP

add OPP_MAX_TYPES here

arch/arm/plat-omap/include/plat/opp.h line 27

Quoted 1 line

arch/arm/plat-omap/include/plat/opp.h +1 -0

+};

And with that, how do you suggest handling SMP. Do we assume that the OPP_MPU OPPs are common across all CPUs? Do we possibly need other types for other busses on future SoCs?

We don't need the answeres to all these questions today, but I'd like to know your thoughts on how this would be extended or made SoC specific.

Thanks,

Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi Kevin,

Quoted 12 lines

From: [email protected] [mailto:linux-omap- [email protected]] On Behalf Of Kevin Hilman
Sent: Tuesday, January 12, 2010 6:19 PM
To: Dasgupta, Romit
Cc: Menon, Nishanth; [email protected]
Subject: Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types

Quoted 2 lines · Romit Dasgupta

Introduces enum for identifying OPP types. This helps in querying the OPP layer by passing the type of OPP (enum types) and gets away from

Quoted 7 lines

maintaining

Quoted 3 lines

the pointer to the OPP data list outside the OPP layer.

Signed-off-by Romit Dasgupta <[email protected]>

I like this idea... but I have some questions about how we should cleanly handle SMP and future SoCs.

Well, it is better than what we have today, but maybe not super scalable for next device. This information is completely device dependant and should not be in a plat-omap file. In that case, it will be tricky to have an enum per devices because it will prevent multiple omap build. That for that reason that I think we might use hwmod to identify the relevant scalable IPs, instead of yet another identifier for something that is IP related.

I like this idea... but I have some questions about how we should cleanly handle SMP and future SoCs.

Quoted 7 lines

arch/arm/plat-omap/include/plat/opp.h +0 -0

index 9f91ad3..c4d5bf9 100644
@@ -13,9 +13,18 @@
 #ifndef __ASM_ARM_OMAP_OPP_H

arch/arm/plat-omap/include/plat/opp.h line 20

Quoted 10 lines

arch/arm/plat-omap/include/plat/opp.h +5 -3

 #define __ASM_ARM_OMAP_OPP_H
 
-extern struct omap_opp *mpu_opps;
-extern struct omap_opp *dsp_opps;
-extern struct omap_opp *l3_opps;
+#ifdef CONFIG_ARCH_OMAP3
+#define OPP_TYPES 3
+#else
+#error "You need to put the number of OPP types for OMAP chip type."
+#endif

Rather than the #ifdef...

arch/arm/plat-omap/include/plat/opp.h line 26

Quoted 5 lines

arch/arm/plat-omap/include/plat/opp.h +5 -0

+enum opp_t {
+	OPP_NONE,
+	OPP_MPU,
+	OPP_L3,
+	OPP_DSP

add OPP_MAX_TYPES here

arch/arm/plat-omap/include/plat/opp.h line 27

Quoted 1 line

arch/arm/plat-omap/include/plat/opp.h +1 -0

+};

Introduces enum for identifying OPP types. This helps in querying the OPP layer by passing the type of OPP (enum types) and gets away from maintaining the pointer to the OPP data list outside the OPP layer.

Signed-off-by Romit Dasgupta <[email protected]>

---

 arch/arm/mach-omap2/pm34xx.c          |   15 +--
 arch/arm/mach-omap2/resource34xx.c    |   84 +++++++++------------
 arch/arm/mach-omap2/smartreflex.c     |   33 ++------
 arch/arm/plat-omap/common.c           |    6 -
 arch/arm/plat-omap/cpu-omap.c         |   12 +--
 arch/arm/plat-omap/include/plat/opp.h |   60 +++++++--------
 arch/arm/plat-omap/omap-pm-noop.c     |    4 -
 arch/arm/plat-omap/omap-pm-srf.c      |    4 -
 arch/arm/plat-omap/opp.c              |   96 +++++++++++++++---------
 9 files changed, 148 insertions(+), 166 deletions(-)

arch/arm/mach-omap2/pm34xx.c +1 -5

index 5c751c1..8c73e0e 100644
@@ -1347,7 +1347,7 @@ static void __init configure_vc(void)
 
 void __init omap3_pm_init_opp_table(void)
 {
-       int i;
+       int i, ret, entries;
        struct omap_opp_def **omap3_opp_def_list;
        struct omap_opp_def *omap34xx_opp_def_list[] = {
                omap34xx_mpu_rate_table,

@@ -1359,18 +1359,15 @@ void __init omap3_pm_init_opp_table(void)
                omap36xx_l3_rate_table,
                omap36xx_dsp_rate_table
        };
-       struct omap_opp **omap3_rate_tables[] = {
-               &mpu_opps,
-               &l3_opps,
-               &dsp_opps

arch/arm/mach-omap2/pm34xx.c +1 -1

                /* We dont want half configured system at the moment */
-               BUG_ON(IS_ERR(omap3_rate_tables[i]));
+               BUG_ON(ret);
        }
 }
 

arch/arm/mach-omap2/resource34xx.c +1 -1

index 157b38e..38c44ee 100644
@@ -161,7 +161,7 @@ static DEFINE_MUTEX(dvfs_mutex);
 /**
  * opp_to_freq - convert OPPID to frequency (DEPRECATED)
  * @freq: return frequency back to caller
- * @opps: opp list
+ * @opp_t: OPP type where we need to look.
  * @opp_id: OPP ID we are searching for
  *
  * return 0 and freq is populated if we find the opp_id, else,

arch/arm/mach-omap2/resource34xx.c +1 -1

-       opp = opp_find_by_opp_id(opps, opp_id);
+       opp = opp_find_by_opp_id(opp_t, opp_id);
        if (!opp)
                return -EINVAL;
 

@@ -188,20 +188,20 @@ static int __deprecated opp_to_freq(unsigned long *freq,
 /**
  * freq_to_opp - convert a frequency back to OPP ID (DEPRECATED)
  * @opp_id: opp ID returned back to caller

arch/arm/mach-omap2/resource34xx.c +2 -2

                unsigned long freq)
 {
        struct omap_opp *opp;
 
-       BUG_ON(!opp_id || !opps);
-       opp = opp_find_freq_ceil(opps, &freq);
+       BUG_ON(opp_t == OPP_NONE || opp_t > OPP_TYPES);
+       opp = opp_find_freq_ceil(opp_t, &freq);
        if (IS_ERR(opp))
                return -EINVAL;
        *opp_id = opp_get_opp_id(opp);

arch/arm/mach-omap2/resource34xx.c +6 -6

        /* Initialize the current level of the OPP resource
        * to the  opp set by u-boot.
        */

@@ -228,14 +225,14 @@ void init_opp(struct shared_resource *resp)
                vdd1_resp = resp;
                dpll1_clk = clk_get(NULL, "dpll1_ck");
                dpll2_clk = clk_get(NULL, "dpll2_ck");
-               ret = freq_to_opp(&opp_id, mpu_opps, dpll1_clk->rate);
+               ret = freq_to_opp(&opp_id, OPP_MPU, 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");
-               ret = freq_to_opp(&opp_id, l3_opps, l3_clk->rate);
+               ret = freq_to_opp(&opp_id, OPP_L3, l3_clk->rate);
                BUG_ON(ret); /* TBD Cleanup handling */
                curr_vdd2_opp = opp_id;
        }

@@ -288,13 +285,13 @@ static int program_opp_freq(int res, int target_level, int current_level)
 
        /* 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);
+               ret = opp_to_freq(&mpu_freq, OPP_MPU, target_level);
+               ret |= opp_to_freq(&dsp_freq, OPP_DSP, target_level);
 #ifndef CONFIG_CPU_FREQ
-               ret |= opp_to_freq(&mpu_cur_freq, mpu_opps, current_level);
+               ret |= opp_to_freq(&mpu_cur_freq, OPP_MPU, current_level);
 #endif
        } else {
-               ret = opp_to_freq(&l3_freq, l3_opps, target_level);
+               ret = opp_to_freq(&l3_freq, OPP_L3, target_level);
        }
        /* we would have caught all bad levels earlier.. */
        if (unlikely(ret))

arch/arm/mach-omap2/resource34xx.c +14 -17

                int current_level)
 {
        int i, ret = 0, raise;

@@ -342,7 +339,7 @@ static int program_opp(int res, struct omap_opp *opp, int target_level,
 #endif
 
        /* See if have a freq associated, if not, invalid opp */
-       ret = opp_to_freq(&freq, opp, target_level);
+       ret = opp_to_freq(&freq, opp_t, target_level);
        if (unlikely(ret))
                return ret;
 

@@ -365,13 +362,13 @@ static int program_opp(int res, struct omap_opp *opp, int target_level,
                         * transitioning from good to good OPP
                         * none of the following should fail..
                         */
-                       oppx = opp_find_freq_exact(opp, freq, true);
+                       oppx = opp_find_freq_exact(opp_t, freq, true);
                        BUG_ON(IS_ERR(oppx));
                        uvdc = opp_get_voltage(oppx);
                        vt = omap_twl_uv_to_vsel(uvdc);
 
-                       BUG_ON(opp_to_freq(&freq, opp, current_level));
-                       oppx = opp_find_freq_exact(opp, freq, true);
+                       BUG_ON(opp_to_freq(&freq, opp_t, current_level));
+                       oppx = opp_find_freq_exact(opp_t, freq, true);
                        BUG_ON(IS_ERR(oppx));
                        uvdc = opp_get_voltage(oppx);
                        vc = omap_twl_uv_to_vsel(uvdc);

@@ -404,15 +401,12 @@ int resource_set_opp_level(int res, u32 target_level, int flags)
        if (resp->curr_level == target_level)
                return 0;
 
-       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);
+               ret = opp_to_freq(&mpu_freq, OPP_MPU, target_level);
+               ret |= opp_to_freq(&mpu_old_freq, OPP_MPU, resp->curr_level);
        } else {
-               ret = opp_to_freq(&l3_freq, l3_opps, target_level);
+               ret = opp_to_freq(&l3_freq, OPP_L3, target_level);
        }
        if (ret)
                return ret;

@@ -431,7 +425,7 @@ int resource_set_opp_level(int res, u32 target_level, int flags)
                /* Send pre notification to CPUFreq */
                cpufreq_notify_transition(&freqs_notify, CPUFREQ_PRECHANGE);
 #endif
-               resp->curr_level = program_opp(res, mpu_opps, target_level,
+               resp->curr_level = program_opp(res, OPP_MPU, target_level,
                        resp->curr_level);
 #ifdef CONFIG_CPU_FREQ
                /* Send a post notification to CPUFreq */

@@ -442,7 +436,7 @@ int resource_set_opp_level(int res, u32 target_level, int flags)
                        mutex_unlock(&dvfs_mutex);
                        return 0;
                }
-               resp->curr_level = program_opp(res, l3_opps, target_level,
+               resp->curr_level = program_opp(res, OPP_L3, target_level,
                        resp->curr_level);
        }
        mutex_unlock(&dvfs_mutex);

@@ -474,17 +468,17 @@ int set_opp(struct shared_resource *resp, u32 target_level)
                req_l3_freq = (target_level * 1000)/4;
 
                /* Do I have a best match? */
-               oppx = opp_find_freq_ceil(l3_opps, &req_l3_freq);
+               oppx = opp_find_freq_ceil(OPP_L3, &req_l3_freq);
                if (IS_ERR(oppx)) {
                        /* Give me the best we got */
                        req_l3_freq = ULONG_MAX;
-                       oppx = opp_find_freq_floor(l3_opps, &req_l3_freq);
+                       oppx = opp_find_freq_floor(OPP_L3, &req_l3_freq);
                }
 
                /* uh uh.. no OPPs?? */
                BUG_ON(IS_ERR(oppx));
 
-               ret = freq_to_opp((u8 *)&target_level, l3_opps, req_l3_freq);
+               ret = freq_to_opp((u8 *)&target_level, OPP_L3, req_l3_freq);
                /* we dont expect this to fail */
                BUG_ON(ret);
 

@@ -503,9 +497,9 @@ 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);
+               return opp_to_freq(&x, OPP_MPU, target_level);
        else if (strcmp(resp->name, "dsp_freq") == 0)
-               return opp_to_freq(&x, dsp_opps, target_level);
+               return opp_to_freq(&x, OPP_DSP, target_level);
        return 0;
 }
 

arch/arm/mach-omap2/resource34xx.c +6 -9

        linked_res_name = (char *)resp->resource_data;
        /* Initialize the current level of the Freq resource
        * to the frequency set by u-boot.
        */
        if (strcmp(resp->name, "mpu_freq") == 0)
                /* MPU freq in Mhz */
-               ret = opp_to_freq(&freq, mpu_opps, curr_vdd1_opp);
+               ret = opp_to_freq(&freq, OPP_MPU, curr_vdd1_opp);
        else if (strcmp(resp->name, "dsp_freq") == 0)
                /* DSP freq in Mhz */
-               ret = opp_to_freq(&freq, dsp_opps, curr_vdd1_opp);
+               ret = opp_to_freq(&freq, OPP_DSP, curr_vdd1_opp);
        BUG_ON(ret);
 
        resp->curr_level = freq;

@@ -543,16 +534,13 @@ int set_freq(struct shared_resource *resp, u32 target_level)
        u8 vdd1_opp;
        int ret = -EINVAL;
 
-       if (!mpu_opps || !dsp_opps)
-               return 0;
-
        if (strcmp(resp->name, "mpu_freq") == 0) {
-               ret = freq_to_opp(&vdd1_opp, mpu_opps, target_level);
+               ret = freq_to_opp(&vdd1_opp, OPP_MPU, target_level);
                if (!ret)
                        ret = resource_request("vdd1_opp", &dummy_mpu_dev,
                                        vdd1_opp);
        } else if (strcmp(resp->name, "dsp_freq") == 0) {
-               ret = freq_to_opp(&vdd1_opp, dsp_opps, target_level);
+               ret = freq_to_opp(&vdd1_opp, OPP_DSP, target_level);
                if (!ret)
                        ret = resource_request("vdd1_opp", &dummy_dsp_dev,
                                        vdd1_opp);

@@ -566,8 +554,8 @@ 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);
+               return freq_to_opp(&x, OPP_MPU, target_level);
        else if (strcmp(resp->name, "dsp_freq") == 0)
-               return freq_to_opp(&x, dsp_opps, target_level);
+               return freq_to_opp(&x, OPP_DSP, target_level);
        return 0;
 }

arch/arm/mach-omap2/smartreflex.c +10 -17

index 3c37837..1c5ec37 100644
@@ -152,12 +152,11 @@ static u8 get_vdd1_opp(void)
        struct omap_opp *opp;
        unsigned long freq;
 
-       if (sr1.vdd_opp_clk == NULL || IS_ERR(sr1.vdd_opp_clk) ||
-                                                       mpu_opps == NULL)
+       if (sr1.vdd_opp_clk == NULL || IS_ERR(sr1.vdd_opp_clk))
                return 0;
 
        freq = sr1.vdd_opp_clk->rate;
-       opp = opp_find_freq_ceil(mpu_opps, &freq);
+       opp = opp_find_freq_ceil(OPP_MPU, &freq);
        if (IS_ERR(opp))
                return 0;
        /*

@@ -176,12 +175,11 @@ static u8 get_vdd2_opp(void)
        struct omap_opp *opp;
        unsigned long freq;
 
-       if (sr2.vdd_opp_clk == NULL || IS_ERR(sr2.vdd_opp_clk) ||
-                                                       l3_opps == NULL)
+       if (sr2.vdd_opp_clk == NULL || IS_ERR(sr2.vdd_opp_clk))
                return 0;
 
        freq = sr2.vdd_opp_clk->rate;
-       opp = opp_find_freq_ceil(l3_opps, &freq);
+       opp = opp_find_freq_ceil(OPP_L3, &freq);
        if (IS_ERR(opp))
                return 0;
 

@@ -310,7 +308,7 @@ static void sr_configure_vp(int srid)
                if (!target_opp_no)
                        target_opp_no = VDD1_OPP3;
 
-               opp = opp_find_by_opp_id(mpu_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_MPU, target_opp_no);
                BUG_ON(!opp); /* XXX ugh */
 
                uvdc = opp_get_voltage(opp);

@@ -359,7 +357,7 @@ static void sr_configure_vp(int srid)
                if (!target_opp_no)
                        target_opp_no = VDD2_OPP3;
 
-               opp = opp_find_by_opp_id(l3_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_L3, target_opp_no);
                BUG_ON(!opp); /* XXX ugh */
 
                uvdc = opp_get_voltage(opp);

@@ -472,7 +470,7 @@ static int sr_reset_voltage(int srid)
                        return 1;
                }
 
-               opp = opp_find_by_opp_id(mpu_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_MPU, target_opp_no);
                if (!opp)
                        return 1;
 

@@ -490,7 +488,7 @@ static int sr_reset_voltage(int srid)
                        return 1;
                }
 
-               opp = opp_find_by_opp_id(l3_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_L3, target_opp_no);
                if (!opp)
                        return 1;
 

@@ -546,11 +544,6 @@ static int sr_enable(struct omap_sr *sr, u32 target_opp_no)
        int uvdc;
        char vsel;
 
-       if (!(mpu_opps && l3_opps)) {
-               pr_notice("VSEL values not found\n");
-               return false;
-       }
-
        sr->req_opp_no = target_opp_no;
 
        if (sr->srid == SR1) {

@@ -575,7 +568,7 @@ static int sr_enable(struct omap_sr *sr, u32 target_opp_no)
                        break;
                }
 
-               opp = opp_find_by_opp_id(mpu_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_MPU, target_opp_no);
                if (!opp)
                        return false;
        } else {

@@ -594,7 +587,7 @@ static int sr_enable(struct omap_sr *sr, u32 target_opp_no)
                        break;
                }
 
-               opp = opp_find_by_opp_id(l3_opps, target_opp_no);
+               opp = opp_find_by_opp_id(OPP_L3, target_opp_no);
                if (!opp)
                        return false;
        }

arch/arm/mach-omap2/smartreflex.c +0 -0

        /* Enable SR on T2 */
        ret = twl_i2c_read_u8(TWL4030_MODULE_PM_RECEIVER, &RdReg,
                              R_DCDC_GLOBAL_CFG);

arch/arm/plat-omap/common.c +0 -6

index 6bd62ea..dddc027 100644
@@ -52,12 +52,6 @@ int omap_board_config_size;
 /* used by omap-smp.c and board-4430sdp.c */
 void __iomem *gic_cpu_base_addr;
 
-#ifdef CONFIG_OMAP_PM_NONE
-struct omap_opp *mpu_opps;
-struct omap_opp *dsp_opps;
-struct omap_opp *l3_opps;
-#endif
-
 static const void *get_config(u16 tag, size_t len, int skip, size_t *len_out)
 {
        struct omap_board_config_kernel *kinfo = NULL;

arch/arm/plat-omap/cpu-omap.c +6 -6

index 109203e..a69b557 100644
@@ -87,6 +87,9 @@ static int omap_target(struct cpufreq_policy *policy,
 #ifdef CONFIG_ARCH_OMAP1
        struct cpufreq_freqs freqs;
 #endif
+#if defined(CONFIG_ARCH_OMAP3) && !defined(CONFIG_OMAP_PM_NONE)
+       unsigned long freq = target_freq * 1000;
+#endif
        int ret = 0;
 
        /* Ensure desired rate is within allowed range.  Some govenors

@@ -111,11 +114,8 @@ static int omap_target(struct cpufreq_policy *policy,
        ret = clk_set_rate(mpu_clk, freqs.new * 1000);
        cpufreq_notify_transition(&freqs, CPUFREQ_POSTCHANGE);
 #elif defined(CONFIG_ARCH_OMAP3) && !defined(CONFIG_OMAP_PM_NONE)
-       if (mpu_opps) {
-               unsigned long freq = target_freq * 1000;
-               if (!IS_ERR(opp_find_freq_ceil(mpu_opps, &freq)))
-                       omap_pm_cpu_set_freq(freq);
-       }
+       if (opp_find_freq_ceil(OPP_MPU, &freq))
+               omap_pm_cpu_set_freq(freq);
 #endif
        return ret;
 }

@@ -136,7 +136,7 @@ static int __init omap_cpu_init(struct cpufreq_policy *policy)
        if (!cpu_is_omap34xx())
                clk_init_cpufreq_table(&freq_table);
        else
-               opp_init_cpufreq_table(mpu_opps, &freq_table);
+               opp_init_cpufreq_table(OPP_MPU, &freq_table);
 
        if (freq_table) {
                result = cpufreq_frequency_table_cpuinfo(policy, freq_table);

arch/arm/plat-omap/include/plat/opp.h +0 -0

index 9f91ad3..c4d5bf9 100644
@@ -13,9 +13,18 @@
 #ifndef __ASM_ARM_OMAP_OPP_H

arch/arm/plat-omap/include/plat/opp.h +1 -0

+};
 
 struct omap_opp;
 

@@ -40,16 +49,16 @@ unsigned long opp_get_freq(const struct omap_opp *opp);
 /**

arch/arm/plat-omap/include/plat/opp.h +1 -1

 /**
  * opp_find_freq_exact() - search for an exact frequency
- * @oppl:      OPP list
+ * @opp_t:     OPP type we want to search in.
  * @freq:      frequency to search for
  * @enabled:   enabled/disabled OPP to search for
  *

arch/arm/plat-omap/include/plat/opp.h +1 -1

                                     unsigned long freq, bool enabled);
 
 /* XXX This documentation needs fixing */
 
 /**
  * opp_find_freq_floor() - Search for an rounded freq
- * @oppl:      Starting list
+ * @opp_t:     OPP type we want to search in
  * @freq:      Start frequency
  *
  * Search for the lower *enabled* OPP from a starting freq

arch/arm/plat-omap/include/plat/opp.h +1 -1

 /* XXX This documentation needs fixing */
 
 /**
  * opp_find_freq_ceil() - Search for an rounded freq
- * @oppl:      Starting list
+ * @opp_t:     OPP type where we want to search in
  * @freq:      Start frequency
  *
  * Search for the higher *enabled* OPP from a starting freq

arch/arm/plat-omap/include/plat/opp.h +1 -1

 /**

@@ -170,35 +178,27 @@ struct omap_opp_def {
 
 /**
  * opp_init_list() - Initialize an opp list from the opp definitions
+ * @opp_t:     OPP type to initialize this list for.
  * @opp_defs:  Initial opp definitions to create the list.
  *
- * This function creates a list of opp definitions and returns a handle.

arch/arm/plat-omap/include/plat/opp.h +4 -12

+                        const struct omap_opp_def *opp_defs);
 
 /**
  * opp_add()  - Add an OPP table from a table definitions
- * @oppl:      List to add the OPP to
+ * @opp_t:     OPP type under which we want to add our new OPP.
  * @opp_def:   omap_opp_def to describe the OPP which we want to add to list.
  *
- * This function adds an opp definition to the opp list and returns
- * a handle representing the new OPP list. This handle is then used for further
- * validation, search, modification operations on the OPP list.
- *
- * This function returns the pointer to the allocated list through oppl if
- * success, else corresponding ERR_PTR value. Caller should NOT free the oppl.
- * opps_defs can be freed after use.
+ * This function adds an opp definition to the opp list and returns status.
  *
- * NOTE: caller should assume that on success, oppl is probably populated with
- * a new handle and the new handle should be used for further referencing
  */
-struct omap_opp *opp_add(struct omap_opp *oppl,
-                        const struct omap_opp_def *opp_def);
+int opp_add(enum opp_t opp_t, const struct omap_opp_def *opp_def);
 
 /**
  * opp_enable() - Enable a specific OPP

arch/arm/plat-omap/include/plat/opp.h +0 -0

                            struct cpufreq_frequency_table **table);
 
 

arch/arm/plat-omap/omap-pm-noop.c +0 -4

index aeb372e..713a59f 100644
@@ -26,10 +26,6 @@
 
 #include <plat/powerdomain.h>
 
-struct omap_opp *dsp_opps;
-struct omap_opp *mpu_opps;
-struct omap_opp *l3_opps;
-
 /*
  * Device-driver-originated constraints (via board-*.c files)
  */

arch/arm/plat-omap/omap-pm-srf.c +0 -4

index f7bf353..9b4cf7f 100644
@@ -25,10 +25,6 @@
 #include <plat/resource.h>
 #include <plat/omap_device.h>
 
-struct omap_opp *dsp_opps;
-struct omap_opp *mpu_opps;
-struct omap_opp *l3_opps;
-
 #define LAT_RES_POSTAMBLE "_latency"
 #define MAX_LATENCY_RES_NAME 30
 

arch/arm/plat-omap/opp.c +0 -0

index 4fe1933..aa21fc5 100644

arch/arm/plat-omap/opp.c +6 -4

 /*
  * DEPRECATED: Meant to detect end of opp array
  * This is meant to help co-exist with current SRF etc

@@ -65,18 +67,20 @@ unsigned long opp_get_freq(const struct omap_opp *opp)
 
 /**
  * opp_find_by_opp_id - look up OPP by OPP ID (deprecated)
- * @opps: pointer to an array of struct omap_opp
+ * @opp_t: OPP type where we want the look up to happen.
  *
  * Returns the struct omap_opp pointer corresponding to the given OPP
  * ID @opp_id, or returns NULL on error.
  */
-struct omap_opp * __deprecated opp_find_by_opp_id(struct omap_opp *opps,
+struct omap_opp * __deprecated opp_find_by_opp_id(enum opp_t opp_t,
                                                  u8 opp_id)
 {
+       struct omap_opp *opps;
        int i = 0;
 
-       if (!opps || !opp_id)
+       if (opp_t == OPP_NONE || opp_t > OPP_TYPES || !opp_id)
                return NULL;
+       opps = _opp_list[opp_t - 1];
 
        while (!OPP_TERM(&opps[i])) {
                if (opps[i].enabled && (opps[i].opp_id == opp_id))

@@ -93,14 +97,17 @@ u8 __deprecated opp_get_opp_id(struct omap_opp *opp)
        return opp->opp_id;
 }
 
-int opp_get_opp_count(struct omap_opp *oppl)
+int opp_get_opp_count(enum opp_t opp_t)
 {

arch/arm/plat-omap/opp.c +47 -30

        while (!OPP_TERM(oppl)) {
                if (oppl->enabled)
                        n++;

@@ -109,14 +116,18 @@ int opp_get_opp_count(struct omap_opp *oppl)
        return n;
 }
 
-struct omap_opp *opp_find_freq_exact(struct omap_opp *oppl,
+struct omap_opp *opp_find_freq_exact(enum opp_t opp_t,
                                     unsigned long freq, bool enabled)
 {
-       if (unlikely(!oppl || IS_ERR(oppl))) {
+       struct omap_opp *oppl;
+
+       if (unlikely(opp_t == OPP_NONE || opp_t > OPP_TYPES)) {
                pr_err("%s: Invalid parameters being passed\n", __func__);
                return ERR_PTR(-EINVAL);
        }
 
+       oppl = _opp_list[opp_t - 1];
+
        while (!OPP_TERM(oppl)) {
                if ((oppl->rate == freq) && (oppl->enabled == enabled))
                        break;

@@ -126,13 +137,18 @@ struct omap_opp *opp_find_freq_exact(struct omap_opp *oppl,
        return OPP_TERM(oppl) ? ERR_PTR(-ENOENT) : oppl;
 }
 
-struct omap_opp *opp_find_freq_ceil(struct omap_opp *oppl, unsigned long *freq)
+struct omap_opp *opp_find_freq_ceil(enum opp_t opp_t, unsigned long *freq)
 {
-       if (unlikely(!oppl || IS_ERR(oppl) || !freq || IS_ERR(freq))) {
+       struct omap_opp *oppl;
+
+       if (unlikely(opp_t == OPP_NONE || opp_t > OPP_TYPES || !freq ||
+                IS_ERR(freq))) {
                pr_err("%s: Invalid parameters being passed\n", __func__);
                return ERR_PTR(-EINVAL);
        }
 
+       oppl = _opp_list[opp_t - 1];
+
        while (!OPP_TERM(oppl)) {
                if (oppl->enabled && oppl->rate >= *freq)
                        break;

@@ -148,14 +164,16 @@ struct omap_opp *opp_find_freq_ceil(struct omap_opp *oppl, unsigned long *freq)
        return oppl;
 }
 
-struct omap_opp *opp_find_freq_floor(struct omap_opp *oppl, unsigned long *freq)
+struct omap_opp *opp_find_freq_floor(enum opp_t opp_t, unsigned long *freq)
 {
-       struct omap_opp *prev_opp = oppl;
+       struct omap_opp *prev_opp, *oppl;
 
-       if (unlikely(!oppl || IS_ERR(oppl) || !freq || IS_ERR(freq))) {
+       if (unlikely(opp_t == OPP_NONE || opp_t > OPP_TYPES || !freq ||
+                IS_ERR(freq))) {
                pr_err("%s: Invalid parameters being passed\n", __func__);
                return ERR_PTR(-EINVAL);
        }
+       oppl = prev_opp = _opp_list[opp_t - 1];
 
        while (!OPP_TERM(oppl)) {
                if (oppl->enabled) {

@@ -185,19 +203,19 @@ static void omap_opp_populate(struct omap_opp *opp,
        opp->u_volt = opp_def->u_volt;
 }
 
-struct omap_opp *opp_add(struct omap_opp *oppl,
-                        const struct omap_opp_def *opp_def)
+int opp_add(enum opp_t opp_t, const struct omap_opp_def *opp_def)
 {
-       struct omap_opp *opp, *oppt, *oppr;
+       struct omap_opp *opp, *oppt, *oppr, *oppl;
        u8 n, i, ins;
 
-       if (unlikely(!oppl || IS_ERR(oppl) || !opp_def || IS_ERR(opp_def))) {
+       if (unlikely(opp_t == OPP_NONE || opp_t > OPP_TYPES || !opp_def ||
+                        IS_ERR(opp_def))) {
                pr_err("%s: Invalid params being passed\n", __func__);
-               return ERR_PTR(-EINVAL);
+               return -EINVAL;
        }
 
        n = 0;
-       opp = oppl;
+       oppl = opp = _opp_list[opp_t - 1];
        while (!OPP_TERM(opp)) {
                n++;
                opp++;

@@ -210,11 +228,11 @@ struct omap_opp *opp_add(struct omap_opp *oppl,
        oppr = kzalloc(sizeof(struct omap_opp) * (n + 2), GFP_KERNEL);
        if (!oppr) {
                pr_err("%s: No memory for new opp array\n", __func__);
-               return ERR_PTR(-ENOMEM);
+               return -ENOMEM;
        }
 
        /* Simple insertion sort */
-       opp = oppl;
+       opp = _opp_list[opp_t - 1];
        oppt = oppr;
        ins = 0;
        i = 1;

@@ -238,23 +256,27 @@ struct omap_opp *opp_add(struct omap_opp *oppl,
                oppt++;
        }
 
+       _opp_list[opp_t - 1] = oppr;
+
        /* Terminator implicitly added by kzalloc() */
 
        /* Free the old list */
        kfree(oppl);
 
-       return oppr;
+       return 0;
 }
 
-struct omap_opp __init *opp_init_list(const struct omap_opp_def *opp_defs)
+int __init opp_init_list(enum opp_t opp_t,
+                                const struct omap_opp_def *opp_defs)
 {
        struct omap_opp_def *t = (struct omap_opp_def *)opp_defs;
-       struct omap_opp *opp, *oppl;
+       struct omap_opp *oppl;
        u8 n = 0, i = 1;
 
-       if (unlikely(!opp_defs || IS_ERR(opp_defs))) {
+       if (unlikely(opp_t == OPP_NONE || opp_t > OPP_TYPES || !opp_defs ||
+                        IS_ERR(opp_defs))) {
                pr_err("%s: Invalid params being passed\n", __func__);
-               return ERR_PTR(-EINVAL);
+               return -EINVAL;
        }
        /* Grab a count */
        while (t->enabled || t->freq || t->u_volt) {

@@ -269,22 +291,23 @@ struct omap_opp __init *opp_init_list(const struct omap_opp_def *opp_defs)
        oppl = kzalloc(sizeof(struct omap_opp) * (n + 1), GFP_KERNEL);
        if (!oppl) {
                pr_err("%s: No memory for opp array\n", __func__);
-               return ERR_PTR(-ENOMEM);
+               return -ENOMEM;
        }
 
-       opp = oppl;
+
+       _opp_list[opp_t - 1] = oppl;
        while (n) {
-               omap_opp_populate(opp, opp_defs);
-               opp->opp_id = i;
+               omap_opp_populate(oppl, opp_defs);
+               oppl->opp_id = i;
                n--;
-               opp++;
+               oppl++;
                opp_defs++;
                i++;
        }
 
        /* Terminator implicitly added by kzalloc() */
 
-       return oppl;
+       return 0;
 }
 
 int opp_enable(struct omap_opp *opp)

@@ -308,20 +331,21 @@ int opp_disable(struct omap_opp *opp)
 }
 
 /* XXX document */
-void opp_init_cpufreq_table(struct omap_opp *opps,
+void opp_init_cpufreq_table(enum opp_t opp_t,
                            struct cpufreq_frequency_table **table)
 {
        int i = 0, j;
        int opp_num;
        struct cpufreq_frequency_table *freq_table;
+       struct omap_opp *opp;
 
-       if (!opps) {
+       if (opp_t == OPP_NONE || opp_t > OPP_TYPES) {
                pr_warning("%s: failed to initialize frequency"
                                "table\n", __func__);
                return;
        }
 
-       opp_num = opp_get_opp_count(opps);
+       opp_num = opp_get_opp_count(opp_t);
        if (opp_num < 0) {
                pr_err("%s: no opp table?\n", __func__);
                return;

@@ -335,14 +359,14 @@ void opp_init_cpufreq_table(struct omap_opp *opps,
                return;
        }
 
+       opp = _opp_list[opp_t - 1];
        for (j = opp_num; j >= 0; j--) {
-               struct omap_opp *opp = &opps[j];

General comment: might be good to state the enum types you are introducing for OMAP3 in the commit message

arch/arm/mach-omap2/pm34xx.c void __init omap3_pm_init_opp_table(void) · line 1368

Quoted 12 lines

Signed-off-by Romit Dasgupta <[email protected]>

---

        omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
                                omap34xx_opp_def_list;

arch/arm/mach-omap2/pm34xx.c +4 -2

-       for (i = 0; i < ARRAY_SIZE(omap3_rate_tables); i++) {
-               *omap3_rate_tables[i] = opp_init_list(omap3_opp_def_list[i]);
+       entries = cpu_is_omap3630() ? ARRAY_SIZE(omap34xx_opp_def_list) :
+                       ARRAY_SIZE(omap36xx_opp_def_list);
+       for (i = 1; i <= entries; i++) {
+               ret = opp_init_list(i, omap3_opp_def_list[i - 1]);

a) if you remove OPP_NONE, i-1 is not needed (same everywhere in the patch)

b) if we modify the ENUMS or the sequence of definitions in opp_t the logic here becomes fault. it might be good to retain an equivalent of omap3_rate_table with enum equivalents and register by indexing off that.

Quoted 22 lines

                /* We dont want half configured system at the moment */

arch/arm/mach-omap2/pm34xx.c +1 -1

-               BUG_ON(IS_ERR(omap3_rate_tables[i]));
+               BUG_ON(ret);
        }
 }

arch/arm/mach-omap2/resource34xx.c +1 -1

index 157b38e..38c44ee 100644
@@ -161,7 +161,7 @@ static DEFINE_MUTEX(dvfs_mutex);
 /**
  * opp_to_freq - convert OPPID to frequency (DEPRECATED)
  * @freq: return frequency back to caller
- * @opps: opp list
+ * @opp_t: OPP type where we need to look.
  * @opp_id: OPP ID we are searching for
  *
  * return 0 and freq is populated if we find the opp_id, else,

@@ -169,14 +169,14 @@ static DEFINE_MUTEX(dvfs_mutex);
  *

arch/arm/mach-omap2/resource34xx.c static DEFINE_MUTEX(dvfs_mutex); · line 172

Quoted 5 lines

arch/arm/mach-omap2/resource34xx.c +1 -2

  * NOTE: this function is a standin for the timebeing as opp_id is deprecated
  */
-static int __deprecated opp_to_freq(unsigned long *freq,
-               const struct omap_opp *opps, u8 opp_id)
+static int __deprecated opp_to_freq(unsigned long *freq, enum opp_t opp_t,

Enum type and variable have the same name :( mebbe a rename of variable is appropriate

arch/arm/mach-omap2/resource34xx.c static int __deprecated opp_to_freq(unsigned long *freq, · line 188

Quoted 3 lines

arch/arm/mach-omap2/resource34xx.c +1 -1

@@ -188,20 +188,20 @@ static int __deprecated opp_to_freq(unsigned long *freq,
-static int __deprecated freq_to_opp(u8 *opp_id, struct omap_opp *opps,
+static int __deprecated freq_to_opp(u8 *opp_id, enum opp_t opp_t,

Re: enum type and variable have the same name :( mebbe a rename of variable is appropriate

Quoted 15 lines

arch/arm/mach-omap2/resource34xx.c +2 -2

                unsigned long freq)
 {
        struct omap_opp *opp;
 
-       BUG_ON(!opp_id || !opps);
-       opp = opp_find_freq_ceil(opps, &freq);
+       BUG_ON(opp_t == OPP_NONE || opp_t > OPP_TYPES);
+       opp = opp_find_freq_ceil(opp_t, &freq);
        if (IS_ERR(opp))
                return -EINVAL;
        *opp_id = opp_get_opp_id(opp);

@@ -218,9 +218,6 @@ void init_opp(struct shared_resource *resp)
        u8 opp_id;
        resp->no_of_users = 0;
 

arch/arm/mach-omap2/resource34xx.c void init_opp(struct shared_resource *resp) · line 223

Quoted 3 lines

arch/arm/mach-omap2/resource34xx.c +0 -3

-       if (!mpu_opps || !dsp_opps || !l3_opps)
-               return;
-

the original intent of this check is lost here - if the initializations did not take place, we will not proceed. An equivalent check might be good to maintain at this point.

arch/arm/mach-omap2/resource34xx.c void init_freq(struct shared_resource *resp) · line 524

Quoted 7 lines

arch/arm/mach-omap2/resource34xx.c +0 -3

@@ -519,19 +513,16 @@ void init_freq(struct shared_resource *resp)
        unsigned long freq;
        resp->no_of_users = 0;
 
-       if (!mpu_opps || !dsp_opps)
-               return;
-

again the initial intent is lost -> to handle cases where the initialization was not being done.

Quoted 3 lines · Nishanth

General comment: might be good to state the enum types you are introducing for OMAP3 in the commit message

Actually the introduction of enum type itself is the heart of the patch. The details are irrelevant.

Quoted 12 lines

Quoted 12 lines

Signed-off-by Romit Dasgupta <[email protected]>

---

        omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
                                omap34xx_opp_def_list;

arch/arm/mach-omap2/pm34xx.c +4 -2

-       for (i = 0; i < ARRAY_SIZE(omap3_rate_tables); i++) {
-               *omap3_rate_tables[i] = opp_init_list(omap3_opp_def_list[i]);
+       entries = cpu_is_omap3630() ? ARRAY_SIZE(omap34xx_opp_def_list) :
+                       ARRAY_SIZE(omap36xx_opp_def_list);
+       for (i = 1; i <= entries; i++) {
+               ret = opp_init_list(i, omap3_opp_def_list[i - 1]);

Quoted 2 lines

a) if you remove OPP_NONE, i-1 is not needed (same everywhere in the patch)

Frankly, I did not want to introduce OPP_NONE but did so as you are checking all parameters passed to the OPP APIs.

Quoted 5 lines

b) if we modify the ENUMS or the sequence of definitions in opp_t the logic here becomes fault. it might be good to retain an equivalent of omap3_rate_table with enum equivalents and register by indexing off that.

You are right but this is a kernel level API and user level code is not going to use this. Having said this there is no scope for a programmer to introduce new sequences without understanding the consequences.

Quoted 21 lines

Quoted 21 lines

                /* We dont want half configured system at the moment */

arch/arm/mach-omap2/pm34xx.c +1 -1

-               BUG_ON(IS_ERR(omap3_rate_tables[i]));
+               BUG_ON(ret);
        }
 }

arch/arm/mach-omap2/resource34xx.c +1 -1

index 157b38e..38c44ee 100644
@@ -161,7 +161,7 @@ static DEFINE_MUTEX(dvfs_mutex);
 /**
  * opp_to_freq - convert OPPID to frequency (DEPRECATED)
  * @freq: return frequency back to caller
- * @opps: opp list
+ * @opp_t: OPP type where we need to look.
  * @opp_id: OPP ID we are searching for
  *
  * return 0 and freq is populated if we find the opp_id, else,

@@ -169,14 +169,14 @@ static DEFINE_MUTEX(dvfs_mutex);
  *

Quoted 9 lines

arch/arm/mach-omap2/resource34xx.c static DEFINE_MUTEX(dvfs_mutex); · line 176

Quoted 6 lines

arch/arm/mach-omap2/resource34xx.c +1 -2

  * NOTE: this function is a standin for the timebeing as opp_id is deprecated
  */
-static int __deprecated opp_to_freq(unsigned long *freq,
-               const struct omap_opp *opps, u8 opp_id)
+static int __deprecated opp_to_freq(unsigned long *freq, enum opp_t opp_t,
 

Enum type and variable have the same name :( mebbe a rename of variable is appropriate

Not sure why you say this. Did you see the compiler throwing up any warning?

Quoted 22 lines

arch/arm/mach-omap2/resource34xx.c static int __deprecated opp_to_freq(unsigned long *freq, · line 189

Quoted 4 lines

arch/arm/mach-omap2/resource34xx.c +1 -1

@@ -188,20 +188,20 @@ static int __deprecated opp_to_freq(unsigned long *freq,
-static int __deprecated freq_to_opp(u8 *opp_id, struct omap_opp *opps,
+static int __deprecated freq_to_opp(u8 *opp_id, enum opp_t opp_t,
 

Re: enum type and variable have the same name :( mebbe a rename of variable is appropriate

Quoted 15 lines

arch/arm/mach-omap2/resource34xx.c +2 -2

                unsigned long freq)
 {
        struct omap_opp *opp;
 
-       BUG_ON(!opp_id || !opps);
-       opp = opp_find_freq_ceil(opps, &freq);
+       BUG_ON(opp_t == OPP_NONE || opp_t > OPP_TYPES);
+       opp = opp_find_freq_ceil(opp_t, &freq);
        if (IS_ERR(opp))
                return -EINVAL;
        *opp_id = opp_get_opp_id(opp);

@@ -218,9 +218,6 @@ void init_opp(struct shared_resource *resp)
        u8 opp_id;
        resp->no_of_users = 0;
 

Quoted 8 lines

arch/arm/mach-omap2/resource34xx.c void init_opp(struct shared_resource *resp)

Quoted 4 lines

arch/arm/mach-omap2/resource34xx.c +0 -3

-       if (!mpu_opps || !dsp_opps || !l3_opps)
-               return;
-
 

the original intent of this check is lost here - if the initializations did not take place, we will not proceed. An equivalent check might be good to maintain at this point.

You are partially correct. I took off the checks because we have a BUG_ON() call in the beginning of the boot code right after we initialize the OPP tables. So we should not hit this check.

Romit Dasgupta said the following on 01/13/2010 04:41 AM:

Quoted 8 lines

Quoted 3 lines · Nishanth

General comment: might be good to state the enum types you are introducing for OMAP3 in the commit message

Actually the introduction of enum type itself is the heart of the patch. The details are irrelevant.

could you be a little more verbose as to what is irrelevant? the OMAP3 enums descriptions in commit message?

Quoted 13 lines

Quoted 13 lines

Quoted 13 lines

Signed-off-by Romit Dasgupta <[email protected]>

---

        omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
                                omap34xx_opp_def_list;

arch/arm/mach-omap2/pm34xx.c +4 -2

-       for (i = 0; i < ARRAY_SIZE(omap3_rate_tables); i++) {
-               *omap3_rate_tables[i] = opp_init_list(omap3_opp_def_list[i]);
+       entries = cpu_is_omap3630() ? ARRAY_SIZE(omap34xx_opp_def_list) :
+                       ARRAY_SIZE(omap36xx_opp_def_list);
+       for (i = 1; i <= entries; i++) {
+               ret = opp_init_list(i, omap3_opp_def_list[i - 1]);

Quoted 5 lines

Quoted 2 lines

a) if you remove OPP_NONE, i-1 is not needed (same everywhere in the patch)

Frankly, I did not want to introduce OPP_NONE but did so as you are checking all parameters passed to the OPP APIs.

Lets remove it then.

Quoted 2 lines

definition of enum and the implicit usage of enums are in two different files. there is a distinct possibility of some one modifying the header without actually knowing that .c depends on the exact values of the enum definition.

pm34xx.c has no right to depend on opp.h definition values -> if it does it ties it down and a constraint for future flexibility. please change.

Quoted 8 lines

Quoted 8 lines

arch/arm/mach-omap2/resource34xx.c static DEFINE_MUTEX(dvfs_mutex); · line 172

Quoted 5 lines

*/

arch/arm/mach-omap2/resource34xx.c +1 -2

-static int __deprecated opp_to_freq(unsigned long *freq,
-               const struct omap_opp *opps, u8 opp_id)
+static int __deprecated opp_to_freq(unsigned long *freq, enum opp_t opp_t,

Enum type and variable have the same name :( mebbe a rename of variable is appropriate

Quoted 2 lines

Not sure why you say this. Did you see the compiler throwing up any warning?

The usage later in the code is opp_t -> this is a readability issue not a compiler warning.

Quoted 8 lines

Quoted 4 lines

the original intent of this check is lost here - if the initializations did not take place, we will not proceed. An equivalent check might be good to maintain at this point.

You are partially correct. I took off the checks because we have a BUG_ON() call in the beginning of the boot code right after we initialize the OPP tables. So we should not hit this check.

hmm.. interesting.. can you elaborate with exact functions?

if you are removing this check, please do a follow up patch and maintain equivalent one for this so that the patch does exactly what the commit message says "introduce enums" - not do something more.

Quoted 3 lines · Nishanth Menon

Romit Dasgupta said the following on 01/13/2010 04:41 AM:

Quoted 2 lines

Menon, Nishanth wrote:

Quoted 7 lines

Quoted 6 lines

Quoted 3 lines

General comment: might be good to state the enum types you are introducing for OMAP3 in the commit message

Actually the introduction of enum type itself is the heart of the patch. The details are irrelevant.

could you be a little more verbose as to what is irrelevant? the OMAP3 enums descriptions in commit message?

Yes, because they are going to be SoC specific.

Quoted 9 lines

Quoted 8 lines

Quoted 5 lines

arch/arm/mach-omap2/pm34xx.c void __init omap3_pm_init_opp_table(void) · line 1368

Quoted 3 lines

arch/arm/mach-omap2/pm34xx.c +2 -0

+       for (i = 1; i <= entries; i++) {
+               ret = opp_init_list(i, omap3_opp_def_list[i - 1]);

a) if you remove OPP_NONE, i-1 is not needed (same everywhere in the patch)

Frankly, I did not want to introduce OPP_NONE but did so as you are checking all parameters passed to the OPP APIs.

Lets remove it then.

OK

Quoted 3 lines

Quoted 2 lines

definition of enum and the implicit usage of enums are in two different files. there is a distinct possibility of some one modifying the header without actually knowing that .c depends on the exact values of the enum definition.

As I said before one needs to make changes in the kernel by knowing what they are doing.

Quoted 1 line

pm34xx.c has no right to depend on opp.h definition values -> if it does it ties it down and a constraint for future flexibility. please change.

The right approach should be to take out the loop in pm34xx.c for now and explicitly call the opp_init_list function after passing OPP_MPU, OPP_L3, OPP_DSP in any order. So pm34xx.c needs to change not opp.[ch]. What do you think?

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

arch/arm/mach-omap2/resource34xx.c static DEFINE_MUTEX(dvfs_mutex); · line 172

Quoted 5 lines

*/

arch/arm/mach-omap2/resource34xx.c +1 -2

-static int __deprecated opp_to_freq(unsigned long *freq,
-               const struct omap_opp *opps, u8 opp_id)
+static int __deprecated opp_to_freq(unsigned long *freq, enum opp_t opp_t,

Enum type and variable have the same name :( mebbe a rename of variable is appropriate

Quoted 3 lines

Quoted 2 lines

Not sure why you say this. Did you see the compiler throwing up any warning?

The usage later in the code is opp_t -> this is a readability issue not a compiler warning.

What is the readability issue? Why cant we declare something like enum opp_t opp_t?

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

the original intent of this check is lost here - if the initializations did not take place, we will not proceed. An equivalent check might be good to maintain at this point.

You are partially correct. I took off the checks because we have a BUG_ON() call in the beginning of the boot code right after we initialize the OPP tables. So we should not hit this check.

Quoted 1 line

hmm.. interesting.. can you elaborate with exact functions?

omap3_pm_init_opp_table

Quoted 2 lines

if you are removing this check, please do a follow up patch and maintain equivalent one for this so that the patch does exactly what the commit message says "introduce enums" - not do something more.

How on earth?? I have removed mpu_opps, l3_opps, dsp_opps from the code so how do you think I should preserve the checking of the variables when they don't exist.

Here is a sample commit message I can think of:
----
Using omap_opp * to refer to domain types restricts opp implementation into maintaining pointers outside the opp layer. This causes issues such as:
a) Describing cross domain dependencies (e.g. dsp vs mpu)
b) Ease of transitioning/supporting to multiple silicon variants and families
c) Choice of varied options in implementing opp layer internals.

Since all we need a identifying a specific domain for query/operational purposes, we introduce enum for identifying OPP types instead of using opp layer's internal data structure pointer.

Currently, OMAP3 is the only silicon supporting the OPP layer, hence mpu_opps, l3_opps and dsp_opps are deprecated and replaced with OPP_MPU, OPP_L3 and OPP_DSP respectively.

Quoted 6 lines

Quoted 6 lines

Quoted 2 lines

definition of enum and the implicit usage of enums are in two different files. there is a distinct possibility of some one modifying the header without actually knowing that .c depends on the exact values of the enum definition.

Quoted 7 lines

As I said before one needs to make changes in the kernel by knowing what they are doing.

Quoted 2 lines

pm34xx.c has no right to depend on opp.h definition values -> if it does it ties it down and a constraint for future flexibility. please change.

The right approach should be to take out the loop in pm34xx.c for now and explicitly call the opp_init_list function after passing OPP_MPU, OPP_L3, OPP_DSP in any order. So pm34xx.c needs to change not opp.[ch]. What do you think?

I did dig into this a few mins ago.. and yes I can see similar example in drivers/mfd/twl4030-core.c

The intent of my comment is this: when someone else, few months from now, is focusing on adding/changing opp logic, they will focus on opp.c and .h. we have two choices to handle this:
a) Ensure that users of opp.h do not know how it works internally -> e.g. ordering of opp list for example.
b) add
/* WARNING: See file:arch/arm/mach-omap2/pm34xx.c before modifying the sequence of these enums */ to opp.h
and
/* WARNING: See file:arch/arm/plat-omap/include/plat/opp.h before modifying this */ in pm34xx.c

now, I was recommending doing a, till a thought a little more on the implementation(array based) and how long that implementation might last(we might potentially move opp.c to a list implementation). the effort would be to complicate the opp_init,add functions for a very short lifetime. This effort maynot be worth it.

Quoted 8 lines

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Enum type and variable have the same name :( mebbe a rename of variable is appropriate

Not sure why you say this. Did you see the compiler throwing up any warning?

The usage later in the code is opp_t -> this is a readability issue not a compiler warning.

What is the readability issue? Why cant we declare something like enum opp_t opp_t?

Let me try to explain this clearly. assume we have a struct opp_t (not enum) for the time being.
void some_func(struct opp_t *opp_t)

{
   struct opp_t *opp;

..
200 line of code (>one page full)
....
/* point 1 */

    BUG_ON(opp_t.xyz)

...
200 lines of more code
..
/* point 2 */

    BUG_ON(opp.xyz)

...

}

lets say this is compiled by some non follower of this mail chain, compiler throws an error for point 1: filex:liney
so the guy/gal fires up vim and opens the filex, goes to line y he/she cannot see the start of the function, knows that there is a struct opp_t

I like this message. I will include it.

Quoted 1 line

now, I was recommending doing a, till a thought a little more on the implementation(array based) and how long that implementation might last(we might potentially move opp.c to a list implementation). the effort would be to complicate the opp_init,add functions for a very short lifetime. This effort maynot be worth it.

I understand your concern. I have made some changes in the code. Please look at the reposted patch (in few mins from now I shall post them).

Quoted 22 lines

Quoted 7 lines

Quoted 6 lines

Quoted 5 lines

Quoted 3 lines

Enum type and variable have the same name :( mebbe a rename of variable is appropriate

Not sure why you say this. Did you see the compiler throwing up any warning?

The usage later in the code is opp_t -> this is a readability issue not a compiler warning.

What is the readability issue? Why cant we declare something like enum opp_t opp_t?

Let me try to explain this clearly. assume we have a struct opp_t (not enum) for the time being. void some_func(struct opp_t *opp_t)

{
   struct opp_t *opp;

..
200 line of code (>one page full)
....
/* point 1 */

    BUG_ON(opp_t.xyz)

...
200 lines of more code
..
/* point 2 */

    BUG_ON(opp.xyz)

Quoted 8 lines

...

}

lets say this is compiled by some non follower of this mail chain, compiler throws an error for point 1: filex:liney
so the guy/gal fires up vim and opens the filex, goes to line y he/she cannot see the start of the function, knows that there is a struct opp_t

If a function is that big then the fault lies there to start with! What do you say? Nevertheless, your suggestion is cosmetic but I think we should not assume that developers are so ignorant. For now I will do away with your suggestion. Please feel free to change the code if you think what you say is the right thing.

Quoted 1 line · Nishanth

Quoted 19 lines

arch/arm/mach-omap2/resource34xx.c static int __deprecated opp_to_freq(unsigned long *freq, · line 198

Quoted 2 lines

arch/arm/mach-omap2/resource34xx.c +1 -1

-static int __deprecated freq_to_opp(u8 *opp_id, struct omap_opp *opps,
+static int __deprecated freq_to_opp(u8 *opp_id, enum opp_t opp_t,

Re: enum type and variable have the same name :( mebbe a rename of variable is appropriate

Quoted 15 lines

unsigned long freq)

 {
        struct omap_opp *opp;

arch/arm/mach-omap2/resource34xx.c +2 -2

-       BUG_ON(!opp_id || !opps);
-       opp = opp_find_freq_ceil(opps, &freq);
+       BUG_ON(opp_t == OPP_NONE || opp_t > OPP_TYPES);
+       opp = opp_find_freq_ceil(opp_t, &freq);
        if (IS_ERR(opp))
                return -EINVAL;
        *opp_id = opp_get_opp_id(opp);

@@ -218,9 +218,6 @@ void init_opp(struct shared_resource *resp)
        u8 opp_id;
        resp->no_of_users = 0;
 

Quoted 6 lines

arch/arm/mach-omap2/resource34xx.c void init_opp(struct shared_resource *resp) · line 223

Quoted 3 lines

arch/arm/mach-omap2/resource34xx.c +0 -3

-       if (!mpu_opps || !dsp_opps || !l3_opps)
-               return;
-

the original intent of this check is lost here - if the initializations did not take place, we will not proceed. An equivalent check might be good to maintain at this point.

You are partially correct. I took off the checks because we have a BUG_ON() call in the beginning of the boot code right after we initialize the OPP tables. So we should not hit this check.