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

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

[Srinivas Pandruvada]

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>
---
v4
- Changes for s/>/$ /
- new lines commmit description after paragraph
- coding style for for "if (ret)" in show_base_frequency

v3
-Merged Documentation and code patch to one patch.
-Created a common function to read tar which is used init and during read
-Documentation and code changed suggested by Viresh
-On unsupported CPUs, user when tries to read base_frequency
"Operation not supported" error will be returned.

v2
sysfs_create_file on policy->kobj is no longer possible on recent kernel
versions. So we can't create selectively create base_frequency attribute
on supported policies.
This version creates a base_frequency attribute, if the boot cpu supports
as part of cpufreq_driver->attributes. If for some CPUs the TAR can't be
read, return error.

v1:
Base version


 Documentation/cpu-freq/acpi-cpufreq.txt | 27 ++++++++++++++++++++
 drivers/cpufreq/acpi-cpufreq.c          | 45 +++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)
 create mode 100644 Documentation/cpu-freq/acpi-cpufreq.txt

diff --git a/Documentation/cpu-freq/acpi-cpufreq.txt b/Documentation/cpu-freq/acpi-cpufreq.txt
new file mode 100644
index 0000000..8f719cf
--- /dev/null
+++ b/Documentation/cpu-freq/acpi-cpufreq.txt
@@ -0,0 +1,26 @@
+Additional sysfs attributes for acpi-cpufreq
+
+acpi-cpufreq sysfs has following sysfs attributes in addition to standard
+cpufreq attributes:
+
+base_frequency :		Max non-turbo frequency
+				For example:
+				scaling_available_frequencies displays list
+				of available frequencies.
+
+				$ cat scaling_available_frequencies
+				2301000 2300000 2200000 2000000 1900000
+				1800000 1700000 1500000 1400000 1300000
+				1100000 1000000 900000 800000 600000
+				500000
+
+				If the base_frequency attribute is present
+				and readable, then any frequency above
+				base_frequency is turbo frequency. For example
+
+				$ cat base_frequency
+				2200000
+
+				Then in the above displayed list of
+				scaling_available_frequencies, 2300000 and
+				2301000 are turbo frequencies.
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 51eef87..2706bb5 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -646,6 +646,37 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c)
 }
 #endif
 
+static int x86_get_turbo_activation_ratio(int cpu, u64 *tar)
+{
+	u64 plat_info;
+	int err;
+
+	err = rdmsrl_safe_on_cpu(cpu, MSR_PLATFORM_INFO, &plat_info);
+	/* Check number of config TDP levels > 0 */
+	if (!err && ((plat_info >> 33) & 0x03) > 0)
+		err = rdmsrl_safe_on_cpu(cpu, MSR_TURBO_ACTIVATION_RATIO,
+					 tar);
+	else
+		err = -EOPNOTSUPP;
+
+	return err;
+}
+
+static ssize_t show_base_frequency(struct cpufreq_policy *policy, char *buf)
+{
+	u64 tar;
+	int err;
+
+	err = x86_get_turbo_activation_ratio(policy->cpu, &tar);
+	if (err)
+		return err;
+
+	/* Refer to IA64, IA32 SDM table 35-20, unit = 100 MHz */
+	return sprintf(buf, "%llu\n", tar * 100000);
+}
+
+cpufreq_freq_attr_ro(base_frequency);
+
 static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
 	unsigned int i;
@@ -888,6 +919,7 @@ static struct freq_attr *acpi_cpufreq_attr[] = {
 #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
 	&cpb,
 #endif
+	NULL, /* Extra space for base_frequency attr, if required */
 	NULL,
 };
 
@@ -971,6 +1003,19 @@ static int __init acpi_cpufreq_init(void)
 			}
 	}
 #endif
+
+	if (boot_cpu_has(X86_FEATURE_IDA)) {
+		u64 tar;
+
+		if (!x86_get_turbo_activation_ratio(0, &tar)) {
+			struct freq_attr **attr;
+
+			for (attr = acpi_cpufreq_attr; *attr; attr++)
+			;
+			*attr = &base_frequency;
+		}
+	}
+
 	acpi_cpufreq_boost_init();
 
 	ret = cpufreq_register_driver(&acpi_cpufreq_driver);
-- 
2.5.0

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

[Rafael J. Wysocki]

On Wednesday, March 02, 2016 11:13:14 AM 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>
> ---
> v4
> - Changes for s/>/$ /
> - new lines commmit description after paragraph
> - coding style for for "if (ret)" in show_base_frequency
> 
> v3
> -Merged Documentation and code patch to one patch.
> -Created a common function to read tar which is used init and during read
> -Documentation and code changed suggested by Viresh
> -On unsupported CPUs, user when tries to read base_frequency
> "Operation not supported" error will be returned.
> 
> v2
> sysfs_create_file on policy->kobj is no longer possible on recent kernel
> versions. So we can't create selectively create base_frequency attribute
> on supported policies.
> This version creates a base_frequency attribute, if the boot cpu supports
> as part of cpufreq_driver->attributes. If for some CPUs the TAR can't be
> read, return error.
> 
> v1:
> Base version
> 
> 
>  Documentation/cpu-freq/acpi-cpufreq.txt | 27 ++++++++++++++++++++
>  drivers/cpufreq/acpi-cpufreq.c          | 45 +++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+)
>  create mode 100644 Documentation/cpu-freq/acpi-cpufreq.txt
> 
> diff --git a/Documentation/cpu-freq/acpi-cpufreq.txt b/Documentation/cpu-freq/acpi-cpufreq.txt
> new file mode 100644
> index 0000000..8f719cf
> --- /dev/null
> +++ b/Documentation/cpu-freq/acpi-cpufreq.txt
> @@ -0,0 +1,26 @@
> +Additional sysfs attributes for acpi-cpufreq
> +
> +acpi-cpufreq sysfs has following sysfs attributes in addition to standard
> +cpufreq attributes:
> +
> +base_frequency :		Max non-turbo frequency
> +				For example:
> +				scaling_available_frequencies displays list
> +				of available frequencies.
> +
> +				$ cat scaling_available_frequencies
> +				2301000 2300000 2200000 2000000 1900000
> +				1800000 1700000 1500000 1400000 1300000
> +				1100000 1000000 900000 800000 600000
> +				500000
> +
> +				If the base_frequency attribute is present
> +				and readable, then any frequency above
> +				base_frequency is turbo frequency. For example
> +
> +				$ cat base_frequency
> +				2200000
> +
> +				Then in the above displayed list of
> +				scaling_available_frequencies, 2300000 and
> +				2301000 are turbo frequencies.

I'd add "That is, if any of these frequencies is requested, the processor will
be allowed to run at a frequency of its choice above base_frequency that need
not be equal to any of them and may be above the maximum listed one."

> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 51eef87..2706bb5 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -646,6 +646,37 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c)
>  }
>  #endif
>  
> +static int x86_get_turbo_activation_ratio(int cpu, u64 *tar)
> +{
> +	u64 plat_info;
> +	int err;
> +
> +	err = rdmsrl_safe_on_cpu(cpu, MSR_PLATFORM_INFO, &plat_info);
> +	/* Check number of config TDP levels > 0 */
> +	if (!err && ((plat_info >> 33) & 0x03) > 0)
> +		err = rdmsrl_safe_on_cpu(cpu, MSR_TURBO_ACTIVATION_RATIO,
> +					 tar);
> +	else
> +		err = -EOPNOTSUPP;
> +
> +	return err;

What about

	err = rdmsrl_safe_on_cpu(cpu, MSR_PLATFORM_INFO, &plat_info);
	if (err)
		return err;

	if (plat_info >> 33) & 0x03 > 0)
		return rdmsrl_safe_on_cpu(cpu, MSR_TURBO_ACTIVATION_RATIO, tar);

	return -ENXIO;

> +}

And I'd add

static inline bool boot_cpu_has_turbo_activation_ratio(void)
{
	u64 tar;

	return !x86_get_turbo_activation_ratio(0, &tar);
}

> +
> +static ssize_t show_base_frequency(struct cpufreq_policy *policy, char *buf)
> +{
> +	u64 tar;
> +	int err;
> +
> +	err = x86_get_turbo_activation_ratio(policy->cpu, &tar);
> +	if (err)
> +		return err;
> +
> +	/* Refer to IA64, IA32 SDM table 35-20, unit = 100 MHz */
> +	return sprintf(buf, "%llu\n", tar * 100000);
> +}
> +
> +cpufreq_freq_attr_ro(base_frequency);
> +
>  static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
>  {
>  	unsigned int i;
> @@ -888,6 +919,7 @@ static struct freq_attr *acpi_cpufreq_attr[] = {
>  #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
>  	&cpb,
>  #endif
> +	NULL, /* Extra space for base_frequency attr, if required */
>  	NULL,
>  };
>  
> @@ -971,6 +1003,19 @@ static int __init acpi_cpufreq_init(void)
>  			}
>  	}
>  #endif
> +
> +	if (boot_cpu_has(X86_FEATURE_IDA)) {
> +		u64 tar;
> +
> +		if (!x86_get_turbo_activation_ratio(0, &tar)) {
> +			struct freq_attr **attr;
> +
> +			for (attr = acpi_cpufreq_attr; *attr; attr++)
> +			;
> +			*attr = &base_frequency;
> +		}
> +	}
> +

What about doing it the way it is done for the CPB, that is, pointing
the entry to &base_frequency and clearing it if not supported later?

If you put it in front of &cpb, you only need one loop for the both of
them (attr has to be declared earlier):

	for (attr = acpi_cpufreq_attr; *attr; attr++) {
		if (*attr == &base_frequency && (!boot_cpu_has(X86_FEATURE_IDA)
		    || !boot_cpu_has_turbo_activation_ratio())) {
			struct freq_attr **next = attr + 1;

			*attr = *next;
			*next = NULL;
		}
#ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
		if (*attr == &cpb && !check_amd_hwpstate_cpu(0)) {
			pr_debug("CPB unsupported, do not expose it\n");
			*attr = NULL;
			continue;
		}
#endif
	}



>  	acpi_cpufreq_boost_init();
>  
>  	ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> 

Thanks,
Rafael

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

[Rafael J. Wysocki]

On Thu, Mar 3, 2016 at 1:43 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, March 02, 2016 11:13:14 AM Srinivas Pandruvada wrote:

[cut]

>> Currently scaling_available_frequencies displays list of available
>> frequencies which can be used to set max/min or current scaling frequency.

>> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
>> index 51eef87..2706bb5 100644
>> --- a/drivers/cpufreq/acpi-cpufreq.c
>> +++ b/drivers/cpufreq/acpi-cpufreq.c
>> @@ -646,6 +646,37 @@ static int acpi_cpufreq_blacklist(struct cpuinfo_x86 *c)
>>  }
>>  #endif
>>
>> +static int x86_get_turbo_activation_ratio(int cpu, u64 *tar)
>> +{
>> +     u64 plat_info;
>> +     int err;
>> +
>> +     err = rdmsrl_safe_on_cpu(cpu, MSR_PLATFORM_INFO, &plat_info);
>> +     /* Check number of config TDP levels > 0 */
>> +     if (!err && ((plat_info >> 33) & 0x03) > 0)
>> +             err = rdmsrl_safe_on_cpu(cpu, MSR_TURBO_ACTIVATION_RATIO,
>> +                                      tar);
>> +     else
>> +             err = -EOPNOTSUPP;
>> +
>> +     return err;
>
> What about
>
>         err = rdmsrl_safe_on_cpu(cpu, MSR_PLATFORM_INFO, &plat_info);
>         if (err)
>                 return err;
>
>         if (plat_info >> 33) & 0x03 > 0)

Missing parens.  The above should be

        if (((plat_info >> 33) & 0x03) > 0)

>                 return rdmsrl_safe_on_cpu(cpu, MSR_TURBO_ACTIVATION_RATIO, tar);
>
>         return -ENXIO;
>
>> +}

Thanks,
Rafael

[PATCH] cpufreq: acpi: Allow new dynamics attributes to be added to acpi_cpufreq_attr

[Viresh Kumar]

acpi_cpufreq_attr contains at least one dynamically populated (removed)
attribute today, cpb. But the code isn't friendly enough for new
attributes to be populated in a similar way.

Make some changes to allow new attributes to be easily added to the
struct.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
Srinivas,

This should make it easy for you to add another dynamic entry into the
acpi_cpufreq_attr structure.

 drivers/cpufreq/acpi-cpufreq.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 59a7b380fbe2..c37617ddcc9e 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -866,7 +866,7 @@ static struct freq_attr *acpi_cpufreq_attr[] = {
 	&cpufreq_freq_attr_scaling_available_freqs,
 	&freqdomain_cpus,
 #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
-	&cpb,
+	NULL,	/* Extra space for cpb if required */
 #endif
 	NULL,
 };
@@ -917,6 +917,7 @@ static void acpi_cpufreq_boost_exit(void)
 
 static int __init acpi_cpufreq_init(void)
 {
+	struct freq_attr **attr;
 	int ret;
 
 	if (acpi_disabled)
@@ -932,6 +933,10 @@ static int __init acpi_cpufreq_init(void)
 	if (ret)
 		return ret;
 
+	/* Find first empty entry */
+	for (attr = acpi_cpufreq_attr; *attr; attr++)
+		;
+
 #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
 	/* this is a sysfs file with a strange name and an even stranger
 	 * semantic - per CPU instantiation, but system global effect.
@@ -939,17 +944,11 @@ static int __init acpi_cpufreq_init(void)
 	 * only if configured. This is considered legacy code, which
 	 * will probably be removed at some point in the future.
 	 */
-	if (!check_amd_hwpstate_cpu(0)) {
-		struct freq_attr **attr;
-
+	if (check_amd_hwpstate_cpu(0))
+		*attr++ = &cpb;
+	else
 		pr_debug("CPB unsupported, do not expose it\n");
 
-		for (attr = acpi_cpufreq_attr; *attr; attr++)
-			if (*attr == &cpb) {
-				*attr = NULL;
-				break;
-			}
-	}
 #endif
 	acpi_cpufreq_boost_init();
 
-- 
2.7.1.410.g6faf27b

Re: [PATCH] cpufreq: acpi: Allow new dynamics attributes to be added to acpi_cpufreq_attr

[Rafael J. Wysocki]

On Thu, Mar 3, 2016 at 6:40 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> acpi_cpufreq_attr contains at least one dynamically populated (removed)
> attribute today, cpb. But the code isn't friendly enough for new
> attributes to be populated in a similar way.
>
> Make some changes to allow new attributes to be easily added to the
> struct.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> Srinivas,
>
> This should make it easy for you to add another dynamic entry into the
> acpi_cpufreq_attr structure.
>
>  drivers/cpufreq/acpi-cpufreq.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 59a7b380fbe2..c37617ddcc9e 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -866,7 +866,7 @@ static struct freq_attr *acpi_cpufreq_attr[] = {
>         &cpufreq_freq_attr_scaling_available_freqs,
>         &freqdomain_cpus,
>  #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
> -       &cpb,
> +       NULL,   /* Extra space for cpb if required */
>  #endif
>         NULL,
>  };
>
> @@ -917,6 +917,7 @@ static void acpi_cpufreq_boost_exit(void)
>
>  static int __init acpi_cpufreq_init(void)
>  {
> +       struct freq_attr **attr;
>         int ret;
>
>         if (acpi_disabled)
> @@ -932,6 +933,10 @@ static int __init acpi_cpufreq_init(void)
>         if (ret)
>                 return ret;
>
> +       /* Find first empty entry */
> +       for (attr = acpi_cpufreq_attr; *attr; attr++)
> +               ;
> +
>  #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
>         /* this is a sysfs file with a strange name and an even stranger
>          * semantic - per CPU instantiation, but system global effect.
> @@ -939,17 +944,11 @@ static int __init acpi_cpufreq_init(void)
>          * only if configured. This is considered legacy code, which
>          * will probably be removed at some point in the future.
>          */
> -       if (!check_amd_hwpstate_cpu(0)) {
> -               struct freq_attr **attr;
> -
> +       if (check_amd_hwpstate_cpu(0))
> +               *attr++ = &cpb;
> +       else
>                 pr_debug("CPB unsupported, do not expose it\n");
>
> -               for (attr = acpi_cpufreq_attr; *attr; attr++)
> -                       if (*attr == &cpb) {
> -                               *attr = NULL;
> -                               break;
> -                       }
> -       }
>  #endif
>         acpi_cpufreq_boost_init();

OK, this makes sense.

The table definition starts to look somewhat ugly with all of those
NULLs at the end, but then adding the base_frequency thing to it would
be easy.

Srinivas, can you please take this and rebase your patch on top of it?
 Or if you prefer, I can take it into my linux-next branch.

Thanks,
Rafael

Re: [PATCH] cpufreq: acpi: Allow new dynamics attributes to be added to acpi_cpufreq_attr

[Viresh Kumar]

On 03-03-16, 18:29, Rafael J. Wysocki wrote:
> Srinivas, can you please take this and rebase your patch on top of it?
>  Or if you prefer, I can take it into my linux-next branch.

Right, so you need to apply this patch as Srinivas already sent a separate patch
for his stuff, rebased over this.

-- 
viresh

Re: [PATCH] cpufreq: acpi: Allow new dynamics attributes to be added to acpi_cpufreq_attr

[Rafael J. Wysocki]

On Wed, Mar 9, 2016 at 10:41 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 03-03-16, 18:29, Rafael J. Wysocki wrote:
>> Srinivas, can you please take this and rebase your patch on top of it?
>>  Or if you prefer, I can take it into my linux-next branch.
>
> Right, so you need to apply this patch as Srinivas already sent a separate patch
> for his stuff, rebased over this.

I know, but I'm deferring that one as we aren't entirely sure about
the approach yet.

Thanks,
Rafael

Re: [PATCH] cpufreq: acpi: Allow new dynamics attributes to be added to acpi_cpufreq_attr

[Viresh Kumar]

On 09-03-16, 14:13, Rafael J. Wysocki wrote:
> On Wed, Mar 9, 2016 at 10:41 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > On 03-03-16, 18:29, Rafael J. Wysocki wrote:
> >> Srinivas, can you please take this and rebase your patch on top of it?
> >>  Or if you prefer, I can take it into my linux-next branch.
> >
> > Right, so you need to apply this patch as Srinivas already sent a separate patch
> > for his stuff, rebased over this.
> 
> I know, but I'm deferring that one as we aren't entirely sure about
> the approach yet.

I might have missed, but what are we not sure about here ? I thought, all three
of us agreed that this is a good idea ..

-- 
viresh

Re: [PATCH] cpufreq: acpi: Allow new dynamics attributes to be added to acpi_cpufreq_attr

[Rafael J. Wysocki]

On Wed, Mar 16, 2016 at 5:53 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 09-03-16, 14:13, Rafael J. Wysocki wrote:
>> On Wed, Mar 9, 2016 at 10:41 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> > On 03-03-16, 18:29, Rafael J. Wysocki wrote:
>> >> Srinivas, can you please take this and rebase your patch on top of it?
>> >>  Or if you prefer, I can take it into my linux-next branch.
>> >
>> > Right, so you need to apply this patch as Srinivas already sent a separate patch
>> > for his stuff, rebased over this.
>>
>> I know, but I'm deferring that one as we aren't entirely sure about
>> the approach yet.
>
> I might have missed, but what are we not sure about here ? I thought, all three
> of us agreed that this is a good idea ..

We are not sure if the new attribute is needed.