* [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
@ 2010-01-12 12:39 Romit Dasgupta
2010-01-12 17:19 ` Nishanth Menon
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Romit Dasgupta @ 2010-01-12 12:39 UTC (permalink / raw)
To: khilman; +Cc: nm, linux-omap
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 <romit@ti.com>
---
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(-)
diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
index 5c751c1..8c73e0e 100644
--- a/arch/arm/mach-omap2/pm34xx.c
+++ b/arch/arm/mach-omap2/pm34xx.c
@@ -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
- };
omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
omap34xx_opp_def_list;
- 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]);
/* We dont want half configured system at the moment */
- BUG_ON(IS_ERR(omap3_rate_tables[i]));
+ BUG_ON(ret);
}
}
diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c
index 157b38e..38c44ee 100644
--- a/arch/arm/mach-omap2/resource34xx.c
+++ b/arch/arm/mach-omap2/resource34xx.c
@@ -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);
*
* 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,
+ u8 opp_id)
{
struct omap_opp *opp;
- BUG_ON(!freq || !opps);
+ BUG_ON(!freq || opp_t == OPP_NONE || opp_t > OPP_TYPES);
- 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
- * @opps: opp list
+ * @opp_t: OPP type where we need to look.
* @freq: frequency we are searching for
*
* return 0 and opp_id is populated if we find the freq, else, we return error
*
* NOTE: this function is a standin for the timebeing as opp_id is deprecated
*/
-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,
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;
- if (!mpu_opps || !dsp_opps || !l3_opps)
- return;
-
/* 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))
@@ -329,7 +326,7 @@ static int program_opp_freq(int res, int target_level, int current_level)
return target_level;
}
-static int program_opp(int res, struct omap_opp *opp, int target_level,
+static int program_opp(int res, enum opp_t opp_t, int target_level,
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;
}
@@ -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;
-
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;
}
diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
index 3c37837..1c5ec37 100644
--- a/arch/arm/mach-omap2/smartreflex.c
+++ b/arch/arm/mach-omap2/smartreflex.c
@@ -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;
}
@@ -1010,12 +1003,6 @@ static int __init omap3_sr_init(void)
int ret = 0;
u8 RdReg;
- /* Exit if OPP tables are not defined */
- if (!(mpu_opps && l3_opps)) {
- pr_err("SR: OPP rate tables not defined for platform, not enabling SmartReflex\n");
- return -ENODEV;
- }
-
/* Enable SR on T2 */
ret = twl_i2c_read_u8(TWL4030_MODULE_PM_RECEIVER, &RdReg,
R_DCDC_GLOBAL_CFG);
diff --git a/arch/arm/plat-omap/common.c b/arch/arm/plat-omap/common.c
index 6bd62ea..dddc027 100644
--- a/arch/arm/plat-omap/common.c
+++ b/arch/arm/plat-omap/common.c
@@ -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;
diff --git a/arch/arm/plat-omap/cpu-omap.c b/arch/arm/plat-omap/cpu-omap.c
index 109203e..a69b557 100644
--- a/arch/arm/plat-omap/cpu-omap.c
+++ b/arch/arm/plat-omap/cpu-omap.c
@@ -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);
diff --git a/arch/arm/plat-omap/include/plat/opp.h b/arch/arm/plat-omap/include/plat/opp.h
index 9f91ad3..c4d5bf9 100644
--- a/arch/arm/plat-omap/include/plat/opp.h
+++ b/arch/arm/plat-omap/include/plat/opp.h
@@ -13,9 +13,18 @@
#ifndef __ASM_ARM_OMAP_OPP_H
#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
+
+enum opp_t {
+ OPP_NONE,
+ OPP_MPU,
+ OPP_L3,
+ OPP_DSP
+};
struct omap_opp;
@@ -40,16 +49,16 @@ unsigned long opp_get_freq(const struct omap_opp *opp);
/**
* opp_get_opp_count() - Get number of opps enabled in the opp list
* @num: returns the number of opps
- * @oppl: opp list
+ * @opp_t: OPP type we want to count
*
* This functions returns the number of opps if there are any OPPs enabled,
* else returns corresponding error value.
*/
-int opp_get_opp_count(struct omap_opp *oppl);
+int opp_get_opp_count(enum opp_t opp_t);
/**
* 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
*
@@ -61,14 +70,14 @@ int opp_get_opp_count(struct omap_opp *oppl);
* for exact matching frequency and is enabled. if true, the match is for exact
* frequency which is disabled.
*/
-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);
/* 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
@@ -97,14 +106,13 @@ struct omap_opp *opp_find_freq_exact(struct omap_opp *oppl,
* NOTE: if we set freq as ULONG_MAX and search low, we get the
* highest enabled frequency
*/
-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);
/* 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
@@ -130,7 +138,7 @@ struct omap_opp *opp_find_freq_floor(struct omap_opp *oppl,
* freq++; * for next higher match *
* }
*/
-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);
/**
@@ -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.
+ * This function creates a list of opp definitions and returns status.
* This list can be used to further validation/search/modifications. New
* opp entries can be added to this list by using opp_add().
*
- * In the case of error, ERR_PTR is returned to the caller and should be
- * appropriately handled with IS_ERR.
+ * In the case of error, suitable error code is returned.
*/
-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);
/**
* 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
@@ -224,11 +224,11 @@ int opp_enable(struct omap_opp *opp);
*/
int opp_disable(struct omap_opp *opp);
-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);
u8 __deprecated opp_get_opp_id(struct omap_opp *opp);
-void opp_init_cpufreq_table(struct omap_opp *opps,
+void opp_init_cpufreq_table(enum opp_t opp_t,
struct cpufreq_frequency_table **table);
diff --git a/arch/arm/plat-omap/omap-pm-noop.c b/arch/arm/plat-omap/omap-pm-noop.c
index aeb372e..713a59f 100644
--- a/arch/arm/plat-omap/omap-pm-noop.c
+++ b/arch/arm/plat-omap/omap-pm-noop.c
@@ -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)
*/
diff --git a/arch/arm/plat-omap/omap-pm-srf.c b/arch/arm/plat-omap/omap-pm-srf.c
index f7bf353..9b4cf7f 100644
--- a/arch/arm/plat-omap/omap-pm-srf.c
+++ b/arch/arm/plat-omap/omap-pm-srf.c
@@ -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
diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
index 4fe1933..aa21fc5 100644
--- a/arch/arm/plat-omap/opp.c
+++ b/arch/arm/plat-omap/opp.c
@@ -38,6 +38,8 @@ struct omap_opp {
u8 opp_id;
};
+
+static struct omap_opp *_opp_list[OPP_TYPES];
/*
* 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)
{
u8 n = 0;
+ struct omap_opp *oppl;
- if (unlikely(!oppl || IS_ERR(oppl))) {
+ if (unlikely((opp_t == OPP_NONE) || opp_t > OPP_TYPES)) {
pr_err("%s: Invalid parameters being passed\n", __func__);
return -EINVAL;
}
+
+ oppl = _opp_list[opp_t - 1];
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;
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-12 12:39 [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types Romit Dasgupta
@ 2010-01-12 17:19 ` Nishanth Menon
2010-01-12 17:19 ` Kevin Hilman
2010-01-12 17:57 ` Menon, Nishanth
2 siblings, 0 replies; 14+ messages in thread
From: Nishanth Menon @ 2010-01-12 17:19 UTC (permalink / raw)
To: Dasgupta, Romit; +Cc: khilman, linux-omap
Dasgupta, Romit had written, on 01/12/2010 06:39 AM, the following:
> 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.
Style comment: an additional EOL here will be nice between commit
message and signed-off-by.
General comment: might be good to state the enum types you are
introducing for
OMAP3 in the commit message
> Signed-off-by: Romit Dasgupta <romit@ti.com>
> ---
>
link to previous discussions here might help reviewers esp to give
context for the design choice.
> 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(-)
>
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 5c751c1..8c73e0e 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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
> - };
>
> omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
> omap34xx_opp_def_list;
> - 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.
> /* We dont want half configured system at the moment */
> - BUG_ON(IS_ERR(omap3_rate_tables[i]));
> + BUG_ON(ret);
> }
> }
>
> diff --git a/arch/arm/mach-omap2/resource34xx.c
b/arch/arm/mach-omap2/resource34xx.c
> index 157b38e..38c44ee 100644
> --- a/arch/arm/mach-omap2/resource34xx.c
> +++ b/arch/arm/mach-omap2/resource34xx.c
> @@ -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);
> *
> * 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
> + u8 opp_id)
> {
> struct omap_opp *opp;
>
> - BUG_ON(!freq || !opps);
> + BUG_ON(!freq || opp_t == OPP_NONE || opp_t > OPP_TYPES);
why OPP_NONE?
>
> - 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
> - * @opps: opp list
> + * @opp_t: OPP type where we need to look.
> * @freq: frequency we are searching for
> *
> * return 0 and opp_id is populated if we find the freq, else, we
return error
> *
> * NOTE: this function is a standin for the timebeing as opp_id is
deprecated
> */
> -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
> 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;
>
> - 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.
> /* 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))
> @@ -329,7 +326,7 @@ static int program_opp_freq(int res, int
target_level, int current_level)
> return target_level;
> }
>
> -static int program_opp(int res, struct omap_opp *opp, int target_level,
> +static int program_opp(int res, enum opp_t opp_t, int target_level,
Re: enum type and variable have the same name :( mebbe a rename of
variable is appropriate
> 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;
> }
>
> @@ -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.
See commit: 0fea42354997641652623a7b046c14d580fca80c
OMAP3 SRF: Fix crash on non-3430SDP platforms with DVFS/CPUFreq
The SRF/DVFS + CPUFreq patches had issues booting on
non-3430SDP platforms as reported by Kevin Hilman.
This patch puts non-NULL checks in place for mpu_opps/dsp_opps
/l3_opps before accessing them and fixes the issue.
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> 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;
> }
> diff --git a/arch/arm/mach-omap2/smartreflex.c
b/arch/arm/mach-omap2/smartreflex.c
> index 3c37837..1c5ec37 100644
> --- a/arch/arm/mach-omap2/smartreflex.c
> +++ b/arch/arm/mach-omap2/smartreflex.c
> @@ -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;
> }
> @@ -1010,12 +1003,6 @@ static int __init omap3_sr_init(void)
> int ret = 0;
> u8 RdReg;
>
> - /* Exit if OPP tables are not defined */
> - if (!(mpu_opps && l3_opps)) {
> - pr_err("SR: OPP rate tables not defined for
platform, not enabling SmartReflex\n");
> - return -ENODEV;
> - }
> -
re: the same intent of handling un-initialized tables, but we loose it
at this point..
> /* Enable SR on T2 */
> ret = twl_i2c_read_u8(TWL4030_MODULE_PM_RECEIVER, &RdReg,
> R_DCDC_GLOBAL_CFG);
> diff --git a/arch/arm/plat-omap/common.c b/arch/arm/plat-omap/common.c
> index 6bd62ea..dddc027 100644
> --- a/arch/arm/plat-omap/common.c
> +++ b/arch/arm/plat-omap/common.c
> @@ -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;
> diff --git a/arch/arm/plat-omap/cpu-omap.c
b/arch/arm/plat-omap/cpu-omap.c
> index 109203e..a69b557 100644
> --- a/arch/arm/plat-omap/cpu-omap.c
> +++ b/arch/arm/plat-omap/cpu-omap.c
> @@ -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);
> diff --git a/arch/arm/plat-omap/include/plat/opp.h
b/arch/arm/plat-omap/include/plat/opp.h
> index 9f91ad3..c4d5bf9 100644
> --- a/arch/arm/plat-omap/include/plat/opp.h
> +++ b/arch/arm/plat-omap/include/plat/opp.h
> @@ -13,9 +13,18 @@
> #ifndef __ASM_ARM_OMAP_OPP_H
> #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
not a strong requirement, but does it make sense to have these to be
part of silicon specific #includes? once OPPs are active for OMAP1,2
(similar to what Paul has posted), this list will be a lot of #ifdeffery.
> +
> +enum opp_t {
_t is confusing(vs typedefs) for many folks maybe we could have a rename
of this to say opp_type?
> + OPP_NONE,
why is OPP_NONE required?
> + OPP_MPU,
> + OPP_L3,
> + OPP_DSP
you can have OPP_NUM_TYPES here and save on a #define. but u'd have to
move the enum under #ifdef for OMAP3 which seems more appropriate.
in theory, in future for other silicons, as enums get added,
OPP_NUM_TYPES will auto increment preventing buggy
modification/forgetting to update the #define.
> +};
>
> struct omap_opp;
>
> @@ -40,16 +49,16 @@ unsigned long opp_get_freq(const struct omap_opp
*opp);
> /**
> * opp_get_opp_count() - Get number of opps enabled in the opp list
> * @num: returns the number of opps
> - * @oppl: opp list
> + * @opp_t: OPP type we want to count
> *
> * This functions returns the number of opps if there are any OPPs
enabled,
> * else returns corresponding error value.
> */
> -int opp_get_opp_count(struct omap_opp *oppl);
> +int opp_get_opp_count(enum opp_t opp_t);
Re: enum type and variable have the same name :( mebbe a rename of
variable is appropriate
>
> /**
> * 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
> *
> @@ -61,14 +70,14 @@ int opp_get_opp_count(struct omap_opp *oppl);
> * for exact matching frequency and is enabled. if true, the match
is for exact
> * frequency which is disabled.
> */
> -struct omap_opp *opp_find_freq_exact(struct omap_opp *oppl,
> +struct omap_opp *opp_find_freq_exact(enum opp_t opp_t,
Re: enum type and variable have the same name :( mebbe a rename of
variable is appropriate
> 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
> @@ -97,14 +106,13 @@ struct omap_opp *opp_find_freq_exact(struct
omap_opp *oppl,
> * NOTE: if we set freq as ULONG_MAX and search low, we get the
> * highest enabled frequency
> */
> -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);
Re: enum type and variable have the same name :( mebbe a rename of
variable is appropriate
>
> /* 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
> @@ -130,7 +138,7 @@ struct omap_opp *opp_find_freq_floor(struct
omap_opp *oppl,
> * freq++; * for next higher match *
> * }
> */
> -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);
Re: enum type and variable have the same name :( mebbe a rename of
variable is appropriate
>
>
> /**
> @@ -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.
> + * This function creates a list of opp definitions and returns status.
> * This list can be used to further validation/search/modifications. New
> * opp entries can be added to this list by using opp_add().
> *
> - * In the case of error, ERR_PTR is returned to the caller and should be
> - * appropriately handled with IS_ERR.
> + * In the case of error, suitable error code is returned.
> */
> -struct omap_opp __init *opp_init_list(const struct omap_opp_def
*opp_defs);
> +int __init opp_init_list(enum opp_t opp_t,
Re: enum type and variable have the same name :( mebbe a rename of
variable is appropriate
> + 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
> @@ -224,11 +224,11 @@ int opp_enable(struct omap_opp *opp);
> */
> int opp_disable(struct omap_opp *opp);
>
> -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,
Re: enum type and variable have the same name :( mebbe a rename of
variable is appropriate
> u8 opp_id);
> u8 __deprecated opp_get_opp_id(struct omap_opp *opp);
>
> -void opp_init_cpufreq_table(struct omap_opp *opps,
> +void opp_init_cpufreq_table(enum opp_t opp_t,
Re: enum type and variable have the same name :( mebbe a rename of
variable is appropriate
> struct cpufreq_frequency_table **table);
>
>
> diff --git a/arch/arm/plat-omap/omap-pm-noop.c
b/arch/arm/plat-omap/omap-pm-noop.c
> index aeb372e..713a59f 100644
> --- a/arch/arm/plat-omap/omap-pm-noop.c
> +++ b/arch/arm/plat-omap/omap-pm-noop.c
> @@ -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)
> */
> diff --git a/arch/arm/plat-omap/omap-pm-srf.c
b/arch/arm/plat-omap/omap-pm-srf.c
> index f7bf353..9b4cf7f 100644
> --- a/arch/arm/plat-omap/omap-pm-srf.c
> +++ b/arch/arm/plat-omap/omap-pm-srf.c
> @@ -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
>
> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
> index 4fe1933..aa21fc5 100644
> --- a/arch/arm/plat-omap/opp.c
> +++ b/arch/arm/plat-omap/opp.c
> @@ -38,6 +38,8 @@ struct omap_opp {
> u8 opp_id;
> };
>
> +
> +static struct omap_opp *_opp_list[OPP_TYPES];
probably should be a dynamic array, instead of depending on predefined -
init_list to increase the pointer array as needed
and number of registered list should ideally be stored as a local static
variable? that way, we could do away with OPP_TYPES altogether.
> /*
> * 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)
> {
> u8 n = 0;
> + struct omap_opp *oppl;
>
> - if (unlikely(!oppl || IS_ERR(oppl))) {
> + if (unlikely((opp_t == OPP_NONE) || opp_t > OPP_TYPES)) {
> pr_err("%s: Invalid parameters being passed\n",
__func__);
> return -EINVAL;
> }
> +
> + oppl = _opp_list[opp_t - 1];
re: by removing OPP_NONE, we can save on the -1. this true throughout
the patch.
> 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;
>
>
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-12 12:39 [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types Romit Dasgupta
2010-01-12 17:19 ` Nishanth Menon
@ 2010-01-12 17:19 ` Kevin Hilman
2010-01-12 17:36 ` Cousson, Benoit
2010-01-13 10:31 ` Romit Dasgupta
2010-01-12 17:57 ` Menon, Nishanth
2 siblings, 2 replies; 14+ messages in thread
From: Kevin Hilman @ 2010-01-12 17:19 UTC (permalink / raw)
To: romit; +Cc: nm, linux-omap
Romit Dasgupta <romit@ti.com> writes:
> 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 <romit@ti.com>
I like this idea... but I have some questions about how we should
cleanly handle SMP and future SoCs.
> diff --git a/arch/arm/plat-omap/include/plat/opp.h b/arch/arm/plat-omap/include/plat/opp.h
> index 9f91ad3..c4d5bf9 100644
> --- a/arch/arm/plat-omap/include/plat/opp.h
> +++ b/arch/arm/plat-omap/include/plat/opp.h
> @@ -13,9 +13,18 @@
> #ifndef __ASM_ARM_OMAP_OPP_H
> #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...
> +enum opp_t {
> + OPP_NONE,
> + OPP_MPU,
> + OPP_L3,
> + OPP_DSP
add OPP_MAX_TYPES here
> +};
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-12 17:19 ` Kevin Hilman
@ 2010-01-12 17:36 ` Cousson, Benoit
2010-01-12 19:26 ` Kevin Hilman
2010-01-13 10:31 ` Romit Dasgupta
1 sibling, 1 reply; 14+ messages in thread
From: Cousson, Benoit @ 2010-01-12 17:36 UTC (permalink / raw)
To: Kevin Hilman, Dasgupta, Romit; +Cc: Menon, Nishanth, linux-omap
Hi Kevin,
>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>owner@vger.kernel.org] On Behalf Of Kevin Hilman
>Sent: Tuesday, January 12, 2010 6:19 PM
>To: Dasgupta, Romit
>Cc: Menon, Nishanth; linux-omap@vger.kernel.org
>Subject: Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing
>different OPP types
>
>Romit Dasgupta <romit@ti.com> writes:
>
>> 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 <romit@ti.com>
>
>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.
Regards,
Benoit
>> diff --git a/arch/arm/plat-omap/include/plat/opp.h b/arch/arm/plat-
>omap/include/plat/opp.h
>> index 9f91ad3..c4d5bf9 100644
>> --- a/arch/arm/plat-omap/include/plat/opp.h
>> +++ b/arch/arm/plat-omap/include/plat/opp.h
>> @@ -13,9 +13,18 @@
>> #ifndef __ASM_ARM_OMAP_OPP_H
>> #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...
>> +enum opp_t {
>> + OPP_NONE,
>> + OPP_MPU,
>> + OPP_L3,
>> + OPP_DSP
>
>add OPP_MAX_TYPES here
>
>> +};
>
>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 majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
Texas Instruments France SA, 821 Avenue Jack Kilby, 06270 Villeneuve Loubet. 036 420 040 R.C.S Antibes. Capital de EUR 753.920
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-12 12:39 [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types Romit Dasgupta
2010-01-12 17:19 ` Nishanth Menon
2010-01-12 17:19 ` Kevin Hilman
@ 2010-01-12 17:57 ` Menon, Nishanth
2010-01-13 10:41 ` Romit Dasgupta
2 siblings, 1 reply; 14+ messages in thread
From: Menon, Nishanth @ 2010-01-12 17:57 UTC (permalink / raw)
To: Dasgupta, Romit, khilman; +Cc: linux-omap
Apologies on the spam - try two with outlook instead of thunderbird to
prevent horrible linewraps :(
/me learns never to post back into a reply on thunderbird from gedit..
Dasgupta, Romit had written, on 01/12/2010 06:39 AM, the following:
> 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.
Style comment: an additional EOL here will be nice between commit message
and signed-off-by.
General comment: might be good to state the enum types you are introducing
for OMAP3 in the commit message
> Signed-off-by: Romit Dasgupta <romit@ti.com>
> ---
>
link to previous discussions here might help reviewers esp to give context
for the design choice.
> 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(-)
>
>
> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-omap2/pm34xx.c
> index 5c751c1..8c73e0e 100644
> --- a/arch/arm/mach-omap2/pm34xx.c
> +++ b/arch/arm/mach-omap2/pm34xx.c
> @@ -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
> - };
>
> omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
> omap34xx_opp_def_list;
> - 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.
> /* We dont want half configured system at the moment */
> - BUG_ON(IS_ERR(omap3_rate_tables[i]));
> + BUG_ON(ret);
> }
> }
>
> diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c
> index 157b38e..38c44ee 100644
> --- a/arch/arm/mach-omap2/resource34xx.c
> +++ b/arch/arm/mach-omap2/resource34xx.c
> @@ -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);
> *
> * 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
> + u8 opp_id)
> {
> struct omap_opp *opp;
>
> - BUG_ON(!freq || !opps);
> + BUG_ON(!freq || opp_t == OPP_NONE || opp_t > OPP_TYPES);
why OPP_NONE?
>
> - 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
> - * @opps: opp list
> + * @opp_t: OPP type where we need to look.
> * @freq: frequency we are searching for
> *
> * return 0 and opp_id is populated if we find the freq, else, we return error
> *
> * NOTE: this function is a standin for the timebeing as opp_id is deprecated
> */
> -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
> 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;
>
> - 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.
> /* 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))
> @@ -329,7 +326,7 @@ static int program_opp_freq(int res, int target_level, int current_level)
> return target_level;
> }
>
> -static int program_opp(int res, struct omap_opp *opp, int target_level,
> +static int program_opp(int res, enum opp_t opp_t, int target_level,
Re: enum type and variable have the same name :( mebbe a rename of variable is
appropriate
> 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;
> }
>
> @@ -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.
See commit: 0fea42354997641652623a7b046c14d580fca80c
OMAP3 SRF: Fix crash on non-3430SDP platforms with DVFS/CPUFreq
The SRF/DVFS + CPUFreq patches had issues booting on
non-3430SDP platforms as reported by Kevin Hilman.
This patch puts non-NULL checks in place for mpu_opps/dsp_opps
/l3_opps before accessing them and fixes the issue.
Signed-off-by: Rajendra Nayak <rnayak@ti.com>
> 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;
> }
> diff --git a/arch/arm/mach-omap2/smartreflex.c b/arch/arm/mach-omap2/smartreflex.c
> index 3c37837..1c5ec37 100644
> --- a/arch/arm/mach-omap2/smartreflex.c
> +++ b/arch/arm/mach-omap2/smartreflex.c
> @@ -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;
> }
> @@ -1010,12 +1003,6 @@ static int __init omap3_sr_init(void)
> int ret = 0;
> u8 RdReg;
>
> - /* Exit if OPP tables are not defined */
> - if (!(mpu_opps && l3_opps)) {
> - pr_err("SR: OPP rate tables not defined for platform, not enabling SmartReflex\n");
> - return -ENODEV;
> - }
> -
re: the same intent of handling un-initialized tables, but we loose it at this
point..
> /* Enable SR on T2 */
> ret = twl_i2c_read_u8(TWL4030_MODULE_PM_RECEIVER, &RdReg,
> R_DCDC_GLOBAL_CFG);
> diff --git a/arch/arm/plat-omap/common.c b/arch/arm/plat-omap/common.c
> index 6bd62ea..dddc027 100644
> --- a/arch/arm/plat-omap/common.c
> +++ b/arch/arm/plat-omap/common.c
> @@ -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;
> diff --git a/arch/arm/plat-omap/cpu-omap.c b/arch/arm/plat-omap/cpu-omap.c
> index 109203e..a69b557 100644
> --- a/arch/arm/plat-omap/cpu-omap.c
> +++ b/arch/arm/plat-omap/cpu-omap.c
> @@ -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);
> diff --git a/arch/arm/plat-omap/include/plat/opp.h b/arch/arm/plat-omap/include/plat/opp.h
> index 9f91ad3..c4d5bf9 100644
> --- a/arch/arm/plat-omap/include/plat/opp.h
> +++ b/arch/arm/plat-omap/include/plat/opp.h
> @@ -13,9 +13,18 @@
> #ifndef __ASM_ARM_OMAP_OPP_H
> #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
Not a strong requirement, but does it make sense to have these to be part of
silicon specific #includes? once OPPs are active for OMAP1,2 (similar to what
Paul has posted), this list will be a lot of #ifdeffery.
> +
> +enum opp_t {
_t is confusing(vs typedefs) for many folks maybe we could have a rename of
this to say opp_type?
> + OPP_NONE,
why is OPP_NONE required?
> + OPP_MPU,
> + OPP_L3,
> + OPP_DSP
you can have OPP_NUM_TYPES here and save on a #define. but u'd have to move the
enum under #ifdef for OMAP3 which seems more appropriate.
in theory, in future for other silicons, as enums get added, OPP_NUM_TYPES will
auto increment preventing buggy modification/forgetting to update the #define.
> +};
>
> struct omap_opp;
>
> @@ -40,16 +49,16 @@ unsigned long opp_get_freq(const struct omap_opp *opp);
> /**
> * opp_get_opp_count() - Get number of opps enabled in the opp list
> * @num: returns the number of opps
> - * @oppl: opp list
> + * @opp_t: OPP type we want to count
> *
> * This functions returns the number of opps if there are any OPPs enabled,
> * else returns corresponding error value.
> */
> -int opp_get_opp_count(struct omap_opp *oppl);
> +int opp_get_opp_count(enum opp_t opp_t);
Re: enum type and variable have the same name :( mebbe a rename of variable
is appropriate
>
> /**
> * 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
> *
> @@ -61,14 +70,14 @@ int opp_get_opp_count(struct omap_opp *oppl);
> * for exact matching frequency and is enabled. if true, the match is for exact
> * frequency which is disabled.
> */
> -struct omap_opp *opp_find_freq_exact(struct omap_opp *oppl,
> +struct omap_opp *opp_find_freq_exact(enum opp_t opp_t,
Re: enum type and variable have the same name :( mebbe a rename of variable is
appropriate
> 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
> @@ -97,14 +106,13 @@ struct omap_opp *opp_find_freq_exact(struct omap_opp *oppl,
> * NOTE: if we set freq as ULONG_MAX and search low, we get the
> * highest enabled frequency
> */
> -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);
Re: enum type and variable have the same name :( mebbe a rename of variable is
appropriate
>
> /* 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
> @@ -130,7 +138,7 @@ struct omap_opp *opp_find_freq_floor(struct omap_opp *oppl,
> * freq++; * for next higher match *
> * }
> */
> -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);
Re: enum type and variable have the same name :( mebbe a rename of variable is
appropriate
>
>
> /**
> @@ -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.
> + * This function creates a list of opp definitions and returns status.
> * This list can be used to further validation/search/modifications. New
> * opp entries can be added to this list by using opp_add().
> *
> - * In the case of error, ERR_PTR is returned to the caller and should be
> - * appropriately handled with IS_ERR.
> + * In the case of error, suitable error code is returned.
> */
> -struct omap_opp __init *opp_init_list(const struct omap_opp_def *opp_defs);
> +int __init opp_init_list(enum opp_t opp_t,
Re: enum type and variable have the same name :( mebbe a rename of variable is
appropriate
> + 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
> @@ -224,11 +224,11 @@ int opp_enable(struct omap_opp *opp);
> */
> int opp_disable(struct omap_opp *opp);
>
> -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,
Re: enum type and variable have the same name :( mebbe a rename of variable is
appropriate
> u8 opp_id);
> u8 __deprecated opp_get_opp_id(struct omap_opp *opp);
>
> -void opp_init_cpufreq_table(struct omap_opp *opps,
> +void opp_init_cpufreq_table(enum opp_t opp_t,
Re: enum type and variable have the same name :( mebbe a rename of variable is
appropriate
> struct cpufreq_frequency_table **table);
>
>
> diff --git a/arch/arm/plat-omap/omap-pm-noop.c b/arch/arm/plat-omap/omap-pm-noop.c
> index aeb372e..713a59f 100644
> --- a/arch/arm/plat-omap/omap-pm-noop.c
> +++ b/arch/arm/plat-omap/omap-pm-noop.c
> @@ -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)
> */
> diff --git a/arch/arm/plat-omap/omap-pm-srf.c b/arch/arm/plat-omap/omap-pm-srf.c
> index f7bf353..9b4cf7f 100644
> --- a/arch/arm/plat-omap/omap-pm-srf.c
> +++ b/arch/arm/plat-omap/omap-pm-srf.c
> @@ -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
>
> diff --git a/arch/arm/plat-omap/opp.c b/arch/arm/plat-omap/opp.c
> index 4fe1933..aa21fc5 100644
> --- a/arch/arm/plat-omap/opp.c
> +++ b/arch/arm/plat-omap/opp.c
> @@ -38,6 +38,8 @@ struct omap_opp {
> u8 opp_id;
> };
>
> +
> +static struct omap_opp *_opp_list[OPP_TYPES];
probably should be a dynamic array, instead of depending on predefined -
init_list to increase the pointer array as needed and number of registered list
should ideally be stored as a local static variable? that way, we could do away
with OPP_TYPES altogether.
> /*
> * 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)
> {
> u8 n = 0;
> + struct omap_opp *oppl;
>
> - if (unlikely(!oppl || IS_ERR(oppl))) {
> + if (unlikely((opp_t == OPP_NONE) || opp_t > OPP_TYPES)) {
> pr_err("%s: Invalid parameters being passed\n", __func__);
> return -EINVAL;
> }
> +
> + oppl = _opp_list[opp_t - 1];
re: by removing OPP_NONE, we can save on the -1. this true throughout the patch.
> 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;
>
>
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-12 17:36 ` Cousson, Benoit
@ 2010-01-12 19:26 ` Kevin Hilman
0 siblings, 0 replies; 14+ messages in thread
From: Kevin Hilman @ 2010-01-12 19:26 UTC (permalink / raw)
To: Cousson, Benoit; +Cc: Dasgupta, Romit, Menon, Nishanth, linux-omap
"Cousson, Benoit" <b-cousson@ti.com> writes:
> Hi Kevin,
>
>>From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>>owner@vger.kernel.org] On Behalf Of Kevin Hilman
>>Sent: Tuesday, January 12, 2010 6:19 PM
>>To: Dasgupta, Romit
>>Cc: Menon, Nishanth; linux-omap@vger.kernel.org
>>Subject: Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing
>>different OPP types
>>
>>Romit Dasgupta <romit@ti.com> writes:
>>
>>> 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 <romit@ti.com>
>>
>>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.
OK. Let's take this approach for now, and after hwmods get
stabilized/merged we can consider alternate per-SoC ways to handle
this.
Thanks,
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-12 17:19 ` Kevin Hilman
2010-01-12 17:36 ` Cousson, Benoit
@ 2010-01-13 10:31 ` Romit Dasgupta
1 sibling, 0 replies; 14+ messages in thread
From: Romit Dasgupta @ 2010-01-13 10:31 UTC (permalink / raw)
To: Kevin Hilman; +Cc: Menon, Nishanth, linux-omap
>
> I like this idea... but I have some questions about how we should
> cleanly handle SMP and future SoCs.
Actually for OMAP4 AFAICT MPU's share only one set of OPPs (their clocks are
tied). So it should not be a problem there. OTOH, the OPP layer itself is not
thread safe today(I tried to make it thread + SMP safe in the version of the
code I posted earlier). You can look into opp_add and it shows that it is not
thread safe.
If you are talking about future SOCs we need to define the OPP list for the new
SoCs. The exact mechanism can be clear once we have the SRF replacement.
>
>> diff --git a/arch/arm/plat-omap/include/plat/opp.h b/arch/arm/plat-omap/include/plat/opp.h
>> index 9f91ad3..c4d5bf9 100644
>> --- a/arch/arm/plat-omap/include/plat/opp.h
>> +++ b/arch/arm/plat-omap/include/plat/opp.h
>> @@ -13,9 +13,18 @@
>> #ifndef __ASM_ARM_OMAP_OPP_H
>> #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...
>> +enum opp_t {
>> + OPP_NONE,
>> + OPP_MPU,
>> + OPP_L3,
>> + OPP_DSP
>
> add OPP_MAX_TYPES here
>
Yes you are right. I will change the code and re-post.
>> +};
>
Thanks,
-Romit
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-12 17:57 ` Menon, Nishanth
@ 2010-01-13 10:41 ` Romit Dasgupta
2010-01-13 12:54 ` Nishanth Menon
2010-01-13 14:43 ` Kevin Hilman
0 siblings, 2 replies; 14+ messages in thread
From: Romit Dasgupta @ 2010-01-13 10:41 UTC (permalink / raw)
To: Menon, Nishanth; +Cc: khilman, linux-omap
Menon, Nishanth wrote:
>
> 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.
>
>> Signed-off-by: Romit Dasgupta <romit@ti.com>
>> ---
>>
>> omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
>> omap34xx_opp_def_list;
>> - 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)
Frankly, I did not want to introduce OPP_NONE but did so as you are checking all
parameters passed to the OPP APIs.
> 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.
>
>> /* We dont want half configured system at the moment */
>> - BUG_ON(IS_ERR(omap3_rate_tables[i]));
>> + BUG_ON(ret);
>> }
>> }
>>
>> diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c
>> index 157b38e..38c44ee 100644
>> --- a/arch/arm/mach-omap2/resource34xx.c
>> +++ b/arch/arm/mach-omap2/resource34xx.c
>> @@ -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);
>> *
>> * 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?
>> @@ -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
>> 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;
>>
>> - 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.
>> @@ -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.
Same comment as before.
Thanks,
-Romit
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-13 10:41 ` Romit Dasgupta
@ 2010-01-13 12:54 ` Nishanth Menon
2010-01-13 13:22 ` Romit Dasgupta
2010-01-13 14:43 ` Kevin Hilman
1 sibling, 1 reply; 14+ messages in thread
From: Nishanth Menon @ 2010-01-13 12:54 UTC (permalink / raw)
To: Romit Dasgupta; +Cc: khilman, linux-omap
Romit Dasgupta said the following on 01/13/2010 04:41 AM:
> Menon, Nishanth wrote:
>
>> 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?
>>> Signed-off-by: Romit Dasgupta <romit@ti.com>
>>> ---
>>>
>>> omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
>>> omap34xx_opp_def_list;
>>> - 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)
>>
> 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.
>
>> 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.
>
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.
>>> /* We dont want half configured system at the moment */
>>> - BUG_ON(IS_ERR(omap3_rate_tables[i]));
>>> + BUG_ON(ret);
>>> }
>>> }
>>>
>>> diff --git a/arch/arm/mach-omap2/resource34xx.c b/arch/arm/mach-omap2/resource34xx.c
>>> index 157b38e..38c44ee 100644
>>> --- a/arch/arm/mach-omap2/resource34xx.c
>>> +++ b/arch/arm/mach-omap2/resource34xx.c
>>> @@ -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);
>>> *
>>> * 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?
>
The usage later in the code is opp_t -> this is a readability issue not
a compiler warning.
>>> @@ -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
>>
>>> 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;
>>>
>>> - 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.
>
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.
[...]
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-13 12:54 ` Nishanth Menon
@ 2010-01-13 13:22 ` Romit Dasgupta
2010-01-15 10:35 ` Nishanth Menon
0 siblings, 1 reply; 14+ messages in thread
From: Romit Dasgupta @ 2010-01-13 13:22 UTC (permalink / raw)
To: Nishanth Menon; +Cc: khilman, linux-omap
Nishanth Menon wrote:
> Romit Dasgupta said the following on 01/13/2010 04:41 AM:
>> Menon, Nishanth wrote:
>>
>>> 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.
>
>>>> Signed-off-by: Romit Dasgupta <romit@ti.com>
>>>> ---
>>>>
>>>> omap3_opp_def_list = cpu_is_omap3630() ? omap36xx_opp_def_list :
>>>> omap34xx_opp_def_list;
>>>> - 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)
>>>
>> 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
>>
>>
> 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.
>
> 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?
>>>> */
>>>> -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?
>>
> 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?
>>> 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?
omap3_pm_init_opp_table
>
> 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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-13 10:41 ` Romit Dasgupta
2010-01-13 12:54 ` Nishanth Menon
@ 2010-01-13 14:43 ` Kevin Hilman
1 sibling, 0 replies; 14+ messages in thread
From: Kevin Hilman @ 2010-01-13 14:43 UTC (permalink / raw)
To: Romit Dasgupta; +Cc: Menon, Nishanth, linux-omap
Romit Dasgupta <romit@ti.com> writes:
> Menon, Nishanth wrote:
>>
[...]
>>> -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
>>> 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;
>>>
>>> - 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.
Please do not code things based on the the existence of BUG_ON().
All of these BUG* points need to be removed as we none of these are fatal. We need
better error recovery, most likely using WARN* and returning error codes.
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-13 13:22 ` Romit Dasgupta
@ 2010-01-15 10:35 ` Nishanth Menon
2010-01-15 10:42 ` Romit Dasgupta
0 siblings, 1 reply; 14+ messages in thread
From: Nishanth Menon @ 2010-01-15 10:35 UTC (permalink / raw)
To: Romit Dasgupta; +Cc: Nishanth Menon, khilman, linux-omap
Romit Dasgupta had written, on 01/13/2010 07:22 AM, the following:
Apologies on a delayed response, few other topics required urgent
attention meanwhile.
> Nishanth Menon wrote:
>> Romit Dasgupta said the following on 01/13/2010 04:41 AM:
>>> Menon, Nishanth wrote:
>>>
>>>> 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.
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.
---
Benefit of this is that someone who may not monitoring linux-omap very
closely and is not aware of this change (e.g. private trees and
potential users such as dspbridge) can figure out from the git log which
commit might potentially affect them. The patch does SOC specific
change, so the commit message ought to catch their attention. It also
allows maintainers(later in the pipeline) who maynot know about this
specific discussion (btw, a link might be useful as mentioned
previously), to understand the scope of the change accurately.
Please note I have divided the commit message into three parts:
i) why is the old strategy not effection (motivation for the patch)
ii) why is the new strategy beneficial (benefit of the patch)
iii) what is the impact of the patch (warning for historical
purposes/informative).
[...]
>>>>> + 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
Thanks.
>>>
>>>
>> 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.
>> 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.
Instead I would request at least (b) be done to prevent mistaken
modifications.
>>>>> */
>>>>> -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?
>>>
>> 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
seeing opp_t.xyz the reader is confused and has to search for what opp_t
really is.
Now if it was point 2, it might have been easier for the reader at the
first look.
I know we cannot write code idiot friendly, but we dont need to make it
obfuscated either.. renaming opp_t to *oppt or enum opp_t to opp_type
might be a little verbose, but it helps for some poor bloke later on..
>
>>>> 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?
>
> omap3_pm_init_opp_table
>> 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.
the test there was checking if the opp tables were initialized - at that
time those variables were the only way to do it. now you have replaced
those variables with enums, I think I understand your point about
redundancy of those checks, but that will be fixing a different problem
altogether and breaks the atomicity of this patch. apologies on being a
nitpick, but it does indeed help a git bisect if something goes wrong in
the future due to the missing of the check and something we did not see
today.
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-15 10:35 ` Nishanth Menon
@ 2010-01-15 10:42 ` Romit Dasgupta
2010-01-15 10:56 ` Nishanth Menon
0 siblings, 1 reply; 14+ messages in thread
From: Romit Dasgupta @ 2010-01-15 10:42 UTC (permalink / raw)
To: Menon, Nishanth; +Cc: Nishanth Menon, khilman, linux-omap
Nishanth,
I was about to post a re-worked patch. Anyways, please see below:
> 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.
I like this message. I will include it.
>>>>
>>>>
>>> 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.
>>> 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.
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).
>>>>> 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
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.
Regards,
-Romit
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
2010-01-15 10:42 ` Romit Dasgupta
@ 2010-01-15 10:56 ` Nishanth Menon
0 siblings, 0 replies; 14+ messages in thread
From: Nishanth Menon @ 2010-01-15 10:56 UTC (permalink / raw)
To: Dasgupta, Romit; +Cc: Nishanth Menon, khilman, linux-omap
Dasgupta, Romit had written, on 01/15/2010 04:42 AM, the following:
[..]
> I like this message. I will include it.
:) I am getting this wrong feel that I should start writing novels ;).
[...]
>> 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).
Thanks..
>>>>>> 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
>
> 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.
Having spent a few years on properitory s/w and been one of those
confused blokes(I being a certified nitwit), I will try to see if I can
provide a patch on top and Kevin/community can choose to add their
comments/club the patches and make the few folks like me happy.
--
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2010-01-15 10:56 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-01-12 12:39 [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types Romit Dasgupta
2010-01-12 17:19 ` Nishanth Menon
2010-01-12 17:19 ` Kevin Hilman
2010-01-12 17:36 ` Cousson, Benoit
2010-01-12 19:26 ` Kevin Hilman
2010-01-13 10:31 ` Romit Dasgupta
2010-01-12 17:57 ` Menon, Nishanth
2010-01-13 10:41 ` Romit Dasgupta
2010-01-13 12:54 ` Nishanth Menon
2010-01-13 13:22 ` Romit Dasgupta
2010-01-15 10:35 ` Nishanth Menon
2010-01-15 10:42 ` Romit Dasgupta
2010-01-15 10:56 ` Nishanth Menon
2010-01-13 14:43 ` Kevin Hilman
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.