From: Nishanth Menon <nm@ti.com>
To: "Dasgupta, Romit" <romit@ti.com>
Cc: "khilman@deeprootsystems.com" <khilman@deeprootsystems.com>,
"linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>
Subject: Re: [PM-WIP-OPP][PATCH] OPP: Introduces enum for addressing different OPP types
Date: Tue, 12 Jan 2010 11:19:01 -0600 [thread overview]
Message-ID: <4B4CAF05.2060001@ti.com> (raw)
In-Reply-To: <1263299979.1536.7.camel@boson>
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
next prev parent reply other threads:[~2010-01-12 17:19 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4B4CAF05.2060001@ti.com \
--to=nm@ti.com \
--cc=khilman@deeprootsystems.com \
--cc=linux-omap@vger.kernel.org \
--cc=romit@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox