* [PM-WIP_CPUFREQ][PATCH V3 1/8] OMAP2+: cpufreq: move clk name decision to init
2011-05-25 23:38 [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Nishanth Menon
@ 2011-05-25 23:38 ` Nishanth Menon
2011-05-26 17:33 ` Kevin Hilman
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 2/8] OMAP2+: cpufreq: deny initialization if no mpudev Nishanth Menon
` (7 subsequent siblings)
8 siblings, 1 reply; 37+ messages in thread
From: Nishanth Menon @ 2011-05-25 23:38 UTC (permalink / raw)
To: linux-omap; +Cc: Kevin, Nishanth Menon
Clk name does'nt need to dynamically detected during clk init.
move them off to driver initialization, if we dont have a clk name,
there is no point in registering the driver anyways. The actual clk
get and put is left at cpu_init and exit functions.
Signed-off-by: Nishanth Menon <nm@ti.com>
---
arch/arm/mach-omap2/omap2plus-cpufreq.c | 20 +++++++++++++-------
1 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index d53ce23..a57b322 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -42,6 +42,7 @@
static struct cpufreq_frequency_table *freq_table;
static struct clk *mpu_clk;
+static char *mpu_clk_name;
static int omap_verify_speed(struct cpufreq_policy *policy)
{
@@ -157,13 +158,7 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
struct device *mpu_dev;
static cpumask_var_t cpumask;
- if (cpu_is_omap24xx())
- mpu_clk = clk_get(NULL, "virt_prcm_set");
- else if (cpu_is_omap34xx())
- mpu_clk = clk_get(NULL, "dpll1_ck");
- else if (cpu_is_omap44xx())
- mpu_clk = clk_get(NULL, "dpll_mpu_ck");
-
+ mpu_clk = clk_get(NULL, mpu_clk_name);
if (IS_ERR(mpu_clk))
return PTR_ERR(mpu_clk);
@@ -238,6 +233,17 @@ static struct cpufreq_driver omap_driver = {
static int __init omap_cpufreq_init(void)
{
+ if (cpu_is_omap24xx())
+ mpu_clk_name = "virt_prcm_set";
+ else if (cpu_is_omap34xx())
+ mpu_clk_name = "dpll1_ck";
+ else if (cpu_is_omap44xx())
+ mpu_clk_name = "dpll_mpu_ck";
+
+ if (!mpu_clk_name) {
+ pr_err("%s: unsupported Silicon?\n", __func__);
+ return -EINVAL;
+ }
return cpufreq_register_driver(&omap_driver);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PM-WIP_CPUFREQ][PATCH V3 2/8] OMAP2+: cpufreq: deny initialization if no mpudev
2011-05-25 23:38 [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Nishanth Menon
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 1/8] OMAP2+: cpufreq: move clk name decision to init Nishanth Menon
@ 2011-05-25 23:38 ` Nishanth Menon
2011-05-26 17:34 ` Kevin Hilman
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 3/8] OMAP2+: cpufreq: use opp/clk_*cpufreq_table based on silicon Nishanth Menon
` (6 subsequent siblings)
8 siblings, 1 reply; 37+ messages in thread
From: Nishanth Menon @ 2011-05-25 23:38 UTC (permalink / raw)
To: linux-omap; +Cc: Kevin, Nishanth Menon
if we do not have mpu_dev we normally fail in cpu_init. It is better
to fail driver registration if the devices are not available.
Signed-off-by: Nishanth Menon <nm@ti.com>
---
arch/arm/mach-omap2/omap2plus-cpufreq.c | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index a57b322..2d4e9d7 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -43,6 +43,7 @@
static struct cpufreq_frequency_table *freq_table;
static struct clk *mpu_clk;
static char *mpu_clk_name;
+static struct device *mpu_dev;
static int omap_verify_speed(struct cpufreq_policy *policy)
{
@@ -155,7 +156,6 @@ skip_lpj:
static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
{
int result = 0;
- struct device *mpu_dev;
static cpumask_var_t cpumask;
mpu_clk = clk_get(NULL, mpu_clk_name);
@@ -166,12 +166,6 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
return -EINVAL;
policy->cur = policy->min = policy->max = omap_getspeed(policy->cpu);
- mpu_dev = omap2_get_mpuss_device();
-
- if (!mpu_dev) {
- pr_warning("%s: unable to get the mpu device\n", __func__);
- return -EINVAL;
- }
opp_init_cpufreq_table(mpu_dev, &freq_table);
if (freq_table) {
@@ -244,6 +238,13 @@ static int __init omap_cpufreq_init(void)
pr_err("%s: unsupported Silicon?\n", __func__);
return -EINVAL;
}
+
+ mpu_dev = omap2_get_mpuss_device();
+ if (!mpu_dev) {
+ pr_warning("%s: unable to get the mpu device\n", __func__);
+ return -EINVAL;
+ }
+
return cpufreq_register_driver(&omap_driver);
}
--
1.7.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PM-WIP_CPUFREQ][PATCH V3 3/8] OMAP2+: cpufreq: use opp/clk_*cpufreq_table based on silicon
2011-05-25 23:38 [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Nishanth Menon
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 1/8] OMAP2+: cpufreq: move clk name decision to init Nishanth Menon
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 2/8] OMAP2+: cpufreq: deny initialization if no mpudev Nishanth Menon
@ 2011-05-25 23:38 ` Nishanth Menon
2011-05-26 17:38 ` Kevin Hilman
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 4/8] OMAP2+: cpufreq: dont support !freq_table Nishanth Menon
` (5 subsequent siblings)
8 siblings, 1 reply; 37+ messages in thread
From: Nishanth Menon @ 2011-05-25 23:38 UTC (permalink / raw)
To: linux-omap; +Cc: Kevin, Nishanth Menon
OMAP2 is the only family using clk_[init|exit]_cpufreq_table, while
OMAP3+ use OPP table to generate and release the cpufreq tables.
Hence use a flag to mark which API to use for allocating and freeing
the tables.
Signed-off-by: Nishanth Menon <nm@ti.com>
---
arch/arm/mach-omap2/omap2plus-cpufreq.c | 20 +++++++++++++++-----
1 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 2d4e9d7..dbbf8b2 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -44,6 +44,7 @@ static struct cpufreq_frequency_table *freq_table;
static struct clk *mpu_clk;
static char *mpu_clk_name;
static struct device *mpu_dev;
+static bool use_opp;
static int omap_verify_speed(struct cpufreq_policy *policy)
{
@@ -166,7 +167,10 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
return -EINVAL;
policy->cur = policy->min = policy->max = omap_getspeed(policy->cpu);
- opp_init_cpufreq_table(mpu_dev, &freq_table);
+ if (use_opp)
+ opp_init_cpufreq_table(mpu_dev, &freq_table);
+ else
+ clk_init_cpufreq_table(&freq_table);
if (freq_table) {
result = cpufreq_frequency_table_cpuinfo(policy, freq_table);
@@ -204,7 +208,10 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
static int omap_cpu_exit(struct cpufreq_policy *policy)
{
- clk_exit_cpufreq_table(&freq_table);
+ if (use_opp)
+ opp_free_cpufreq_table(mpu_dev, &freq_table);
+ else
+ clk_exit_cpufreq_table(&freq_table);
clk_put(mpu_clk);
return 0;
}
@@ -227,12 +234,15 @@ static struct cpufreq_driver omap_driver = {
static int __init omap_cpufreq_init(void)
{
- if (cpu_is_omap24xx())
+ use_opp = true;
+ if (cpu_is_omap24xx()) {
mpu_clk_name = "virt_prcm_set";
- else if (cpu_is_omap34xx())
+ use_opp = false;
+ } else if (cpu_is_omap34xx()) {
mpu_clk_name = "dpll1_ck";
- else if (cpu_is_omap44xx())
+ } else if (cpu_is_omap44xx()) {
mpu_clk_name = "dpll_mpu_ck";
+ }
if (!mpu_clk_name) {
pr_err("%s: unsupported Silicon?\n", __func__);
--
1.7.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 3/8] OMAP2+: cpufreq: use opp/clk_*cpufreq_table based on silicon
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 3/8] OMAP2+: cpufreq: use opp/clk_*cpufreq_table based on silicon Nishanth Menon
@ 2011-05-26 17:38 ` Kevin Hilman
2011-05-26 18:35 ` Menon, Nishanth
0 siblings, 1 reply; 37+ messages in thread
From: Kevin Hilman @ 2011-05-26 17:38 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap
Nishanth Menon <nm@ti.com> writes:
> OMAP2 is the only family using clk_[init|exit]_cpufreq_table, while
> OMAP3+ use OPP table to generate and release the cpufreq tables.
>
> Hence use a flag to mark which API to use for allocating and freeing
> the tables.
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
I'd prefer to see this even cleaner by dropping the clk_* versions all
together. Then, for those who want OMAP2 support (currently not working
or validated anyways), all that's needed is to add a function simlilar
to clk_init_cpufreq_table() which registers OPPs.
Kevin
> ---
> arch/arm/mach-omap2/omap2plus-cpufreq.c | 20 +++++++++++++++-----
> 1 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> index 2d4e9d7..dbbf8b2 100644
> --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
> +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> @@ -44,6 +44,7 @@ static struct cpufreq_frequency_table *freq_table;
> static struct clk *mpu_clk;
> static char *mpu_clk_name;
> static struct device *mpu_dev;
> +static bool use_opp;
>
> static int omap_verify_speed(struct cpufreq_policy *policy)
> {
> @@ -166,7 +167,10 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
> return -EINVAL;
>
> policy->cur = policy->min = policy->max = omap_getspeed(policy->cpu);
> - opp_init_cpufreq_table(mpu_dev, &freq_table);
> + if (use_opp)
> + opp_init_cpufreq_table(mpu_dev, &freq_table);
> + else
> + clk_init_cpufreq_table(&freq_table);
>
> if (freq_table) {
> result = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> @@ -204,7 +208,10 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
>
> static int omap_cpu_exit(struct cpufreq_policy *policy)
> {
> - clk_exit_cpufreq_table(&freq_table);
> + if (use_opp)
> + opp_free_cpufreq_table(mpu_dev, &freq_table);
> + else
> + clk_exit_cpufreq_table(&freq_table);
> clk_put(mpu_clk);
> return 0;
> }
> @@ -227,12 +234,15 @@ static struct cpufreq_driver omap_driver = {
>
> static int __init omap_cpufreq_init(void)
> {
> - if (cpu_is_omap24xx())
> + use_opp = true;
> + if (cpu_is_omap24xx()) {
> mpu_clk_name = "virt_prcm_set";
> - else if (cpu_is_omap34xx())
> + use_opp = false;
> + } else if (cpu_is_omap34xx()) {
> mpu_clk_name = "dpll1_ck";
> - else if (cpu_is_omap44xx())
> + } else if (cpu_is_omap44xx()) {
> mpu_clk_name = "dpll_mpu_ck";
> + }
>
> if (!mpu_clk_name) {
> pr_err("%s: unsupported Silicon?\n", __func__);
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 3/8] OMAP2+: cpufreq: use opp/clk_*cpufreq_table based on silicon
2011-05-26 17:38 ` Kevin Hilman
@ 2011-05-26 18:35 ` Menon, Nishanth
2011-05-26 18:39 ` Menon, Nishanth
0 siblings, 1 reply; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-26 18:35 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap
On Thu, May 26, 2011 at 10:38, Kevin Hilman <khilman@ti.com> wrote:
>
> I'd prefer to see this even cleaner by dropping the clk_* versions all
> together. Then, for those who want OMAP2 support (currently not working
> or validated anyways), all that's needed is to add a function simlilar
> to clk_init_cpufreq_table() which registers OPPs.
yeah - that is better as well.. will do
Regards,
Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PM-WIP_CPUFREQ][PATCH V3 3/8] OMAP2+: cpufreq: use opp/clk_*cpufreq_table based on silicon
2011-05-26 18:35 ` Menon, Nishanth
@ 2011-05-26 18:39 ` Menon, Nishanth
2011-05-26 20:25 ` Kevin Hilman
0 siblings, 1 reply; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-26 18:39 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap
On Thu, May 26, 2011 at 11:35, Menon, Nishanth <nm@ti.com> wrote:
> On Thu, May 26, 2011 at 10:38, Kevin Hilman <khilman@ti.com> wrote:
>>
>> I'd prefer to see this even cleaner by dropping the clk_* versions all
>> together. Then, for those who want OMAP2 support (currently not working
>> or validated anyways), all that's needed is to add a function simlilar
>> to clk_init_cpufreq_table() which registers OPPs.
>
> yeah - that is better as well.. will do
oops - missed asking - are you ok with just returning a warning and
not registering the cpufreq driver for OMAP2?
Regards,
Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PM-WIP_CPUFREQ][PATCH V3 3/8] OMAP2+: cpufreq: use opp/clk_*cpufreq_table based on silicon
2011-05-26 18:39 ` Menon, Nishanth
@ 2011-05-26 20:25 ` Kevin Hilman
0 siblings, 0 replies; 37+ messages in thread
From: Kevin Hilman @ 2011-05-26 20:25 UTC (permalink / raw)
To: Menon, Nishanth; +Cc: linux-omap
"Menon, Nishanth" <nm@ti.com> writes:
> On Thu, May 26, 2011 at 11:35, Menon, Nishanth <nm@ti.com> wrote:
>> On Thu, May 26, 2011 at 10:38, Kevin Hilman <khilman@ti.com> wrote:
>>>
>>> I'd prefer to see this even cleaner by dropping the clk_* versions all
>>> together. Then, for those who want OMAP2 support (currently not working
>>> or validated anyways), all that's needed is to add a function simlilar
>>> to clk_init_cpufreq_table() which registers OPPs.
>>
>> yeah - that is better as well.. will do
>
> oops - missed asking - are you ok with just returning a warning and
> not registering the cpufreq driver for OMAP2?
s/OMAP2/platforms without OPPs/
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PM-WIP_CPUFREQ][PATCH V3 4/8] OMAP2+: cpufreq: dont support !freq_table
2011-05-25 23:38 [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Nishanth Menon
` (2 preceding siblings ...)
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 3/8] OMAP2+: cpufreq: use opp/clk_*cpufreq_table based on silicon Nishanth Menon
@ 2011-05-25 23:38 ` Nishanth Menon
2011-05-26 0:51 ` Todd Poynor
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 5/8] OMAP2+: cpufreq: fix invalid cpufreq table with central alloc/free Nishanth Menon
` (4 subsequent siblings)
8 siblings, 1 reply; 37+ messages in thread
From: Nishanth Menon @ 2011-05-25 23:38 UTC (permalink / raw)
To: linux-omap; +Cc: Kevin, Nishanth Menon
OMAP2+ all have frequency tables, hence the hacks we had for older
silicon do not need to be carried forward. As part of this change,
use cpufreq_frequency_table_target to find the best match for
frequency requested.
Signed-off-by: Nishanth Menon <nm@ti.com>
---
arch/arm/mach-omap2/omap2plus-cpufreq.c | 64 +++++++++++++++---------------
1 files changed, 32 insertions(+), 32 deletions(-)
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index dbbf8b2..7c0eb77 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -38,8 +38,6 @@
#include <mach/hardware.h>
-#define VERY_HI_RATE 900000000
-
static struct cpufreq_frequency_table *freq_table;
static struct clk *mpu_clk;
static char *mpu_clk_name;
@@ -48,20 +46,9 @@ static bool use_opp;
static int omap_verify_speed(struct cpufreq_policy *policy)
{
- if (freq_table)
- return cpufreq_frequency_table_verify(policy, freq_table);
-
- if (policy->cpu)
+ if (!freq_table)
return -EINVAL;
-
- cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
- policy->cpuinfo.max_freq);
-
- policy->min = clk_round_rate(mpu_clk, policy->min * 1000) / 1000;
- policy->max = clk_round_rate(mpu_clk, policy->max * 1000) / 1000;
- cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
- policy->cpuinfo.max_freq);
- return 0;
+ return cpufreq_frequency_table_verify(policy, freq_table);
}
static unsigned int omap_getspeed(unsigned int cpu)
@@ -79,22 +66,35 @@ static int omap_target(struct cpufreq_policy *policy,
unsigned int target_freq,
unsigned int relation)
{
- int i, ret = 0;
+ unsigned int i;
+ int ret = 0;
struct cpufreq_freqs freqs;
/* Changes not allowed until all CPUs are online */
if (is_smp() && (num_online_cpus() < NR_CPUS))
return ret;
- /* Ensure desired rate is within allowed range. Some govenors
- * (ondemand) will just pass target_freq=0 to get the minimum. */
- if (target_freq < policy->min)
- target_freq = policy->min;
- if (target_freq > policy->max)
- target_freq = policy->max;
+ if (!freq_table) {
+ dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
+ policy->cpu);
+ return -EINVAL;
+ }
+
+ ret = cpufreq_frequency_table_target(policy, freq_table, target_freq,
+ relation, &i);
+ if (ret) {
+ dev_dbg(mpu_dev, "%s: cpu%d: no freq match for %d(ret=%d)\n",
+ __func__, policy->cpu, target_freq, ret);
+ return ret;
+ }
+ freqs.new = freq_table[i].frequency;
+ if (!freqs.new) {
+ dev_err(mpu_dev, "%s: cpu%d: no match for freq %d\n", __func__,
+ policy->cpu, target_freq);
+ return -EINVAL;
+ }
freqs.old = omap_getspeed(policy->cpu);
- freqs.new = clk_round_rate(mpu_clk, target_freq * 1000) / 1000;
freqs.cpu = policy->cpu;
if (freqs.old == freqs.new)
@@ -172,17 +172,17 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
else
clk_init_cpufreq_table(&freq_table);
- if (freq_table) {
- result = cpufreq_frequency_table_cpuinfo(policy, freq_table);
- if (!result)
- cpufreq_frequency_table_get_attr(freq_table,
- policy->cpu);
- } else {
- policy->cpuinfo.min_freq = clk_round_rate(mpu_clk, 0) / 1000;
- policy->cpuinfo.max_freq = clk_round_rate(mpu_clk,
- VERY_HI_RATE) / 1000;
+ if (!freq_table) {
+ dev_err(mpu_dev, "%s: cpu%d: unable to allocate freq table\n",
+ __func__, policy->cpu);
+ return -ENOMEM;
}
+ result = cpufreq_frequency_table_cpuinfo(policy, freq_table);
+ if (!result)
+ cpufreq_frequency_table_get_attr(freq_table,
+ policy->cpu);
+
policy->min = policy->cpuinfo.min_freq;
policy->max = policy->cpuinfo.max_freq;
policy->cur = omap_getspeed(policy->cpu);
--
1.7.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 4/8] OMAP2+: cpufreq: dont support !freq_table
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 4/8] OMAP2+: cpufreq: dont support !freq_table Nishanth Menon
@ 2011-05-26 0:51 ` Todd Poynor
2011-05-26 0:53 ` Menon, Nishanth
[not found] ` <SNT104-W336E0DDFB034FA635C328BBA770@phx.gbl>
0 siblings, 2 replies; 37+ messages in thread
From: Todd Poynor @ 2011-05-26 0:51 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap, Kevin
On Wed, May 25, 2011 at 04:38:49PM -0700, Nishanth Menon wrote:
> OMAP2+ all have frequency tables, hence the hacks we had for older
> silicon do not need to be carried forward. As part of this change,
> use cpufreq_frequency_table_target to find the best match for
> frequency requested.
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
...
> @@ -79,22 +66,35 @@ static int omap_target(struct cpufreq_policy *policy,
> unsigned int target_freq,
> unsigned int relation)
> {
> - int i, ret = 0;
> + unsigned int i;
> + int ret = 0;
> struct cpufreq_freqs freqs;
>
> /* Changes not allowed until all CPUs are online */
> if (is_smp() && (num_online_cpus() < NR_CPUS))
> return ret;
>
> - /* Ensure desired rate is within allowed range. Some govenors
> - * (ondemand) will just pass target_freq=0 to get the minimum. */
> - if (target_freq < policy->min)
> - target_freq = policy->min;
> - if (target_freq > policy->max)
> - target_freq = policy->max;
> + if (!freq_table) {
> + dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
> + policy->cpu);
Just a minor comment: suggest dev_dbg() or WARN_ONCE() for some of
these conditions that may be frequently evaluated and probably won't
be cleared up after being hit once.
...
Todd
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 4/8] OMAP2+: cpufreq: dont support !freq_table
2011-05-26 0:51 ` Todd Poynor
@ 2011-05-26 0:53 ` Menon, Nishanth
[not found] ` <SNT104-W336E0DDFB034FA635C328BBA770@phx.gbl>
1 sibling, 0 replies; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-26 0:53 UTC (permalink / raw)
To: Todd Poynor; +Cc: linux-omap, Kevin
On Wed, May 25, 2011 at 17:51, Todd Poynor <toddpoynor@google.com> wrote:
>
>> + if (!freq_table) {
>> + dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
>> + policy->cpu);
>
> Just a minor comment: suggest dev_dbg() or WARN_ONCE() for some of
> these conditions that may be frequently evaluated and probably won't
> be cleared up after being hit once.
This is an error because we dont expect it as far as the device is
concerned. I'd use a debug/warn if I can recover out of it and
continue functionality.
Regards,
Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread[parent not found: <SNT104-W336E0DDFB034FA635C328BBA770@phx.gbl>]
* Re: [PM-WIP_CPUFREQ][PATCH V3 4/8] OMAP2+: cpufreq: dont support !freq_table
[not found] ` <SNT104-W336E0DDFB034FA635C328BBA770@phx.gbl>
@ 2011-05-26 1:03 ` Menon, Nishanth
0 siblings, 0 replies; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-26 1:03 UTC (permalink / raw)
To: ym cheng; +Cc: toddpoynor, linux-omap, khilman
On Wed, May 25, 2011 at 17:53, ym cheng <yongmingcheng@hotmail.com> wrote:
> OK,I wil test on s3c2440 board,and share test data.
The series was meant for OMAP, if you meant:
http://www.samsung.com/global/business/semiconductor/productInfo.do?fmly_id=836&partnum=S3C2440
as far as I see, this platform is supported on arch/arm/mach-s3c24*
Regards,
Nishanth Menon
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PM-WIP_CPUFREQ][PATCH V3 5/8] OMAP2+: cpufreq: fix invalid cpufreq table with central alloc/free
2011-05-25 23:38 [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Nishanth Menon
` (3 preceding siblings ...)
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 4/8] OMAP2+: cpufreq: dont support !freq_table Nishanth Menon
@ 2011-05-25 23:38 ` Nishanth Menon
2011-05-26 1:09 ` Todd Poynor
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak Nishanth Menon
` (3 subsequent siblings)
8 siblings, 1 reply; 37+ messages in thread
From: Nishanth Menon @ 2011-05-25 23:38 UTC (permalink / raw)
To: linux-omap; +Cc: Kevin, Nishanth Menon
By creating freq_table_[alloc|free] we can handle the differences
between OMAP2 and OMAP3+ systems and we have a centralized allocation
and cleanup strategy. We use this to cleanup the freq_table when
cpufreq_frequency_table_cpuinfo fails.
Signed-off-by: Nishanth Menon <nm@ti.com>
---
arch/arm/mach-omap2/omap2plus-cpufreq.c | 48 +++++++++++++++++++++---------
1 files changed, 33 insertions(+), 15 deletions(-)
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 7c0eb77..3ff3302 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -154,6 +154,26 @@ skip_lpj:
return ret;
}
+static int freq_table_alloc(void)
+{
+ if (use_opp)
+ return opp_init_cpufreq_table(mpu_dev, &freq_table);
+
+ clk_init_cpufreq_table(&freq_table);
+ if (!freq_table)
+ return -ENOMEM;
+
+ return 0;
+}
+
+static void freq_table_free(void)
+{
+ if (use_opp)
+ opp_free_cpufreq_table(mpu_dev, &freq_table);
+ else
+ clk_exit_cpufreq_table(&freq_table);
+}
+
static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
{
int result = 0;
@@ -167,21 +187,22 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
return -EINVAL;
policy->cur = policy->min = policy->max = omap_getspeed(policy->cpu);
- if (use_opp)
- opp_init_cpufreq_table(mpu_dev, &freq_table);
- else
- clk_init_cpufreq_table(&freq_table);
- if (!freq_table) {
- dev_err(mpu_dev, "%s: cpu%d: unable to allocate freq table\n",
- __func__, policy->cpu);
- return -ENOMEM;
+ result = freq_table_alloc();
+ if (result || !freq_table) {
+ dev_err(mpu_dev, "%s: cpu%d: unable to get freq table [%d]\n",
+ __func__, policy->cpu, result);
+ return result;
}
result = cpufreq_frequency_table_cpuinfo(policy, freq_table);
- if (!result)
- cpufreq_frequency_table_get_attr(freq_table,
- policy->cpu);
+ if (result) {
+ dev_err(mpu_dev, "%s: cpu%d: unable to get cpuinfo [%d]\n",
+ __func__, policy->cpu, result);
+ freq_table_free();
+ return result;
+ }
+ cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
policy->min = policy->cpuinfo.min_freq;
policy->max = policy->cpuinfo.max_freq;
@@ -208,10 +229,7 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
static int omap_cpu_exit(struct cpufreq_policy *policy)
{
- if (use_opp)
- opp_free_cpufreq_table(mpu_dev, &freq_table);
- else
- clk_exit_cpufreq_table(&freq_table);
+ freq_table_free();
clk_put(mpu_clk);
return 0;
}
--
1.7.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 5/8] OMAP2+: cpufreq: fix invalid cpufreq table with central alloc/free
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 5/8] OMAP2+: cpufreq: fix invalid cpufreq table with central alloc/free Nishanth Menon
@ 2011-05-26 1:09 ` Todd Poynor
2011-05-26 1:21 ` Menon, Nishanth
0 siblings, 1 reply; 37+ messages in thread
From: Todd Poynor @ 2011-05-26 1:09 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap, Kevin
On Wed, May 25, 2011 at 04:38:50PM -0700, Nishanth Menon wrote:
> By creating freq_table_[alloc|free] we can handle the differences
> between OMAP2 and OMAP3+ systems and we have a centralized allocation
> and cleanup strategy. We use this to cleanup the freq_table when
> cpufreq_frequency_table_cpuinfo fails.
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
...
> static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
> {
> int result = 0;
> @@ -167,21 +187,22 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
> return -EINVAL;
>
> policy->cur = policy->min = policy->max = omap_getspeed(policy->cpu);
> - if (use_opp)
> - opp_init_cpufreq_table(mpu_dev, &freq_table);
> - else
> - clk_init_cpufreq_table(&freq_table);
>
> - if (!freq_table) {
> - dev_err(mpu_dev, "%s: cpu%d: unable to allocate freq table\n",
> - __func__, policy->cpu);
> - return -ENOMEM;
> + result = freq_table_alloc();
> + if (result || !freq_table) {
> + dev_err(mpu_dev, "%s: cpu%d: unable to get freq table [%d]\n",
> + __func__, policy->cpu, result);
> + return result;
The "|| !freq_table" isn't needed, and technically allows the code to
return zero for an error return if the subexpression does evaluate
true.
Todd
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 5/8] OMAP2+: cpufreq: fix invalid cpufreq table with central alloc/free
2011-05-26 1:09 ` Todd Poynor
@ 2011-05-26 1:21 ` Menon, Nishanth
0 siblings, 0 replies; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-26 1:21 UTC (permalink / raw)
To: Todd Poynor; +Cc: linux-omap, Kevin
On Wed, May 25, 2011 at 18:09, Todd Poynor <toddpoynor@google.com> wrote:
>> + if (result || !freq_table) {
>> + dev_err(mpu_dev, "%s: cpu%d: unable to get freq table [%d]\n",
>> + __func__, policy->cpu, result);
>> + return result;
>
> The "|| !freq_table" isn't needed, and technically allows the code to
> return zero for an error return if the subexpression does evaluate
> true.
>
Ack..
Regards,
Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
2011-05-25 23:38 [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Nishanth Menon
` (4 preceding siblings ...)
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 5/8] OMAP2+: cpufreq: fix invalid cpufreq table with central alloc/free Nishanth Menon
@ 2011-05-25 23:38 ` Nishanth Menon
2011-05-26 0:16 ` Kevin Hilman
2011-05-26 1:25 ` Todd Poynor
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 7/8] OMAP2+: cpufreq: put clk if cpu_init failed Nishanth Menon
` (2 subsequent siblings)
8 siblings, 2 replies; 37+ messages in thread
From: Nishanth Menon @ 2011-05-25 23:38 UTC (permalink / raw)
To: linux-omap; +Cc: Kevin, Nishanth Menon
Since we have multiple CPUs, the cpuinit call for CPU1 causes
freq_table of CPU0 to be overwritten. Instead, we maintain
a counter to keep track of cpus who use the cpufreq table
allocate it once(one freq table for all CPUs) and free them
once the last user is done with it. We also need to protect
freq_table and this new counter from updates from multiple
contexts to be on a safe side.
Signed-off-by: Nishanth Menon <nm@ti.com>
---
arch/arm/mach-omap2/omap2plus-cpufreq.c | 62 +++++++++++++++++++++++++++----
1 files changed, 54 insertions(+), 8 deletions(-)
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 3ff3302..f026ac4 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -39,6 +39,9 @@
#include <mach/hardware.h>
static struct cpufreq_frequency_table *freq_table;
+static int freq_table_users;
+static DEFINE_MUTEX(freq_table_lock);
+
static struct clk *mpu_clk;
static char *mpu_clk_name;
static struct device *mpu_dev;
@@ -46,9 +49,17 @@ static bool use_opp;
static int omap_verify_speed(struct cpufreq_policy *policy)
{
+ int r = -EINVAL;
+
+ mutex_lock(&freq_table_lock);
if (!freq_table)
- return -EINVAL;
- return cpufreq_frequency_table_verify(policy, freq_table);
+ goto out;
+
+ r = cpufreq_frequency_table_verify(policy, freq_table);
+
+out:
+ mutex_unlock(&freq_table_lock);
+ return r;
}
static unsigned int omap_getspeed(unsigned int cpu)
@@ -74,9 +85,11 @@ static int omap_target(struct cpufreq_policy *policy,
if (is_smp() && (num_online_cpus() < NR_CPUS))
return ret;
+ mutex_lock(&freq_table_lock);
if (!freq_table) {
dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
policy->cpu);
+ mutex_unlock(&freq_table_lock);
return -EINVAL;
}
@@ -85,9 +98,13 @@ static int omap_target(struct cpufreq_policy *policy,
if (ret) {
dev_dbg(mpu_dev, "%s: cpu%d: no freq match for %d(ret=%d)\n",
__func__, policy->cpu, target_freq, ret);
+ mutex_unlock(&freq_table_lock);
return ret;
}
+
freqs.new = freq_table[i].frequency;
+ mutex_unlock(&freq_table_lock);
+
if (!freqs.new) {
dev_err(mpu_dev, "%s: cpu%d: no match for freq %d\n", __func__,
policy->cpu, target_freq);
@@ -156,22 +173,48 @@ skip_lpj:
static int freq_table_alloc(void)
{
- if (use_opp)
- return opp_init_cpufreq_table(mpu_dev, &freq_table);
+ int ret = 0;
- clk_init_cpufreq_table(&freq_table);
- if (!freq_table)
- return -ENOMEM;
+ mutex_lock(&freq_table_lock);
- return 0;
+ freq_table_users++;
+ /* Did we allocate previously? */
+ if (freq_table_users - 1)
+ goto out;
+
+ /* no, so we allocate */
+ if (use_opp) {
+ ret = opp_init_cpufreq_table(mpu_dev, &freq_table);
+ } else {
+ clk_init_cpufreq_table(&freq_table);
+ if (!freq_table)
+ ret = -ENOMEM;
+ }
+ /* if we did not allocate cleanly.. reduce user count */
+ if (!ret)
+ freq_table_users--;
+
+out:
+ mutex_unlock(&freq_table_lock);
+ return ret;
}
static void freq_table_free(void)
{
+ mutex_lock(&freq_table_lock);
+
+ if (!freq_table_users)
+ goto out;
+ freq_table_users--;
+ if (freq_table_users)
+ goto out;
+
if (use_opp)
opp_free_cpufreq_table(mpu_dev, &freq_table);
else
clk_exit_cpufreq_table(&freq_table);
+out:
+ mutex_unlock(&freq_table_lock);
}
static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
@@ -195,14 +238,17 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
return result;
}
+ mutex_lock(&freq_table_lock);
result = cpufreq_frequency_table_cpuinfo(policy, freq_table);
if (result) {
+ mutex_unlock(&freq_table_lock);
dev_err(mpu_dev, "%s: cpu%d: unable to get cpuinfo [%d]\n",
__func__, policy->cpu, result);
freq_table_free();
return result;
}
cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
+ mutex_unlock(&freq_table_lock);
policy->min = policy->cpuinfo.min_freq;
policy->max = policy->cpuinfo.max_freq;
--
1.7.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak Nishanth Menon
@ 2011-05-26 0:16 ` Kevin Hilman
2011-05-26 0:47 ` Menon, Nishanth
2011-05-26 1:25 ` Todd Poynor
1 sibling, 1 reply; 37+ messages in thread
From: Kevin Hilman @ 2011-05-26 0:16 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap
Nishanth Menon <nm@ti.com> writes:
> Since we have multiple CPUs, the cpuinit call for CPU1 causes
> freq_table of CPU0 to be overwritten. Instead, we maintain
> a counter to keep track of cpus who use the cpufreq table
> allocate it once(one freq table for all CPUs) and free them
> once the last user is done with it. We also need to protect
> freq_table and this new counter from updates from multiple
> contexts to be on a safe side.
Not sure I understand the need for all the locking here. Once allocated
and filled, the freq_table isn't changing. Also, all the functions are
only reading the freq_table, not changing it. So what is it you're
trying to protect against?
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> arch/arm/mach-omap2/omap2plus-cpufreq.c | 62 +++++++++++++++++++++++++++----
> 1 files changed, 54 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> index 3ff3302..f026ac4 100644
> --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
> +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> @@ -39,6 +39,9 @@
> #include <mach/hardware.h>
>
> static struct cpufreq_frequency_table *freq_table;
> +static int freq_table_users;
> +static DEFINE_MUTEX(freq_table_lock);
> +
> static struct clk *mpu_clk;
> static char *mpu_clk_name;
> static struct device *mpu_dev;
> @@ -46,9 +49,17 @@ static bool use_opp;
>
> static int omap_verify_speed(struct cpufreq_policy *policy)
> {
> + int r = -EINVAL;
> +
> + mutex_lock(&freq_table_lock);
> if (!freq_table)
> - return -EINVAL;
> - return cpufreq_frequency_table_verify(policy, freq_table);
> + goto out;
> +
> + r = cpufreq_frequency_table_verify(policy, freq_table);
> +
> +out:
> + mutex_unlock(&freq_table_lock);
> + return r;
> }
>
> static unsigned int omap_getspeed(unsigned int cpu)
> @@ -74,9 +85,11 @@ static int omap_target(struct cpufreq_policy *policy,
> if (is_smp() && (num_online_cpus() < NR_CPUS))
> return ret;
>
> + mutex_lock(&freq_table_lock);
> if (!freq_table) {
> dev_err(mpu_dev, "%s: cpu%d: no freq table!\n", __func__,
> policy->cpu);
> + mutex_unlock(&freq_table_lock);
> return -EINVAL;
> }
>
> @@ -85,9 +98,13 @@ static int omap_target(struct cpufreq_policy *policy,
> if (ret) {
> dev_dbg(mpu_dev, "%s: cpu%d: no freq match for %d(ret=%d)\n",
> __func__, policy->cpu, target_freq, ret);
> + mutex_unlock(&freq_table_lock);
> return ret;
> }
> +
> freqs.new = freq_table[i].frequency;
> + mutex_unlock(&freq_table_lock);
> +
> if (!freqs.new) {
> dev_err(mpu_dev, "%s: cpu%d: no match for freq %d\n", __func__,
> policy->cpu, target_freq);
> @@ -156,22 +173,48 @@ skip_lpj:
>
> static int freq_table_alloc(void)
> {
> - if (use_opp)
> - return opp_init_cpufreq_table(mpu_dev, &freq_table);
> + int ret = 0;
>
> - clk_init_cpufreq_table(&freq_table);
> - if (!freq_table)
> - return -ENOMEM;
> + mutex_lock(&freq_table_lock);
>
> - return 0;
> + freq_table_users++;
> + /* Did we allocate previously? */
> + if (freq_table_users - 1)
> + goto out;
Rather than the ' - 1', this can just be
if (freq_table_users++)
goto out;
or better, you probably don't need this check protected by the mutex,
so this could just return directly, and then take the mutex_lock() after
this point.
However, if you get rid of the mutex (and I think you should), you could
use an atomic variable here
> + /* no, so we allocate */
> + if (use_opp) {
> + ret = opp_init_cpufreq_table(mpu_dev, &freq_table);
> + } else {
> + clk_init_cpufreq_table(&freq_table);
> + if (!freq_table)
> + ret = -ENOMEM;
> + }
> + /* if we did not allocate cleanly.. reduce user count */
> + if (!ret)
> + freq_table_users--;
> +
> +out:
> + mutex_unlock(&freq_table_lock);
> + return ret;
> }
>
> static void freq_table_free(void)
> {
> + mutex_lock(&freq_table_lock);
> +
> + if (!freq_table_users)
> + goto out;
> + freq_table_users--;
> + if (freq_table_users)
> + goto out;
Similarily:
if (--freq_table_users)
goto out;
> if (use_opp)
> opp_free_cpufreq_table(mpu_dev, &freq_table);
> else
> clk_exit_cpufreq_table(&freq_table);
> +out:
> + mutex_unlock(&freq_table_lock);
> }
>
> static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
> @@ -195,14 +238,17 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
> return result;
> }
>
> + mutex_lock(&freq_table_lock);
> result = cpufreq_frequency_table_cpuinfo(policy, freq_table);
> if (result) {
> + mutex_unlock(&freq_table_lock);
> dev_err(mpu_dev, "%s: cpu%d: unable to get cpuinfo [%d]\n",
> __func__, policy->cpu, result);
> freq_table_free();
> return result;
> }
> cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
> + mutex_unlock(&freq_table_lock);
>
> policy->min = policy->cpuinfo.min_freq;
> policy->max = policy->cpuinfo.max_freq;
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
2011-05-26 0:16 ` Kevin Hilman
@ 2011-05-26 0:47 ` Menon, Nishanth
2011-05-26 17:11 ` Kevin Hilman
0 siblings, 1 reply; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-26 0:47 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap
On Wed, May 25, 2011 at 17:16, Kevin Hilman <khilman@ti.com> wrote:
>
> Nishanth Menon <nm@ti.com> writes:
>
> > Since we have multiple CPUs, the cpuinit call for CPU1 causes
> > freq_table of CPU0 to be overwritten. Instead, we maintain
> > a counter to keep track of cpus who use the cpufreq table
> > allocate it once(one freq table for all CPUs) and free them
> > once the last user is done with it. We also need to protect
> > freq_table and this new counter from updates from multiple
> > contexts to be on a safe side.
>
> Not sure I understand the need for all the locking here. Once allocated
> and filled, the freq_table isn't changing. Also, all the functions are
> only reading the freq_table, not changing it. So what is it you're
> trying to protect against?
We just have one freq_table for both cpu0 and cpu1. We have common
data structure(freq_table and users) which is modifiable in two
APIs(init/exit) and a set of reads. What if there is a read path while
free occurs - I may be mistaken, but my understanding is that the
datastructure used in my code should be secured in my code and I
cannot depend on higher layer(cpufreq/governors) to ensure that.
>
> > Signed-off-by: Nishanth Menon <nm@ti.com>
> > ---
> > arch/arm/mach-omap2/omap2plus-cpufreq.c | 62 +++++++++++++++++++++++++++----
> > 1 files changed, 54 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> > index 3ff3302..f026ac4 100644
> > --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
> > +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
[..]
> > @@ -156,22 +173,48 @@ skip_lpj:
> >
> > static int freq_table_alloc(void)
> > {
> > - if (use_opp)
> > - return opp_init_cpufreq_table(mpu_dev, &freq_table);
> > + int ret = 0;
> >
> > - clk_init_cpufreq_table(&freq_table);
> > - if (!freq_table)
> > - return -ENOMEM;
> > + mutex_lock(&freq_table_lock);
> >
> > - return 0;
> > + freq_table_users++;
> > + /* Did we allocate previously? */
> > + if (freq_table_users - 1)
> > + goto out;
>
> Rather than the ' - 1', this can just be
>
> if (freq_table_users++)
> goto out;
ok
>
> or better, you probably don't need this check protected by the mutex,
> so this could just return directly, and then take the mutex_lock() after
> this point.
The mutex lock was to protect both the freq_table and the count as
they protect the same resource - freq_table
>
> However, if you get rid of the mutex (and I think you should), you could
> use an atomic variable here
yes, we can use just atomic to protect alloc Vs free - but we cannot
protect read Vs free
Regards,
Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
2011-05-26 0:47 ` Menon, Nishanth
@ 2011-05-26 17:11 ` Kevin Hilman
2011-05-26 18:34 ` Menon, Nishanth
0 siblings, 1 reply; 37+ messages in thread
From: Kevin Hilman @ 2011-05-26 17:11 UTC (permalink / raw)
To: Menon, Nishanth; +Cc: linux-omap
"Menon, Nishanth" <nm@ti.com> writes:
> On Wed, May 25, 2011 at 17:16, Kevin Hilman <khilman@ti.com> wrote:
>>
>> Nishanth Menon <nm@ti.com> writes:
>>
>> > Since we have multiple CPUs, the cpuinit call for CPU1 causes
>> > freq_table of CPU0 to be overwritten. Instead, we maintain
>> > a counter to keep track of cpus who use the cpufreq table
>> > allocate it once(one freq table for all CPUs) and free them
>> > once the last user is done with it. We also need to protect
>> > freq_table and this new counter from updates from multiple
>> > contexts to be on a safe side.
>>
>> Not sure I understand the need for all the locking here. Once allocated
>> and filled, the freq_table isn't changing. Also, all the functions are
>> only reading the freq_table, not changing it. So what is it you're
>> trying to protect against?
>
> We just have one freq_table for both cpu0 and cpu1. We have common
> data structure(freq_table and users) which is modifiable in two
> APIs(init/exit) and a set of reads.
The table is not modifiable in two places. It's only modified once,
upon creation. After that the table contents are constant.
What is changable is simply the existence of the table. This can be
handled by simply checking the pointer (or using your counter.)
> What if there is a read path while free occurs -
Then the CPUfreq driver has a bug. If you want to be safe, check for a
valid pointer or use your counter.
> I may be mistaken, but my understanding is that the
> datastructure used in my code should be secured in my code and I
> cannot depend on higher layer(cpufreq/governors) to ensure that.
When you're talking about potentially concurrent modification of data,
that make sense. Here you're implementing a plugin for an existing
framework, and you can (and have to) make assumptions about when the
callbacks are used.
Kevin
>>
>> > Signed-off-by: Nishanth Menon <nm@ti.com>
>> > ---
>> > arch/arm/mach-omap2/omap2plus-cpufreq.c | 62 +++++++++++++++++++++++++++----
>> > 1 files changed, 54 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
>> > index 3ff3302..f026ac4 100644
>> > --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
>> > +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
>
> [..]
>> > @@ -156,22 +173,48 @@ skip_lpj:
>> >
>> > static int freq_table_alloc(void)
>> > {
>> > - if (use_opp)
>> > - return opp_init_cpufreq_table(mpu_dev, &freq_table);
>> > + int ret = 0;
>> >
>> > - clk_init_cpufreq_table(&freq_table);
>> > - if (!freq_table)
>> > - return -ENOMEM;
>> > + mutex_lock(&freq_table_lock);
>> >
>> > - return 0;
>> > + freq_table_users++;
>> > + /* Did we allocate previously? */
>> > + if (freq_table_users - 1)
>> > + goto out;
>>
>> Rather than the ' - 1', this can just be
>>
>> if (freq_table_users++)
>> goto out;
> ok
>
>>
>> or better, you probably don't need this check protected by the mutex,
>> so this could just return directly, and then take the mutex_lock() after
>> this point.
> The mutex lock was to protect both the freq_table and the count as
> they protect the same resource - freq_table
>
>>
>> However, if you get rid of the mutex (and I think you should), you could
>> use an atomic variable here
> yes, we can use just atomic to protect alloc Vs free - but we cannot
> protect read Vs free
>
>
> Regards,
> Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
2011-05-26 17:11 ` Kevin Hilman
@ 2011-05-26 18:34 ` Menon, Nishanth
0 siblings, 0 replies; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-26 18:34 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap
On Thu, May 26, 2011 at 10:11, Kevin Hilman <khilman@ti.com> wrote:
>
> When you're talking about potentially concurrent modification of data,
> that make sense. Here you're implementing a plugin for an existing
> framework, and you can (and have to) make assumptions about when the
> callbacks are used.
ok, that is the one i was looking for. thanks. will use atomic ops and
drop the mutex.
Regards,
Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak Nishanth Menon
2011-05-26 0:16 ` Kevin Hilman
@ 2011-05-26 1:25 ` Todd Poynor
2011-05-26 1:36 ` Menon, Nishanth
1 sibling, 1 reply; 37+ messages in thread
From: Todd Poynor @ 2011-05-26 1:25 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap, Kevin
On Wed, May 25, 2011 at 04:38:51PM -0700, Nishanth Menon wrote:
> Since we have multiple CPUs, the cpuinit call for CPU1 causes
> freq_table of CPU0 to be overwritten. Instead, we maintain
> a counter to keep track of cpus who use the cpufreq table
> allocate it once(one freq table for all CPUs) and free them
> once the last user is done with it. We also need to protect
> freq_table and this new counter from updates from multiple
> contexts to be on a safe side.
>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
...
> static int freq_table_alloc(void)
> {
> - if (use_opp)
> - return opp_init_cpufreq_table(mpu_dev, &freq_table);
> + int ret = 0;
>
> - clk_init_cpufreq_table(&freq_table);
> - if (!freq_table)
> - return -ENOMEM;
> + mutex_lock(&freq_table_lock);
>
> - return 0;
> + freq_table_users++;
> + /* Did we allocate previously? */
> + if (freq_table_users - 1)
> + goto out;
> +
> + /* no, so we allocate */
> + if (use_opp) {
> + ret = opp_init_cpufreq_table(mpu_dev, &freq_table);
> + } else {
> + clk_init_cpufreq_table(&freq_table);
> + if (!freq_table)
> + ret = -ENOMEM;
> + }
> + /* if we did not allocate cleanly.. reduce user count */
> + if (!ret)
> + freq_table_users--;
"if (ret)" intended? ret == 0 means allocated OK.
Todd
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak
2011-05-26 1:25 ` Todd Poynor
@ 2011-05-26 1:36 ` Menon, Nishanth
0 siblings, 0 replies; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-26 1:36 UTC (permalink / raw)
To: Todd Poynor; +Cc: linux-omap, Kevin
On Wed, May 25, 2011 at 18:25, Todd Poynor <toddpoynor@google.com> wrote:
> On Wed, May 25, 2011 at 04:38:51PM -0700, Nishanth Menon wrote:
>> Since we have multiple CPUs, the cpuinit call for CPU1 causes
>> freq_table of CPU0 to be overwritten. Instead, we maintain
>> a counter to keep track of cpus who use the cpufreq table
>> allocate it once(one freq table for all CPUs) and free them
>> once the last user is done with it. We also need to protect
>> freq_table and this new counter from updates from multiple
>> contexts to be on a safe side.
>>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> ---
> ...
>> static int freq_table_alloc(void)
>> {
>> - if (use_opp)
>> - return opp_init_cpufreq_table(mpu_dev, &freq_table);
>> + int ret = 0;
>>
>> - clk_init_cpufreq_table(&freq_table);
>> - if (!freq_table)
>> - return -ENOMEM;
>> + mutex_lock(&freq_table_lock);
>>
>> - return 0;
>> + freq_table_users++;
>> + /* Did we allocate previously? */
>> + if (freq_table_users - 1)
>> + goto out;
>> +
>> + /* no, so we allocate */
>> + if (use_opp) {
>> + ret = opp_init_cpufreq_table(mpu_dev, &freq_table);
>> + } else {
>> + clk_init_cpufreq_table(&freq_table);
>> + if (!freq_table)
>> + ret = -ENOMEM;
>> + }
>> + /* if we did not allocate cleanly.. reduce user count */
>> + if (!ret)
>> + freq_table_users--;
>
> "if (ret)" intended? ret == 0 means allocated OK.
arrgh.. yikes..
Regards,
Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PM-WIP_CPUFREQ][PATCH V3 7/8] OMAP2+: cpufreq: put clk if cpu_init failed
2011-05-25 23:38 [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Nishanth Menon
` (5 preceding siblings ...)
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 6/8] OMAP2+: cpufreq: fix freq_table leak Nishanth Menon
@ 2011-05-25 23:38 ` Nishanth Menon
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 8/8] OMAP: cpufreq: minor file header updates Nishanth Menon
2011-05-26 18:10 ` [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Kevin Hilman
8 siblings, 0 replies; 37+ messages in thread
From: Nishanth Menon @ 2011-05-25 23:38 UTC (permalink / raw)
To: linux-omap; +Cc: Kevin, Nishanth Menon
Release the mpu_clk in fail paths.
Reported-by: Todd Poynor <toddpoynor@google.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
arch/arm/mach-omap2/omap2plus-cpufreq.c | 17 ++++++++++++-----
1 files changed, 12 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index f026ac4..594100e 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -226,8 +226,10 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
if (IS_ERR(mpu_clk))
return PTR_ERR(mpu_clk);
- if (policy->cpu >= NR_CPUS)
- return -EINVAL;
+ if (policy->cpu >= NR_CPUS) {
+ result = -EINVAL;
+ goto fail1;
+ }
policy->cur = policy->min = policy->max = omap_getspeed(policy->cpu);
@@ -235,7 +237,7 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
if (result || !freq_table) {
dev_err(mpu_dev, "%s: cpu%d: unable to get freq table [%d]\n",
__func__, policy->cpu, result);
- return result;
+ goto fail1;
}
mutex_lock(&freq_table_lock);
@@ -244,8 +246,7 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
mutex_unlock(&freq_table_lock);
dev_err(mpu_dev, "%s: cpu%d: unable to get cpuinfo [%d]\n",
__func__, policy->cpu, result);
- freq_table_free();
- return result;
+ goto fail2;
}
cpufreq_frequency_table_get_attr(freq_table, policy->cpu);
mutex_unlock(&freq_table_lock);
@@ -271,6 +272,12 @@ static int __cpuinit omap_cpu_init(struct cpufreq_policy *policy)
policy->cpuinfo.transition_latency = 300 * 1000;
return 0;
+
+fail2:
+ freq_table_free();
+fail1:
+ clk_put(mpu_clk);
+ return result;
}
static int omap_cpu_exit(struct cpufreq_policy *policy)
--
1.7.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PM-WIP_CPUFREQ][PATCH V3 8/8] OMAP: cpufreq: minor file header updates
2011-05-25 23:38 [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Nishanth Menon
` (6 preceding siblings ...)
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 7/8] OMAP2+: cpufreq: put clk if cpu_init failed Nishanth Menon
@ 2011-05-25 23:38 ` Nishanth Menon
2011-05-26 0:18 ` Kevin Hilman
2011-05-26 18:15 ` Kevin Hilman
2011-05-26 18:10 ` [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Kevin Hilman
8 siblings, 2 replies; 37+ messages in thread
From: Nishanth Menon @ 2011-05-25 23:38 UTC (permalink / raw)
To: linux-omap; +Cc: Kevin, Nishanth Menon
Minor file header updates to reflect 2011 for omap2-cpufreq code
and remove misleading OMAP3 reference in omap1 cpufreq code.
Should probably be squashed to:
"OMAP: cpufreq: Split OMAP1 and OMAP2PLUS CPUfreq drivers."
Reported-by: Todd Poynor <toddpoynor@google.com>
Signed-off-by: Nishanth Menon <nm@ti.com>
---
arch/arm/mach-omap1/omap1-cpufreq.c | 1 -
arch/arm/mach-omap2/omap2plus-cpufreq.c | 2 +-
2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-omap1/omap1-cpufreq.c b/arch/arm/mach-omap1/omap1-cpufreq.c
index 682cdc8..7c5216e 100644
--- a/arch/arm/mach-omap1/omap1-cpufreq.c
+++ b/arch/arm/mach-omap1/omap1-cpufreq.c
@@ -9,7 +9,6 @@
* Based on cpu-sa1110.c, Copyright (C) 2001 Russell King
*
* Copyright (C) 2007-2008 Texas Instruments, Inc.
- * Updated to support OMAP3
* Rajendra Nayak <rnayak@ti.com>
*
* This program is free software; you can redistribute it and/or modify
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 594100e..2482c71 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -8,7 +8,7 @@
*
* Based on cpu-sa1110.c, Copyright (C) 2001 Russell King
*
- * Copyright (C) 2007-2008 Texas Instruments, Inc.
+ * Copyright (C) 2007-20011 Texas Instruments, Inc.
* Updated to support OMAP3
* Rajendra Nayak <rnayak@ti.com>
*
--
1.7.1
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH V3 8/8] OMAP: cpufreq: minor file header updates
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 8/8] OMAP: cpufreq: minor file header updates Nishanth Menon
@ 2011-05-26 0:18 ` Kevin Hilman
2011-05-26 0:48 ` Menon, Nishanth
2011-05-26 18:15 ` Kevin Hilman
1 sibling, 1 reply; 37+ messages in thread
From: Kevin Hilman @ 2011-05-26 0:18 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap
Nishanth Menon <nm@ti.com> writes:
> Minor file header updates to reflect 2011 for omap2-cpufreq code
> and remove misleading OMAP3 reference in omap1 cpufreq code.
>
> Should probably be squashed to:
> "OMAP: cpufreq: Split OMAP1 and OMAP2PLUS CPUfreq drivers."
>
> Reported-by: Todd Poynor <toddpoynor@google.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
[...]
> diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> index 594100e..2482c71 100644
> --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
> +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> @@ -8,7 +8,7 @@
> *
> * Based on cpu-sa1110.c, Copyright (C) 2001 Russell King
> *
> - * Copyright (C) 2007-2008 Texas Instruments, Inc.
> + * Copyright (C) 2007-20011 Texas Instruments, Inc.
Wow, you're really wanting to have copyright coverage well into the
future ! ;)
Kevin
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PM-WIP_CPUFREQ][PATCH V3 8/8] OMAP: cpufreq: minor file header updates
2011-05-26 0:18 ` Kevin Hilman
@ 2011-05-26 0:48 ` Menon, Nishanth
0 siblings, 0 replies; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-26 0:48 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap
On Wed, May 25, 2011 at 17:18, Kevin Hilman <khilman@ti.com> wrote:
> Nishanth Menon <nm@ti.com> writes:
>
>> Minor file header updates to reflect 2011 for omap2-cpufreq code
>> and remove misleading OMAP3 reference in omap1 cpufreq code.
>>
>> Should probably be squashed to:
>> "OMAP: cpufreq: Split OMAP1 and OMAP2PLUS CPUfreq drivers."
>>
>> Reported-by: Todd Poynor <toddpoynor@google.com>
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>
> [...]
>
>> diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
>> index 594100e..2482c71 100644
>> --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
>> +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
>> @@ -8,7 +8,7 @@
>> *
>> * Based on cpu-sa1110.c, Copyright (C) 2001 Russell King
>> *
>> - * Copyright (C) 2007-2008 Texas Instruments, Inc.
>> + * Copyright (C) 2007-20011 Texas Instruments, Inc.
>
> Wow, you're really wanting to have copyright coverage well into the
> future ! ;)
I must be on a timemachine ;).. will fix :D
Regards,
Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PM-WIP_CPUFREQ][PATCH V3 8/8] OMAP: cpufreq: minor file header updates
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 8/8] OMAP: cpufreq: minor file header updates Nishanth Menon
2011-05-26 0:18 ` Kevin Hilman
@ 2011-05-26 18:15 ` Kevin Hilman
1 sibling, 0 replies; 37+ messages in thread
From: Kevin Hilman @ 2011-05-26 18:15 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap
Nishanth Menon <nm@ti.com> writes:
> Minor file header updates to reflect 2011 for omap2-cpufreq code
> and remove misleading OMAP3 reference in omap1 cpufreq code.
>
> Should probably be squashed to:
> "OMAP: cpufreq: Split OMAP1 and OMAP2PLUS CPUfreq drivers."
Thanks, squashed.
Kevin
> Reported-by: Todd Poynor <toddpoynor@google.com>
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
> arch/arm/mach-omap1/omap1-cpufreq.c | 1 -
> arch/arm/mach-omap2/omap2plus-cpufreq.c | 2 +-
> 2 files changed, 1 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/mach-omap1/omap1-cpufreq.c b/arch/arm/mach-omap1/omap1-cpufreq.c
> index 682cdc8..7c5216e 100644
> --- a/arch/arm/mach-omap1/omap1-cpufreq.c
> +++ b/arch/arm/mach-omap1/omap1-cpufreq.c
> @@ -9,7 +9,6 @@
> * Based on cpu-sa1110.c, Copyright (C) 2001 Russell King
> *
> * Copyright (C) 2007-2008 Texas Instruments, Inc.
> - * Updated to support OMAP3
> * Rajendra Nayak <rnayak@ti.com>
> *
> * This program is free software; you can redistribute it and/or modify
> diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> index 594100e..2482c71 100644
> --- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
> +++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
> @@ -8,7 +8,7 @@
> *
> * Based on cpu-sa1110.c, Copyright (C) 2001 Russell King
> *
> - * Copyright (C) 2007-2008 Texas Instruments, Inc.
> + * Copyright (C) 2007-20011 Texas Instruments, Inc.
> * Updated to support OMAP3
> * Rajendra Nayak <rnayak@ti.com>
> *
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
2011-05-25 23:38 [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Nishanth Menon
` (7 preceding siblings ...)
2011-05-25 23:38 ` [PM-WIP_CPUFREQ][PATCH V3 8/8] OMAP: cpufreq: minor file header updates Nishanth Menon
@ 2011-05-26 18:10 ` Kevin Hilman
2011-05-26 18:36 ` Menon, Nishanth
2011-05-27 5:06 ` Santosh Shilimkar
8 siblings, 2 replies; 37+ messages in thread
From: Kevin Hilman @ 2011-05-26 18:10 UTC (permalink / raw)
To: Nishanth Menon; +Cc: linux-omap
So here's a dumb question, being rather ignorant of CPUfreq on SMP.
Should we be running a CPUfreq instance on both CPUs when they cannot be
scaled independently?
What is being scaled here is actually the cluster (the MPU SS via
dpll_mpu_ck), not an individual CPU. So to me, it only makes sense to
have a an instance of the driver per scalable device, which in this case
is a single MPU SS.
What am I missing?
Kevin
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
2011-05-26 18:10 ` [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Kevin Hilman
@ 2011-05-26 18:36 ` Menon, Nishanth
2011-05-27 5:06 ` Santosh Shilimkar
1 sibling, 0 replies; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-26 18:36 UTC (permalink / raw)
To: Kevin Hilman; +Cc: linux-omap
On Thu, May 26, 2011 at 11:10, Kevin Hilman <khilman@ti.com> wrote:
> So here's a dumb question, being rather ignorant of CPUfreq on SMP.
>
> Should we be running a CPUfreq instance on both CPUs when they cannot be
> scaled independently?
>
> What is being scaled here is actually the cluster (the MPU SS via
> dpll_mpu_ck), not an individual CPU. So to me, it only makes sense to
> have a an instance of the driver per scalable device, which in this case
> is a single MPU SS.
>
> What am I missing?
my understanding from the code is that we have one instance of cpufreq
controllable from either cpu0 or 1.
Regards,
Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
2011-05-26 18:10 ` [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq Kevin Hilman
2011-05-26 18:36 ` Menon, Nishanth
@ 2011-05-27 5:06 ` Santosh Shilimkar
2011-05-27 6:07 ` Menon, Nishanth
1 sibling, 1 reply; 37+ messages in thread
From: Santosh Shilimkar @ 2011-05-27 5:06 UTC (permalink / raw)
To: Kevin Hilman; +Cc: Nishanth Menon, linux-omap
On 5/26/2011 11:40 PM, Kevin Hilman wrote:
> So here's a dumb question, being rather ignorant of CPUfreq on SMP.
>
> Should we be running a CPUfreq instance on both CPUs when they cannot be
> scaled independently?
>
> What is being scaled here is actually the cluster (the MPU SS via
> dpll_mpu_ck), not an individual CPU. So to me, it only makes sense to
> have a an instance of the driver per scalable device, which in this case
> is a single MPU SS.
>
We are running only one instance and for the exact same reason as above.
You are completely right and that's the whole intention of those
CPUMASK two lines in the initialization code.
> What am I missing?
>
Not at all.
Regards
Santosh
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
2011-05-27 5:06 ` Santosh Shilimkar
@ 2011-05-27 6:07 ` Menon, Nishanth
2011-05-27 6:26 ` Santosh Shilimkar
0 siblings, 1 reply; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-27 6:07 UTC (permalink / raw)
To: Santosh Shilimkar; +Cc: Kevin Hilman, linux-omap
On Thu, May 26, 2011 at 22:06, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> On 5/26/2011 11:40 PM, Kevin Hilman wrote:
>>
>> So here's a dumb question, being rather ignorant of CPUfreq on SMP.
>>
>> Should we be running a CPUfreq instance on both CPUs when they cannot be
>> scaled independently?
>>
>> What is being scaled here is actually the cluster (the MPU SS via
>> dpll_mpu_ck), not an individual CPU. So to me, it only makes sense to
>> have a an instance of the driver per scalable device, which in this case
>> is a single MPU SS.
>>
> We are running only one instance and for the exact same reason as above.
> You are completely right and that's the whole intention of those
> CPUMASK two lines in the initialization code.
>
>
>> What am I missing?
>>
> Not at all.
So not have cpufreq driver registered at all for CPU1? Life would be a
lot simpler in omap2-cpufreq.c as a result. but that said, two views:
a) future silicon somewhere ahead might need the current
infrastructure to scale into different tables..
b) as far as userspace sees it - cpu0 and cpu1 exists, cool, *but*
cpu1 is not scalable(no /sys/devices/system/cpu/cpu1/cpufreq.. but
.../cpu1/online is present). Keep in mind that userspace is usually
written chip independent. IMHO registering drivers for both devices do
make sense they reflect what the reality of the system is. 2 cpus
scaling together - why do we want to OMAP "specific" stuff here?
Regards,
Nishanth Menon
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
2011-05-27 6:07 ` Menon, Nishanth
@ 2011-05-27 6:26 ` Santosh Shilimkar
2011-05-27 15:33 ` Turquette, Mike
0 siblings, 1 reply; 37+ messages in thread
From: Santosh Shilimkar @ 2011-05-27 6:26 UTC (permalink / raw)
To: Menon, Nishanth; +Cc: Kevin Hilman, linux-omap
On 5/27/2011 11:37 AM, Menon, Nishanth wrote:
> On Thu, May 26, 2011 at 22:06, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
>> On 5/26/2011 11:40 PM, Kevin Hilman wrote:
>>>
>>> So here's a dumb question, being rather ignorant of CPUfreq on SMP.
>>>
>>> Should we be running a CPUfreq instance on both CPUs when they cannot be
>>> scaled independently?
>>>
>>> What is being scaled here is actually the cluster (the MPU SS via
>>> dpll_mpu_ck), not an individual CPU. So to me, it only makes sense to
>>> have a an instance of the driver per scalable device, which in this case
>>> is a single MPU SS.
>>>
>> We are running only one instance and for the exact same reason as above.
>> You are completely right and that's the whole intention of those
>> CPUMASK two lines in the initialization code.
>>
>>
>>> What am I missing?
>>>
>> Not at all.
>
> So not have cpufreq driver registered at all for CPU1? Life would be a
> lot simpler in omap2-cpufreq.c as a result. but that said, two views:
> a) future silicon somewhere ahead might need the current
> infrastructure to scale into different tables..
> b) as far as userspace sees it - cpu0 and cpu1 exists, cool, *but*
> cpu1 is not scalable(no /sys/devices/system/cpu/cpu1/cpufreq.. but
> .../cpu1/online is present). Keep in mind that userspace is usually
> written chip independent. IMHO registering drivers for both devices do
> make sense they reflect what the reality of the system is. 2 cpus
> scaling together - why do we want to OMAP "specific" stuff here?
>
It's not OMAP specific Nishant.
And this feature is supported by CPUFREQ driver. My Intel machine
uses the same exact scheme for CPUFREQ. It's feature provided
specifically for hardwares with individual CPU scaling
limitation. Instead of CPU's, whole CPU cluster scales
together.
Both CPU's still have same consistent view of all CPUFREQ controls.
But in back-ground, CPU1 is carrying only symbolic links.
We make use of "related/affected cpu" feature which is
supported by generic CPUFREQ driver. Nothing OMAP-specific
here.
And as I said i n other thread, if at all in future we get
CPU's which can scale indepedently, we just need to change
that one line where we specify the relation between CPU's.
It's as trivial as that.
Regards
Santosh
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
2011-05-27 6:26 ` Santosh Shilimkar
@ 2011-05-27 15:33 ` Turquette, Mike
2011-05-27 23:27 ` Kevin Hilman
0 siblings, 1 reply; 37+ messages in thread
From: Turquette, Mike @ 2011-05-27 15:33 UTC (permalink / raw)
To: Santosh Shilimkar; +Cc: Menon, Nishanth, Kevin Hilman, linux-omap
On Fri, May 27, 2011 at 1:26 AM, Santosh Shilimkar
<santosh.shilimkar@ti.com> wrote:
> On 5/27/2011 11:37 AM, Menon, Nishanth wrote:
>>
>> On Thu, May 26, 2011 at 22:06, Santosh Shilimkar
>> <santosh.shilimkar@ti.com> wrote:
>>>
>>> On 5/26/2011 11:40 PM, Kevin Hilman wrote:
>>>>
>>>> So here's a dumb question, being rather ignorant of CPUfreq on SMP.
>>>>
>>>> Should we be running a CPUfreq instance on both CPUs when they cannot be
>>>> scaled independently?
>>>>
>>>> What is being scaled here is actually the cluster (the MPU SS via
>>>> dpll_mpu_ck), not an individual CPU. So to me, it only makes sense to
>>>> have a an instance of the driver per scalable device, which in this case
>>>> is a single MPU SS.
>>>>
>>> We are running only one instance and for the exact same reason as above.
>>> You are completely right and that's the whole intention of those
>>> CPUMASK two lines in the initialization code.
>>>
>>>
>>>> What am I missing?
>>>>
>>> Not at all.
>>
>> So not have cpufreq driver registered at all for CPU1? Life would be a
>> lot simpler in omap2-cpufreq.c as a result. but that said, two views:
>> a) future silicon somewhere ahead might need the current
>> infrastructure to scale into different tables..
>> b) as far as userspace sees it - cpu0 and cpu1 exists, cool, *but*
>> cpu1 is not scalable(no /sys/devices/system/cpu/cpu1/cpufreq.. but
>> .../cpu1/online is present). Keep in mind that userspace is usually
>> written chip independent. IMHO registering drivers for both devices do
>> make sense they reflect what the reality of the system is. 2 cpus
>> scaling together - why do we want to OMAP "specific" stuff here?
>>
> It's not OMAP specific Nishant.
> And this feature is supported by CPUFREQ driver. My Intel machine
> uses the same exact scheme for CPUFREQ. It's feature provided
> specifically for hardwares with individual CPU scaling
> limitation. Instead of CPU's, whole CPU cluster scales
> together.
>
> Both CPU's still have same consistent view of all CPUFREQ controls.
> But in back-ground, CPU1 is carrying only symbolic links.
>
> We make use of "related/affected cpu" feature which is
> supported by generic CPUFREQ driver. Nothing OMAP-specific
> here.
Santosh is referring to this code in our cpufreq driver:
/*
* On OMAP SMP configuartion, both processors share the voltage
* and clock. So both CPUs needs to be scaled together and hence
* needs software co-ordination. Use cpufreq affected_cpus
* interface to handle this scenario. Additional is_smp() check
* is to keep SMP_ON_UP build working.
*/
if (is_smp()) {
policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
cpumask_or(cpumask, cpumask_of(policy->cpu), cpumask);
cpumask_copy(policy->cpus, cpumask);
}
policy->cpus knows about each CPU now (in fact, due to this you will
see /sys/devices/system/cpu/cpu1/cpufreq is in fact a symlink to its
cpu0 sibling!)
This is pretty good in fact, since governors like ondemand take into
consideration *all* of the CPUs in policy->cpus:
/* Get Absolute Load - in terms of freq */
max_load_freq = 0; <- tracks greatest need across all CPUs
for_each_cpu(j, policy->cpus) {
... find max_load_freq ...
Ultimate effect is that we run a single workqueue only on CPU0
(kondemand or whatever) that takes the load characteristics of both
CPU0 and CPU1 into account.
Regards,
Mike
> And as I said i n other thread, if at all in future we get
> CPU's which can scale indepedently, we just need to change
> that one line where we specify the relation between CPU's.
> It's as trivial as that.
>
> Regards
> Santosh
>
>
>
>
> --
> 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
>
--
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
2011-05-27 15:33 ` Turquette, Mike
@ 2011-05-27 23:27 ` Kevin Hilman
2011-05-29 17:25 ` Menon, Nishanth
0 siblings, 1 reply; 37+ messages in thread
From: Kevin Hilman @ 2011-05-27 23:27 UTC (permalink / raw)
To: Turquette, Mike; +Cc: Santosh Shilimkar, Menon, Nishanth, linux-omap
"Turquette, Mike" <mturquette@ti.com> writes:
> On Fri, May 27, 2011 at 1:26 AM, Santosh Shilimkar
> <santosh.shilimkar@ti.com> wrote:
>> On 5/27/2011 11:37 AM, Menon, Nishanth wrote:
>>>
>>> On Thu, May 26, 2011 at 22:06, Santosh Shilimkar
>>> <santosh.shilimkar@ti.com> wrote:
>>>>
>>>> On 5/26/2011 11:40 PM, Kevin Hilman wrote:
>>>>>
>>>>> So here's a dumb question, being rather ignorant of CPUfreq on SMP.
>>>>>
>>>>> Should we be running a CPUfreq instance on both CPUs when they cannot be
>>>>> scaled independently?
>>>>>
>>>>> What is being scaled here is actually the cluster (the MPU SS via
>>>>> dpll_mpu_ck), not an individual CPU. So to me, it only makes sense to
>>>>> have a an instance of the driver per scalable device, which in this case
>>>>> is a single MPU SS.
>>>>>
>>>> We are running only one instance and for the exact same reason as above.
>>>> You are completely right and that's the whole intention of those
>>>> CPUMASK two lines in the initialization code.
>>>>
>>>>
>>>>> What am I missing?
>>>>>
>>>> Not at all.
>>>
>>> So not have cpufreq driver registered at all for CPU1? Life would be a
>>> lot simpler in omap2-cpufreq.c as a result. but that said, two views:
>>> a) future silicon somewhere ahead might need the current
>>> infrastructure to scale into different tables..
>>> b) as far as userspace sees it - cpu0 and cpu1 exists, cool, *but*
>>> cpu1 is not scalable(no /sys/devices/system/cpu/cpu1/cpufreq.. but
>>> .../cpu1/online is present). Keep in mind that userspace is usually
>>> written chip independent. IMHO registering drivers for both devices do
>>> make sense they reflect what the reality of the system is. 2 cpus
>>> scaling together - why do we want to OMAP "specific" stuff here?
>>>
>> It's not OMAP specific Nishant.
>> And this feature is supported by CPUFREQ driver. My Intel machine
>> uses the same exact scheme for CPUFREQ. It's feature provided
>> specifically for hardwares with individual CPU scaling
>> limitation. Instead of CPU's, whole CPU cluster scales
>> together.
>>
>> Both CPU's still have same consistent view of all CPUFREQ controls.
>> But in back-ground, CPU1 is carrying only symbolic links.
>>
>> We make use of "related/affected cpu" feature which is
>> supported by generic CPUFREQ driver. Nothing OMAP-specific
>> here.
>
> Santosh is referring to this code in our cpufreq driver:
>
> /*
> * On OMAP SMP configuartion, both processors share the voltage
> * and clock. So both CPUs needs to be scaled together and hence
> * needs software co-ordination. Use cpufreq affected_cpus
> * interface to handle this scenario. Additional is_smp() check
> * is to keep SMP_ON_UP build working.
> */
> if (is_smp()) {
> policy->shared_type = CPUFREQ_SHARED_TYPE_ANY;
> cpumask_or(cpumask, cpumask_of(policy->cpu), cpumask);
> cpumask_copy(policy->cpus, cpumask);
> }
>
> policy->cpus knows about each CPU now (in fact, due to this you will
> see /sys/devices/system/cpu/cpu1/cpufreq is in fact a symlink to its
> cpu0 sibling!)
>
> This is pretty good in fact, since governors like ondemand take into
> consideration *all* of the CPUs in policy->cpus:
>
> /* Get Absolute Load - in terms of freq */
> max_load_freq = 0; <- tracks greatest need across all CPUs
>
> for_each_cpu(j, policy->cpus) {
> ... find max_load_freq ...
>
> Ultimate effect is that we run a single workqueue only on CPU0
> (kondemand or whatever) that takes the load characteristics of both
> CPU0 and CPU1 into account.
OK, makes sense. Thanks for the description.
All of this came up because this series is going through contortions to
make two CPUfreq registrations only using one freq_table, protect
against concurrent access from different CPUs etc., which led me to
wonder why we need two registrations.
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
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
2011-05-27 23:27 ` Kevin Hilman
@ 2011-05-29 17:25 ` Menon, Nishanth
0 siblings, 0 replies; 37+ messages in thread
From: Menon, Nishanth @ 2011-05-29 17:25 UTC (permalink / raw)
To: Kevin Hilman; +Cc: Turquette, Mike, Santosh Shilimkar, linux-omap
On Fri, May 27, 2011 at 18:27, Kevin Hilman <khilman@ti.com> wrote:
> All of this came up because this series is going through contortions to
> make two CPUfreq registrations only using one freq_table, protect
> against concurrent access from different CPUs etc., which led me to
> wonder why we need two registrations.
I believe we have two cpu_inits and exits per cpu -> the
/sys/devices/system/cpu/cpu1/online and
/sys/devices/system/cpu/cpu0/online
are not soft links. this is where the issue starts
Try this diff on the pm-wip/cpufreq branch:
diff --git a/arch/arm/mach-omap2/omap2plus-cpufreq.c
b/arch/arm/mach-omap2/omap2plus-cpufreq.c
index 33a91ec..f3e82ce 100644
--- a/arch/arm/mach-omap2/omap2plus-cpufreq.c
+++ b/arch/arm/mach-omap2/omap2plus-cpufreq.c
@@ -199,11 +199,15 @@ static int __cpuinit omap_cpu_init(struct
cpufreq_policy *policy)
/* FIXME: what's the actual transition time? */
policy->cpuinfo.transition_latency = 300 * 1000;
+ dev_err(mpu_dev, "%s: XXX: cpu%d freq_table pointer=%p\n", __func__,
+ policy->cpu, freq_table);
return 0;
}
static int omap_cpu_exit(struct cpufreq_policy *policy)
{
+ dev_err(mpu_dev, "%s: XXX: cpu%d freq_table pointer=%p\n", __func__,
+ policy->cpu, freq_table);
clk_exit_cpufreq_table(&freq_table);
clk_put(mpu_clk);
return 0;
[ 2.045623] platform mpu.0: omap_cpu_init: XXX: cpu0 freq_table
pointer=eeff7e20
[ 2.055664] platform mpu.0: omap_cpu_init: XXX: cpu1 freq_table
pointer=eeff7de0 <- Freq table eeff7e20 got overwritten by the new
alloc
[ 2.063537] platform mpu.0: omap_cpu_exit: XXX: cpu1 freq_table
pointer=eeff7de0 <- if I had a kfree/ opp_freqtable_free in exit, we'd
have had a dangling pointer.
in this series:
patch 1: OMAP2+: cpufreq: dont support !freq_table -> This is a proper
fix to cleanup the code which seems to think that silicon with no
freq_table is to be supported - this is a hang over from OMAP1 time
and should be removed.
patch 2: OMAP2+: cpufreq: use OPP library -> Since we are using OPP
table and from your recommendation of a previous patch incarnation, I
have moved the cpufreq usage depdendent on OPP.
Patch 3: OMAP2+: cpufreq: put clk if cpu_init failed -> this is a
valid fix as well
Patch 4: OMAP2+: cpufreq: fix freq_table leak -> This is what I have
explained above -> Since online variables are not really a softlink, I
dont think we should be confused as to what the fix should look like!
table creation and registration is done as part of cpu_init - this is
probably the most appropriate place for it. but we should consider the
potential of cpu onlining and offlining dynamically in the system as
well. hence the need for patch 4 where I have used freq_table users.
Regards,
Nishanth Menon
--
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
^ permalink raw reply related [flat|nested] 37+ messages in thread