Messages in this thread Patch in this message | | | From | "Yang, Shunyong" <> | Subject | Re: [RFC PATCH] cpufreq: Calling init() of cpufreq_driver when policy inactive cpu online | Date | Sat, 24 Mar 2018 09:37:37 +0000 |
| |
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. | |