[RFC PATCH] cpufreq: Calling init() of cpufreq_driver when policy inactive cpu online

[RFC PATCH] cpufreq: Calling init() of cpufreq_driver when policy inactive cpu online

[Shunyong Yang]

When multiple cpus are related in one cpufreq policy, the first online cpu
will be chosen by default to handle cpufreq operations. In a CPPC case,
let's take two related cpus, cpu0 and cpu1 as an example.

After system start, cpu0 is the first online cpu. Cpufreq policy will be
allocated and init() in cpufreq_driver will be called to initialize cpu0's
perf capabilities and policy parameters. When cpu1 is online, current code
will not call init() in cpufreq_driver as policy has been allocated and
activated by cpu0. So, cpu1's perf capabilities are not initialized
(all 0s).

When cpu0 is offline, policy->cpu will be shifted to cpu1. As cpu1's perf
capabilities are 0s, speed change will not take effect when setting
speed.

This patch adds calling init() of cpufreq_driver when policy inactive cpu
comes to online. Moreover, perf capabilities of all online cpus are
initialized when init() is called.

This patch is tested on CPPC enabled system. I am not sure it's influnce
on other cpufreq_driver. So, this RFC is sent for comments.

Cc: Wang Dongsheng <dongsheng.wang@hxt-semitech.com>
Cc: Joey Zheng <yu.zheng@hxt-semitech.com>
Signed-off-by: Shunyong Yang <shunyong.yang@hxt-semitech.com>
---
 drivers/cpufreq/cppc_cpufreq.c | 45 ++++++++++++++++++++++++++++++------------
 drivers/cpufreq/cpufreq.c      | 18 ++++++++++++++++-
 2 files changed, 49 insertions(+), 14 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index a1c3025f9df7..f23a2007dd66 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -125,23 +125,12 @@ static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
 				cpu->perf_caps.lowest_perf, cpu_num, ret);
 }
 
-static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
+static int cppc_cpufreq_update_policy(struct cpufreq_policy *policy,
+				      struct cppc_cpudata *cpu)
 {
-	struct cppc_cpudata *cpu;
 	unsigned int cpu_num = policy->cpu;
 	int ret = 0;
 
-	cpu = all_cpu_data[policy->cpu];
-
-	cpu->cpu = cpu_num;
-	ret = cppc_get_perf_caps(policy->cpu, &cpu->perf_caps);
-
-	if (ret) {
-		pr_debug("Err reading CPU%d perf capabilities. ret:%d\n",
-				cpu_num, ret);
-		return ret;
-	}
-
 	cppc_dmi_max_khz = cppc_get_dmi_max_khz();
 
 	/*
@@ -186,6 +175,36 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	return ret;
 }
 
+static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
+{
+	struct cppc_cpudata *cpu;
+	unsigned int cpu_num;
+	int ret = 0;
+
+	for_each_cpu(cpu_num, policy->cpus) {
+		cpu = all_cpu_data[cpu_num];
+
+		cpu->cpu = cpu_num;
+		ret = cppc_get_perf_caps(cpu_num, &cpu->perf_caps);
+		if (ret) {
+			pr_debug("Err reading CPU%d perf capabilities. ret:%d\n",
+				cpu_num, ret);
+			return ret;
+		}
+
+		if (policy->cpu == cpu_num) {
+			ret = cppc_cpufreq_update_policy(policy, cpu);
+			if (ret) {
+				pr_debug("Err update CPU%d perf capabilities. ret:%d\n",
+					cpu_num, ret);
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
 static struct cpufreq_driver cppc_cpufreq_driver = {
 	.flags = CPUFREQ_CONST_LOOPS,
 	.verify = cppc_verify_policy,
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 239063fb6afc..3317c5e55e7f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1192,8 +1192,24 @@ static int cpufreq_online(unsigned int cpu)
 	policy = per_cpu(cpufreq_cpu_data, cpu);
 	if (policy) {
 		WARN_ON(!cpumask_test_cpu(cpu, policy->related_cpus));
-		if (!policy_is_inactive(policy))
+		if (!policy_is_inactive(policy)) {
+			/*
+			 * Parameters of policy inactive CPU should be
+			 * initialized here to make cpufreq work correctly
+			 * when policy active CPU is switched to offline.
+			 * When initialization failed, goto out_destroy_policy
+			 * to destroy.
+			 */
+			if (cpu != policy->cpu) {
+				ret = cpufreq_driver->init(policy);
+				if (ret) {
+					pr_debug("inactive cpu initialization failed\n");
+					goto out_destroy_policy;
+				}
+			}
+
 			return cpufreq_add_policy_cpu(policy, cpu);
+		}
 
 		/* This is the only online CPU for the policy.  Start over. */
 		new_policy = false;
-- 
1.8.3.1

Re: [RFC PATCH] cpufreq: Calling init() of cpufreq_driver when policy inactive cpu online

[Viresh Kumar]

On 21-03-18, 18:21, Shunyong Yang wrote:
> When multiple cpus are related in one cpufreq policy, the first online cpu
> will be chosen by default to handle cpufreq operations. In a CPPC case,
> let's take two related cpus, cpu0 and cpu1 as an example.
> 
> After system start, cpu0 is the first online cpu. Cpufreq policy will be
> allocated and init() in cpufreq_driver will be called to initialize cpu0's
> perf capabilities and policy parameters.

Not exactly. The init() is called to initialize stuff for all the CPUs that
should be part of policy->related_cpus after init() has returned. So you should
initialize perf capabilities for all of them.

> When cpu1 is online, current code
> will not call init() in cpufreq_driver as policy has been allocated and
> activated by cpu0. So, cpu1's perf capabilities are not initialized
> (all 0s).
> 
> When cpu0 is offline, policy->cpu will be shifted to cpu1. As cpu1's perf
> capabilities are 0s, speed change will not take effect when setting
> speed.
> 
> This patch adds calling init() of cpufreq_driver when policy inactive cpu
> comes to online.

No CPU should be inactive here, its just that you haven't initialized it
properly.

And we are not going to call init() multiple times for a group of CPUs. That's
not what the purpose of init() is.

-- 
viresh

Re: [RFC PATCH] cpufreq: Calling init() of cpufreq_driver when policy inactive cpu online

[Yang, Shunyong]

Hi, Kumar

On Thu, 2018-03-22 at 11:30 +0800, Viresh Kumar wrote:
> On 21-03-18, 18:21, Shunyong Yang wrote:
> > 
> > When multiple cpus are related in one cpufreq policy, the first
> > online cpu
> > will be chosen by default to handle cpufreq operations. In a CPPC
> > case,
> > let's take two related cpus, cpu0 and cpu1 as an example.
> > 
> > After system start, cpu0 is the first online cpu. Cpufreq policy
> > will be
> > allocated and init() in cpufreq_driver will be called to initialize
> > cpu0's
> > perf capabilities and policy parameters.
> Not exactly. The init() is called to initialize stuff for all the
> CPUs that
> should be part of policy->related_cpus after init() has returned. So
> you should
> initialize perf capabilities for all of them.

In page 533 of ACPI 6.2 specificaiton, it says,

"Starting with ACPI Specification 6.2, all _CPC registers can be in
PCC, System Memory, System IO, or Functional Fixed Hardware address
spaces. OSPM support for this more flexible register space scheme is
indicated by the “Flexible Address Space for CPPC Registers” _OSC bit."

As _CPC register maybe in System Memory, System IO, or Functional Fixed
Hardware address spaces. I am not sure all architecture implementing
CPPC can return correct value before CPU come into online. That's the
reason I add the extra init() call.

BTW, I've tested on QDF2400 platform and it return correct value when
cpu1 is offline.

Do you know whether firmware can guarantee correct perf capabilities
regardless of CPU online/offline?

> 
> > 
> > When cpu1 is online, current code
> > will not call init() in cpufreq_driver as policy has been allocated
> > and
> > activated by cpu0. So, cpu1's perf capabilities are not initialized
> > (all 0s).
> > 
> > When cpu0 is offline, policy->cpu will be shifted to cpu1. As
> > cpu1's perf
> > capabilities are 0s, speed change will not take effect when setting
> > speed.
> > 
> > This patch adds calling init() of cpufreq_driver when policy
> > inactive cpu
> > comes to online.
> No CPU should be inactive here, its just that you haven't initialized
> it
> properly.
> 

I mean the policy is handled(active) by the first online cpu's (cpu0)
perf capabilities. Not handled (inactive) by the one's just come into
online (cpu1). Sorry for this.

Thanks.
Shunyong.


> And we are not going to call init() multiple times for a group of
> CPUs. That's
> not what the purpose of init() is.
> 

Re: [RFC PATCH] cpufreq: Calling init() of cpufreq_driver when policy inactive cpu online

[Yang, Shunyong]

Hi, Kumar,

On Wed, 2018-03-21 at 22:35 -0700, Yang, Shunyong wrote:
> Hi, Kumar
> 
> On Thu, 2018-03-22 at 11:30 +0800, Viresh Kumar wrote:
> > 
> > On 21-03-18, 18:21, Shunyong Yang wrote:
> > > 
> > > 
> > > When multiple cpus are related in one cpufreq policy, the first
> > > online cpu
> > > will be chosen by default to handle cpufreq operations. In a CPPC
> > > case,
> > > let's take two related cpus, cpu0 and cpu1 as an example.
> > > 
> > > After system start, cpu0 is the first online cpu. Cpufreq policy
> > > will be
> > > allocated and init() in cpufreq_driver will be called to
> > > initialize
> > > cpu0's
> > > perf capabilities and policy parameters.
> > Not exactly. The init() is called to initialize stuff for all the
> > CPUs that
> > should be part of policy->related_cpus after init() has returned.
> > So
> > you should
> > initialize perf capabilities for all of them.

Thanks for your review.
As current CPPC only supports CPUFREQ_SHARED_TYPE_ANY. And I think this
is the case for most systems. 
According to your suggestion to initialize all performance capabilitis
in one init() call, I want to change to only copy the online cpu's
performance capabilities to other shared cpus. And I tested on QDF2400
platform, it works well.

Could you please have comments on this? 


diff --git a/drivers/cpufreq/cppc_cpufreq.c
b/drivers/cpufreq/cppc_cpufreq.c
index a1c3025f9df7..e472e887e91e 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -164,8 +164,18 @@ static int cppc_cpufreq_cpu_init(struct
cpufreq_policy *policy)
        policy->cpuinfo.transition_latency =
cppc_get_transition_latency(cpu_num);
        policy->shared_type = cpu->shared_type;

-       if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY)
+       if (policy->shared_type == CPUFREQ_SHARED_TYPE_ANY) {
+               int i;
+
                cpumask_copy(policy->cpus, cpu->shared_cpu_map);
+
+               for_each_cpu(i, policy->cpus) {
+                       if (i != policy->cpu)
+                               memcpy(&all_cpu_data[i]->perf_caps,
+                                      &cpu->perf_caps,
+                                      sizeof(cpu->perf_caps));
+               }
+       }
        else if (policy->shared_type == CPUFREQ_SHARED_TYPE_ALL) {
                /* Support only SW_ANY for now. */
                pr_debug("Unsupported CPU co-ord type\n");

Thanks.
Shunyong.


> In page 533 of ACPI 6.2 specificaiton, it says,
> 
> "Starting with ACPI Specification 6.2, all _CPC registers can be in
> PCC, System Memory, System IO, or Functional Fixed Hardware address
> spaces. OSPM support for this more flexible register space scheme is
> indicated by the “Flexible Address Space for CPPC Registers” _OSC
> bit."
> 
> As _CPC register maybe in System Memory, System IO, or Functional
> Fixed
> Hardware address spaces. I am not sure all architecture implementing
> CPPC can return correct value before CPU come into online. That's the
> reason I add the extra init() call.
> 
> BTW, I've tested on QDF2400 platform and it return correct value when
> cpu1 is offline.
> 
> Do you know whether firmware can guarantee correct perf capabilities
> regardless of CPU online/offline?
> 
> > 
> > 
> > > 
> > > 
> > > When cpu1 is online, current code
> > > will not call init() in cpufreq_driver as policy has been
> > > allocated
> > > and
> > > activated by cpu0. So, cpu1's perf capabilities are not
> > > initialized
> > > (all 0s).
> > > 
> > > When cpu0 is offline, policy->cpu will be shifted to cpu1. As
> > > cpu1's perf
> > > capabilities are 0s, speed change will not take effect when
> > > setting
> > > speed.
> > > 
> > > This patch adds calling init() of cpufreq_driver when policy
> > > inactive cpu
> > > comes to online.
> > No CPU should be inactive here, its just that you haven't
> > initialized
> > it
> > properly.
> > 
> I mean the policy is handled(active) by the first online cpu's (cpu0)
> perf capabilities. Not handled (inactive) by the one's just come into
> online (cpu1). Sorry for this.
> 
> Thanks.
> Shunyong.
> 
> 
> > 
> > And we are not going to call init() multiple times for a group of
> > CPUs. That's
> > not what the purpose of init() is.