cpufreq_resume warning issue

cpufreq_resume warning issue

[Zengtao (B)]

Hi all:
	I have got the warning in the following code, 
	/*
	 * schedule call cpufreq_update_policy() for first-online CPU, as that
	 * wouldn't be hotplugged-out on suspend. It will verify that the
	 * current freq is in sync with what we believe it to be.
	 */
	policy = cpufreq_cpu_get_raw(cpumask_first(cpu_online_mask));
	if (WARN_ON(!policy))
		return;
	schedule_work(&policy->update);

in my platform, two cores, core 1 don't have the dvfs feature, but core 2 has.
So I think that is the warning reason.

My question:
1. Do we need to warning here? 
2. in my case, the schedule_work will be skipped, is there any problem? 

Thank you.

BR
Zengtao 

Re: cpufreq_resume warning issue

[Viresh Kumar]

On 21-03-16, 03:43, Zengtao (B) wrote:
> Hi all:
> 	I have got the warning in the following code, 
> 	/*
> 	 * schedule call cpufreq_update_policy() for first-online CPU, as that
> 	 * wouldn't be hotplugged-out on suspend. It will verify that the
> 	 * current freq is in sync with what we believe it to be.
> 	 */
> 	policy = cpufreq_cpu_get_raw(cpumask_first(cpu_online_mask));
> 	if (WARN_ON(!policy))
> 		return;
> 	schedule_work(&policy->update);
> 
> in my platform, two cores, core 1 don't have the dvfs feature, but core 2 has.
> So I think that is the warning reason.
> 
> My question:
> 1. Do we need to warning here? 

Yes. But your system just broke an assumption we always had. i.e. all the CPUs
take part in DVFS :)

And its not just about this piece of code, but everything else that cpufreq
does.

For example, cpufreq core must be calling ->init() callback for CPU0 as well..
What are you doing there ?

I am not sure what's the right way of handling this thing is going to be.

Why doesn't you have DVFS for both the cores ?

-- 
viresh

RE: cpufreq_resume warning issue

[Zengtao (B)]

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: Monday, March 21, 2016 3:32 PM
> To: Zengtao (B)
> Cc: rjw@rjwysocki.net; linux-pm@vger.kernel.org
> Subject: Re: cpufreq_resume warning issue
> 
> On 21-03-16, 03:43, Zengtao (B) wrote:
> > Hi all:
> > 	I have got the warning in the following code,
> > 	/*
> > 	 * schedule call cpufreq_update_policy() for first-online CPU, as that
> > 	 * wouldn't be hotplugged-out on suspend. It will verify that the
> > 	 * current freq is in sync with what we believe it to be.
> > 	 */
> > 	policy = cpufreq_cpu_get_raw(cpumask_first(cpu_online_mask));
> > 	if (WARN_ON(!policy))
> > 		return;
> > 	schedule_work(&policy->update);
> >
> > in my platform, two cores, core 1 don't have the dvfs feature, but core 2 has.
> > So I think that is the warning reason.
> >
> > My question:
> > 1. Do we need to warning here?
> 
> Yes. But your system just broke an assumption we always had. i.e. all the CPUs
> take part in DVFS :)
> 
> And its not just about this piece of code, but everything else that cpufreq
> does.
> 
> For example, cpufreq core must be calling ->init() callback for CPU0 as well..
> What are you doing there ?
For CPU0, just returnes false for cpufreq_driver->init(policy).

> 
> I am not sure what's the right way of handling this thing is going to be.
>
It looks that the DVFS works well for CPU1 except the Warning message mentioned above.

> Why doesn't you have DVFS for both the cores ?
> 
The CPU0 is low power, not so much benefit from dvfs, so we just design one power-domain for CPU1 to cost down. 
> --
> viresh


BR
Zengtao 

Re: cpufreq_resume warning issue

[Viresh Kumar]

On 21-03-16, 08:05, Zengtao (B) wrote:
> > Why doesn't you have DVFS for both the cores ?
> > 
> The CPU0 is low power, not so much benefit from dvfs, so we just design one power-domain for CPU1 to cost down. 

What CPU is that ? A7?

-- 
viresh

RE: cpufreq_resume warning issue

[Zengtao (B)]

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: Monday, March 21, 2016 4:34 PM
> To: Zengtao (B)
> Cc: rjw@rjwysocki.net; linux-pm@vger.kernel.org
> Subject: Re: cpufreq_resume warning issue
> 
> On 21-03-16, 08:05, Zengtao (B) wrote:
> > > Why doesn't you have DVFS for both the cores ?
> > >
> > The CPU0 is low power, not so much benefit from dvfs, so we just design one
> power-domain for CPU1 to cost down.
> 
> What CPU is that ? A7?
CPU0 is A7 and CPU1 A17.
> 
> --
> viresh

Re: cpufreq_resume warning issue

[Viresh Kumar]

On 21-03-16, 09:07, Zengtao (B) wrote:
> > -----Original Message-----
> > From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> > Sent: Monday, March 21, 2016 4:34 PM
> > To: Zengtao (B)
> > Cc: rjw@rjwysocki.net; linux-pm@vger.kernel.org
> > Subject: Re: cpufreq_resume warning issue
> > 
> > On 21-03-16, 08:05, Zengtao (B) wrote:
> > > > Why doesn't you have DVFS for both the cores ?
> > > >
> > > The CPU0 is low power, not so much benefit from dvfs, so we just design one
> > power-domain for CPU1 to cost down.
> > 
> > What CPU is that ? A7?
> CPU0 is A7 and CPU1 A17.

That's what I expected.

All platforms using A7 use DVFS, its low power, but not that much.

Why don't you do the same? Or is it that your platform doesn't support DVFS at
all ? i.e. the clk/regulators aren't programmable to the processors ?

I doubt that, and so there is something that you need to fix here instead.

-- 
viresh

RE: cpufreq_resume warning issue

[Zengtao (B)]

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: Monday, March 21, 2016 5:09 PM
> To: Zengtao (B)
> Cc: rjw@rjwysocki.net; linux-pm@vger.kernel.org
> Subject: Re: cpufreq_resume warning issue
> 
> On 21-03-16, 09:07, Zengtao (B) wrote:
> > > -----Original Message-----
> > > From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> > > Sent: Monday, March 21, 2016 4:34 PM
> > > To: Zengtao (B)
> > > Cc: rjw@rjwysocki.net; linux-pm@vger.kernel.org
> > > Subject: Re: cpufreq_resume warning issue
> > >
> > > On 21-03-16, 08:05, Zengtao (B) wrote:
> > > > > Why doesn't you have DVFS for both the cores ?
> > > > >
> > > > The CPU0 is low power, not so much benefit from dvfs, so we just design
> one
> > > power-domain for CPU1 to cost down.
> > >
> > > What CPU is that ? A7?
> > CPU0 is A7 and CPU1 A17.
> 
> That's what I expected.
> 
> All platforms using A7 use DVFS, its low power, but not that much.
> 
> Why don't you do the same? Or is it that your platform doesn't support DVFS at
> all ? i.e. the clk/regulators aren't programmable to the processors ?
> 
Actually the freq is tunable, but voltage is fixed, So we fix it to a high freq because little benefit by freq tuning.

> I doubt that, and so there is something that you need to fix here instead.

Since the DVFS has percpu dvfs policy, so I think it is reasonable to allow cpu to disable dvfs, what do you think about that?

> 
> --
> viresh

Re: cpufreq_resume warning issue

[Viresh Kumar]

On 21-03-16, 09:26, Zengtao (B) wrote:
> Actually the freq is tunable, but voltage is fixed, So we fix it to a high freq because little benefit by freq tuning.

Okay, then just enable cpufreq and select performace governor for it, which will
select the highest frequency for the CPU.

> Since the DVFS has percpu dvfs policy, so I think it is reasonable to allow cpu to disable dvfs, what do you think about that?

Its not that we can't update core code for some specific use cases, but here you
should be using cpufreq core for the CPU0 as well, with a proper governor which
would not have any overheads during operations.

-- 
viresh

RE: cpufreq_resume warning issue

[Zengtao (B)]

> -----Original Message-----
> From: Viresh Kumar [mailto:viresh.kumar@linaro.org]
> Sent: Monday, March 21, 2016 5:45 PM
> To: Zengtao (B)
> Cc: rjw@rjwysocki.net; linux-pm@vger.kernel.org
> Subject: Re: cpufreq_resume warning issue
> 
> On 21-03-16, 09:26, Zengtao (B) wrote:
> > Actually the freq is tunable, but voltage is fixed, So we fix it to a high freq
> because little benefit by freq tuning.
> 
> Okay, then just enable cpufreq and select performace governor for it, which will
> select the highest frequency for the CPU.
> 
> > Since the DVFS has percpu dvfs policy, so I think it is reasonable to allow cpu
> to disable dvfs, what do you think about that?
> 
> Its not that we can't update core code for some specific use cases, but here
> you
> should be using cpufreq core for the CPU0 as well, with a proper governor
> which
> would not have any overheads during operations.

Yes, but we are just cheating the kernel by doing so, the cpu frequency governor of CPU0 is fixed 
even it has got the tunable interface to users. 

Of course, I can resolve my problem by your method, but I still think that the dvfs should get flexible
 enough to deal with my case(maybe big change from the current framework?). 

BR
Zengtao 

Re: cpufreq_resume warning issue

[Viresh Kumar]

On 22-03-16, 09:58, Zengtao (B) wrote:
> Yes, but we are just cheating the kernel by doing so, the cpu frequency governor of CPU0 is fixed 
> even it has got the tunable interface to users. 

You are mixing policy and features here..

Feature wise, your platform has the capability of supporting dynamic-frequency
scaling for CPU0 (A7), but as a policy you don't see much advantage of doing
that today.

But things will change, and perhaps your customers would want to do things
differently to save small amounts of powers lately. They would be required to go
through the code then to make that happen.

That's why I said, Keep the feature in place and support dynamic-frequency
switching for CPU0 as well (Its not gonna consume much of your time, I am quite
sure), and let people deal with policy. They can choose performance/powersave or
ondemand later on. They would be required to touch few sysfs file only then.

Changing core for this would mean, that we go through each and every line of
cpufreq-core again, as its an assumption we always had. Its not impossible and
may be required to be done in future, but it has to be worth the effort.

In this case, you are trying to avoid a small amount of effort, as you don't see
much value in terms of saving power.

But believe me, its a new platform and things will change. All current platforms
that are supporting DVFS for A7s aren't idiots :)

-- 
viresh