[PATCH] cpufreq: Align all CPUs to the same frequency if using shared clock

[lizhuangzhi]

Some SMP systems want to make all the possible CPUs share the clock,
if the CPUs init frequencies aren't the same, we need to align all the
CPUs to the same frequency while CPUs registing to avoid mismatched
CPU's P-states.

Signed-off-by: lizhuangzhi <zhuang...@intel.com>
---
drivers/cpufreq/cpufreq.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 8d19f7c..d00abb5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -991,6 +991,8 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
* CPU because it is in the same boat. */
policy = cpufreq_cpu_get(cpu);
if (unlikely(policy)) {
+ /* according present policy to align all the cpus frequencies */
+ cpufreq_driver->target(policy, policy->cur, CPUFREQ_RELATION_H);
cpufreq_cpu_put(policy);
return 0;
}
--
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Viresh Kumar]

On 21 January 2014 08:35, lizhuangzhi <zhuang...@intel.com> wrote:
> Some SMP systems want to make all the possible CPUs share the clock,
> if the CPUs init frequencies aren't the same, we need to align all the
> CPUs to the same frequency while CPUs registing to avoid mismatched
> CPU's P-states.
>
> Signed-off-by: lizhuangzhi <zhuang...@intel.com>
> ---
> drivers/cpufreq/cpufreq.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 8d19f7c..d00abb5 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -991,6 +991,8 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
> * CPU because it is in the same boat. */
> policy = cpufreq_cpu_get(cpu);
> if (unlikely(policy)) {
> + /* according present policy to align all the cpus frequencies */
> + cpufreq_driver->target(policy, policy->cur, CPUFREQ_RELATION_H);

I don't really understand why is this required? CPUs sharing clocks means
that CPUs runs on the same clock line and so all of them must be running
on same frequency. So, why do we need to call this routine? policy->cur
must be the current freq here for CPU in question.

--
viresh

[Li, Zhuangzhi]

Thanks for reviewing.

Sorry for make you misunderstanding, on our x86 platform, we want all the CPUs share one policy by setting CPUFREQ_SHARED_TYPE_ALL, not share one HW clock line.
If the CPUs work at different frequency at init stage, then while the CPUs registering, no policy to align all the CPUs to the same frequency, this caused some conflicts with current CPUs P-state.

For example:
CPU0: P0
CPU1: P14
CPU2: P14
CPU3: P14
During all the CPUs registering, kernel considers all the CPUs work at P0 state because of sharing CPU0 policy, but the other three CPUs really work at P14 state,
all CPUs frequency only wait to be aligned until CPU0 state changed by governor based on shared policy.

Best Regards

Li zhuangzhi
Android System Integration Shanghai
Tel: +86 (0)21 6116 4323

> + CPUFREQ_RELATION_H);

[Viresh Kumar]

On 21 January 2014 12:56, Li, Zhuangzhi <zhuang...@intel.com> wrote:
> Thanks for reviewing.

Its my job :)

> Sorry for make you misunderstanding, on our x86 platform, we want all the CPUs share one policy by setting CPUFREQ_SHARED_TYPE_ALL, not share one HW clock line.

I see.. Then probably your patch makes sense. But it is
obviously not required for every platform that exists today.

Please update it to do it only for drivers that have set
CPUFREQ_SHARED_TYPE_ALL..

@Rafael: I hope you agree?

[Viresh Kumar]

On 21 January 2014 13:42, Viresh Kumar <viresh...@linaro.org> wrote:
> On 21 January 2014 12:56, Li, Zhuangzhi <zhuang...@intel.com> wrote:
>> Thanks for reviewing.
>
> Its my job :)
>
>> Sorry for make you misunderstanding, on our x86 platform, we want all the CPUs share one policy by setting CPUFREQ_SHARED_TYPE_ALL, not share one HW clock line.
>
> I see.. Then probably your patch makes sense. But it is
> obviously not required for every platform that exists today.
>
> Please update it to do it only for drivers that have set
> CPUFREQ_SHARED_TYPE_ALL..

One more thing, who has set different frequencies to these cores?
I hope kernel hasn't ?

In that case, probably you are fixing a bootloader bug in kernel?
What about doing this in bootloader then?

--
virehs

[Li, Zhuangzhi]

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh...@linaro.org]
> Sent: Wednesday, January 22, 2014 1:18 PM
> To: Li, Zhuangzhi
> Cc: Rafael J. Wysocki; cpu...@vger.kernel.org; linu...@vger.kernel.org;
> Linux Kernel Mailing List; Liu, Chuansheng
> Subject: Re: [PATCH] cpufreq: Align all CPUs to the same frequency if using
> shared clock
>

> On 21 January 2014 13:42, Viresh Kumar <viresh...@linaro.org> wrote:
> > On 21 January 2014 12:56, Li, Zhuangzhi <zhuang...@intel.com> wrote:
> >> Thanks for reviewing.
> >
> > Its my job :)
> >
> >> Sorry for make you misunderstanding, on our x86 platform, we want all the
> CPUs share one policy by setting CPUFREQ_SHARED_TYPE_ALL, not share one
> HW clock line.
> >
> > I see.. Then probably your patch makes sense. But it is obviously not
> > required for every platform that exists today.
> >
> > Please update it to do it only for drivers that have set
> > CPUFREQ_SHARED_TYPE_ALL..
>
> One more thing, who has set different frequencies to these cores?
> I hope kernel hasn't ?
>
> In that case, probably you are fixing a bootloader bug in kernel?
> What about doing this in bootloader then?

I don't think it's a real bug in bootloader, the bootloader can set CPUs to different frequencies according to actually requirements(Power saving first or Performance first),
the CPUs freq policy are initialized in kernel, if the kernel want to share one CPU policy(using CPUFREQ_SHARED_TYPE_ALL type), it should ensure all CPUs frequencies aligned first,
don't depend on the bootloader CPUs Pre-states, then the kernel can have better compatibility.

If the kernel uses CPUFREQ_SHARED_TYPE_ALL policy, the patch can ensure these:
1. If all CPUs are in the same P-state, it does nothing when cpufreq registering
2. If the CPUs are in different P-states, all the other CPUs are aligned once to current frequency of CPU0 according to the present policy.

[Viresh Kumar]

On 22 January 2014 11:56, Li, Zhuangzhi <zhuang...@intel.com> wrote:
> I don't think it's a real bug in bootloader, the bootloader can set CPUs to different frequencies according to actually requirements(Power saving first or Performance first),
> the CPUs freq policy are initialized in kernel, if the kernel want to share one CPU policy(using CPUFREQ_SHARED_TYPE_ALL type), it should ensure all CPUs frequencies aligned first,
> don't depend on the bootloader CPUs Pre-states, then the kernel can have better compatibility.
>
> If the kernel uses CPUFREQ_SHARED_TYPE_ALL policy, the patch can ensure these:
> 1. If all CPUs are in the same P-state, it does nothing when cpufreq registering
> 2. If the CPUs are in different P-states, all the other CPUs are aligned once to current frequency of CPU0 according to the present policy.

I thought, as you are asking kernel to keep same freq on all of them,
then same should be true for bootloaders.

Otherwise it was okay.

[Li, Zhuangzhi]

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh...@linaro.org]
> Sent: Wednesday, January 22, 2014 2:43 PM
> To: Li, Zhuangzhi
> Cc: Rafael J. Wysocki; cpu...@vger.kernel.org; linu...@vger.kernel.org;
> Linux Kernel Mailing List; Liu, Chuansheng
> Subject: Re: [PATCH] cpufreq: Align all CPUs to the same frequency if using
> shared clock
>

> On 22 January 2014 11:56, Li, Zhuangzhi <zhuang...@intel.com> wrote:
> > I don't think it's a real bug in bootloader, the bootloader can set
> > CPUs to different frequencies according to actually requirements(Power
> > saving first or Performance first), the CPUs freq policy are initialized in kernel,
> if the kernel want to share one CPU policy(using CPUFREQ_SHARED_TYPE_ALL
> type), it should ensure all CPUs frequencies aligned first, don't depend on the
> bootloader CPUs Pre-states, then the kernel can have better compatibility.
> >
> > If the kernel uses CPUFREQ_SHARED_TYPE_ALL policy, the patch can ensure
> these:
> > 1. If all CPUs are in the same P-state, it does nothing when cpufreq
> > registering 2. If the CPUs are in different P-states, all the other CPUs are
> aligned once to current frequency of CPU0 according to the present policy.
>
> I thought, as you are asking kernel to keep same freq on all of them, then same
> should be true for bootloaders.
>
> Otherwise it was okay.

Yes, but most of the platforms(ARM&x86) only enable one CPU Core in bootloader or Bios because of multiple threads(SMP) are not supported,
and the other Cores stay in default configuration for saving power in bootloader, kernel is responsible for booting up the other Cores from default frequencies for SMP.
Cpufreq driver need to init them to required P-state before governor active when using CPUFREQ_SHARED_TYPE_ALL policy.