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

[Matthias Kaehlcke <mka@chromium.org>]

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
(https://lore.kernel.org/patchwork/patch/998335/#1186701) on a loaded
system.

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 ;-)

Cheers

Matthias