Re: [PATCH v10 2/2] cpufreq: qcom-hw: Add support for QCOM cpufreq HW driver

From: Matthias Kaehlcke
Date: Mon Nov 26 2018 - 18:30:20 EST

On Thu, Nov 22, 2018 at 10:37:08AM +0530, Viresh Kumar wrote:
> On 21-11-18, 14:06, Matthias Kaehlcke wrote:
> > On Wed, Nov 21, 2018 at 04:12:47PM +0530, Taniya Das wrote:
> > > + .boost_enabled = true,
> >
> > I have no real expertise with cpufreq boost, but after reading a bit
> > through cpufreq code this seems wrong. Boost is enabled statically,
> > however the driver has neither a ->set_boost function nor does it call
> > cpufreq_enable_boost_support() which would use a default
> > implementation for ->set_boost. As a result boost support is
> > effectively disabled:
> >
> > static bool cpufreq_boost_supported(void)
> > {
> > return likely(cpufreq_driver) && cpufreq_driver->set_boost;
> > }
> >
> > The driver should probably do the same as cpufreq-dt.c and call
> > cpufreq_enable_boost_support() if boost frequencies are available,
> > instead of 'enabling' boost statically.
> Feels like I have written the boost support in cpufreq core few decades back as
> I don't remember any of it :)
> But reading through the code this is what I understood.

Thanks for digging into it!

> There are two parts of boosting.
> - Sysfs file available or not to enable/disable boost frequencies on the go.
> This file gets created only when cpufreq_enable_boost_support() gets called.
> - Will cpufreq core consider boost frequencies or not while checking target
> frequency again, this is governed by cpufreq_driver->boost field, which can be
> set from driver or using the sysfs file mentioned above.
> In this driver, all we have done is to set the cpufreq_driver->boost field to
> true, which would allow cpufreq core to use boost frequencies as well. But that
> isn't any better than making them all normal frequencies and getting rid of
> boost stuff. The boosting stuff will be useful only if you want to disable some
> of them at runtime, based on heating, etc. And that is possible only after you
> create a sysfs file.

That matches what Amit reported (and I confirmed) about the CPU
frequency "being stuck" at the boost frequency
( on a loaded

Taniya: I wonder if it would make sense to drop boost support for now
in order to land a first working version of the driver soon, instead
of keeping respinning this series. Boost support could be added as a
separate feature, just like cooling devices. If you have a working
quick fix now that's also fine, otherwise I'd suggest the iterative
approach, I'm sure you also want to see this landing ;-)