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.