Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
On Fri, May 27, 2011 at 18:27, Kevin Hilman 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
"Turquette, Mike" writes:
> On Fri, May 27, 2011 at 1:26 AM, Santosh Shilimkar
> wrote:
>> On 5/27/2011 11:37 AM, Menon, Nishanth wrote:
>>>
>>> On Thu, May 26, 2011 at 22:06, Santosh Shilimkar
>>> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
On Fri, May 27, 2011 at 1:26 AM, Santosh Shilimkar
wrote:
> On 5/27/2011 11:37 AM, Menon, Nishanth wrote:
>>
>> On Thu, May 26, 2011 at 22:06, Santosh Shilimkar
>> 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 majord...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
On 5/27/2011 11:37 AM, Menon, Nishanth wrote: On Thu, May 26, 2011 at 22:06, Santosh Shilimkar 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
On Thu, May 26, 2011 at 22:06, Santosh Shilimkar 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
On Thu, May 26, 2011 at 11:10, 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. > > 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PM-WIP_CPUFREQ][PATCH 0/6 V3] Cleanups for cpufreq
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
