Re: [PATCH] cpufreq: acpi_cpufreq: base frequency attribute support

From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: rjw@rjwysocki.net, linux-pm@vger.kernel.org
Subject: Re: [PATCH] cpufreq: acpi_cpufreq: base frequency attribute support
Date: Tue, 01 Mar 2016 10:10:41 -0800	[thread overview]
Message-ID: <1456855841.21069.59.camel@linux.intel.com> (raw)
In-Reply-To: <20160301022830.GX2791@vireshk-i7>

On Tue, 2016-03-01 at 07:58 +0530, Viresh Kumar wrote:
> On 29-02-16, 12:36, Srinivas Pandruvada wrote:
> > Currently scaling_available_frequencies displays list of available
> > frequencies which can be used to set max/min or current scaling
> > frequency.
> > 
> > > cat scaling_available_frequencies
> > 2301000 2300000 2200000 2000000 1900000 1800000 1700000 1500000
> > 1400000
> > 1300000 1100000 1000000 900000 800000 600000 500000
> > 
> > Here traditionally it is assumed that only 2301000 is a turbo
> > frequency,
> > which is purely opportunistic, anything else user can request and
> > may
> > get it.
> > But because of configurable thermal design power implementation in
> > several
> > Intel CPUs, the opportunistic frequency start can be any frequency
> > in this
> > range. For example it can be 2300000 or any lower value.
> > This change adds an optional new attribute called "base_frequency",
> > which displays the max non-turbo frequency (base frequency). For
> > example:
> > > cat base_frequency
> > 2200000
> > This will allow user to choose a certain frequency which is not
> > opportunistic.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
> > .com>
> > ---
> 
> You missed adding a version log and V2 in subject :(
> 
> >  drivers/cpufreq/acpi-cpufreq.c | 36
> > ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-
> > cpufreq.c
> > index 51eef87..76edd28 100644
> > --- a/drivers/cpufreq/acpi-cpufreq.c
> > +++ b/drivers/cpufreq/acpi-cpufreq.c
> > @@ -646,6 +646,21 @@ static int acpi_cpufreq_blacklist(struct
> > cpuinfo_x86 *c)
> >  }
> >  #endif
> >  
> > +static ssize_t show_base_frequency(struct cpufreq_policy *policy,
> > char *buf)
> > +{
> > +	u64 tar;
> > +	int err;
> > +
> > +	err = rdmsrl_safe_on_cpu(policy->cpu,
> > MSR_TURBO_ACTIVATION_RATIO, &tar);
> > +	if (!err)
> 
> So, this will automatically take care of checking if a CPU has
> support
> for it or not, right ?
Yes. It will fail if it doesn't support.
> 
> > +		/* Refer to IA64, IA32 SDM table 35-20, unit = 100
> > MHz */
> > +		return sprintf(buf, "%llu\n", tar * 100000);
> > +
> > +	return err;
> > +}
> > +
> > +cpufreq_freq_attr_ro(base_frequency);
> > +
> >  static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
> >  {
> >  	unsigned int i;
> > @@ -889,6 +904,7 @@ static struct freq_attr *acpi_cpufreq_attr[] =
> > {
> >  	&cpb,
> >  #endif
> >  	NULL,
> > +	NULL,
> 
> Its not straight forward, so please add a comment (like cpufreq-dt),
> that what the first NULL is going to be used for.
> 
OK.
> >  };
> >  
> >  static struct cpufreq_driver acpi_cpufreq_driver = {
> > @@ -971,6 +987,26 @@ static int __init acpi_cpufreq_init(void)
> >  			}
> >  	}
> >  #endif
> > +
> > +	if (boot_cpu_has(X86_FEATURE_IDA)) {
> > +		u64 plat_info, tar;
> > +		int err;
> > +
> > +		err = rdmsrl_safe_on_cpu(0, MSR_PLATFORM_INFO,
> > &plat_info);
> > +		/* Check number of config TDP levels > 0 */
> > +		if (!err && ((plat_info >> 33) & 0x03) > 0) {
> > +			err = rdmsrl_safe_on_cpu(0,
> > MSR_TURBO_ACTIVATION_RATIO,
> > +						 &tar);
> > +			if (!err) {
> 
> Maybe just:
>         if (!rdmsrl_safe_on_cpu(0, MSR_TURBO_ACTIVATION_RATIO, &tar))
> {
>                 ...
>         }
OK
> 
> > +				struct freq_attr **attr;
> > +
> > +				for (attr = acpi_cpufreq_attr;
> > *attr; attr++)
> > +				;
> > +				*attr = &base_frequency;
> 
> What about:
> 
>                                 acpi_cpufreq_attr[ARRAY_SIZE(acpi_cpu
> freq_attr) - 2] = &base_frequency;
> 
>                                 and a comment to describe that ?
> 
Will it work in this case?
"
Even if CONFIG_X86_ACPI_CPUFREQ_CPB is defined, 
acpi_cpufreq_attr[2] will be set to NULL in the init callback when
some condition fails. So we will have a NULL in the acpi_cpufreq_attr[]
before &base_frequency if we add at fixed place. 
"

Thanks,
Srinivas