diff mbox

[v6,3/8] cpufreq:acpi:x86: Adjust the acpi-cpufreq.c code to work with common boost solution

Message ID 1374770011-22171-4-git-send-email-l.majewski@samsung.com (mailing list archive)
State Changes Requested, archived
Headers show

Commit Message

Lukasz Majewski July 25, 2013, 4:33 p.m. UTC 
The Intel's hardware based boost solution driver has been changed to cooperate with
common cpufreq boost framework.

The global sysfs boost attribute entry code (/sys/devices/system/cpu/cpufreq/boost)
has been moved to a core cpufreq code. This attribute is now only visible,
when cpufreq driver supports it.

The _store_boost() function has been redesigned to be used as set_boost
callback.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com>

---
Changes for v6:
- Perform acpi_cpufreq_boost_init initialization before cpufreq_driver
  registration
- Compile store_boost() only when CONFIG_X86_ACPI_CPUFREQ_CPB defined
- Use boost_enabled flag defined at acpi_cpufreq_driver to store information
  about boost state
- Instead of using cpufreq_set_boost_enabled(), modify the boost_enable in
  the acpi driver code

Changes for v5:
- Remove acpi-cpufreq's boost_enabled global flag and reuse one defined at
  cpufreq core

Changes for v4:
- add _store_boost to acpi_cpufreq_driver structure

Changes for v3:
- Bring back boost_enabled as a global flag
- Move boost_supported to cpufreq_driver structure

Changes for v2:
- Replace boost_enabled and boost_supported global flags with proper entries
  at struct cpufreq_driver.
- Removal of struct cpufreq_boost

 drivers/cpufreq/acpi-cpufreq.c |   87 ++++++++++++++--------------------------
 1 file changed, 30 insertions(+), 57 deletions(-)

Comments

Viresh Kumar July 26, 2013, 7:28 a.m. UTC | #1 
On 25 July 2013 22:03, Lukasz Majewski <l.majewski@samsung.com> wrote:
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c

>  static void __init acpi_cpufreq_boost_init(void)
>  {
> +       acpi_cpufreq_driver.boost_supported = false;

Would be better if we do this in else of below if.

>         if (boot_cpu_has(X86_FEATURE_CPB) || boot_cpu_has(X86_FEATURE_IDA)) {
>                 msrs = msrs_alloc();


> @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void)
>                         *iter = &cpb;
>         }
>  #endif
> +       acpi_cpufreq_boost_init();

We are calling it before registering cpufreq driver. Will this have
any complications?

>         ret = cpufreq_register_driver(&acpi_cpufreq_driver);
>         if (ret)
>                 free_acpi_perf_data();
> -       else
> -               acpi_cpufreq_boost_init();
>
>         return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukasz Majewski July 26, 2013, 8:09 a.m. UTC | #2 
On Fri, 26 Jul 2013 12:58:02 +0530 Viresh Kumar wrote,
> On 25 July 2013 22:03, Lukasz Majewski <l.majewski@samsung.com> wrote:
> > diff --git a/drivers/cpufreq/acpi-cpufreq.c
> > b/drivers/cpufreq/acpi-cpufreq.c
> 
> >  static void __init acpi_cpufreq_boost_init(void)
> >  {
> > +       acpi_cpufreq_driver.boost_supported = false;
	    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [*]
> 
> Would be better if we do this in else of below if.

We need to set boost_supported = false at [*] for the case when:
1. msrs_alloc fails 
or 
2. acpi_cpufreq is built as a module and can be inserted and removed
several times. Without [*] we could end up with wrong (not false)
initial state.

> 
> >         if (boot_cpu_has(X86_FEATURE_CPB) ||
> > boot_cpu_has(X86_FEATURE_IDA)) { msrs = msrs_alloc();
> 
> 
> > @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void)
> >                         *iter = &cpb;
> >         }
> >  #endif
> > +       acpi_cpufreq_boost_init();
	    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
> 
> We are calling it before registering cpufreq driver. Will this have
> any complications?

When we call [**] after the cpufreq_register_driver [***] we end up with
sysfs boost attribute not exported at x86.
The boost attribute is exported at [***] only when
acpi_cpufreq.boost_supported = true. However support for boost at x86
is evaluated at acpi_cpufreq_boost_init().


> 
> >         ret = cpufreq_register_driver(&acpi_cpufreq_driver);
> >         if (ret)
> >                 free_acpi_perf_data();
> > -       else
> > -               acpi_cpufreq_boost_init();
> >
> >         return ret;
Viresh Kumar July 26, 2013, 9:24 a.m. UTC | #3 
On 26 July 2013 13:39, Lukasz Majewski <l.majewski@samsung.com> wrote:
> On Fri, 26 Jul 2013 12:58:02 +0530 Viresh Kumar wrote,
>> On 25 July 2013 22:03, Lukasz Majewski <l.majewski@samsung.com> wrote:
>> > diff --git a/drivers/cpufreq/acpi-cpufreq.c
>> > b/drivers/cpufreq/acpi-cpufreq.c
>>
>> >  static void __init acpi_cpufreq_boost_init(void)
>> >  {
>> > +       acpi_cpufreq_driver.boost_supported = false;
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [*]
>>
>> Would be better if we do this in else of below if.
>
> We need to set boost_supported = false at [*] for the case when:
> 1. msrs_alloc fails
> or
> 2. acpi_cpufreq is built as a module and can be inserted and removed
> several times. Without [*] we could end up with wrong (not false)
> initial state.

Hmm.. Now that I see the code again, we don't need to set it to false
as it is a global variable and this field is already set to false..

>> >         if (boot_cpu_has(X86_FEATURE_CPB) ||
>> > boot_cpu_has(X86_FEATURE_IDA)) { msrs = msrs_alloc();
>>
>>
>> > @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void)
>> >                         *iter = &cpb;
>> >         }
>> >  #endif
>> > +       acpi_cpufreq_boost_init();
>             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
>>
>> We are calling it before registering cpufreq driver. Will this have
>> any complications?
>
> When we call [**] after the cpufreq_register_driver [***] we end up with
> sysfs boost attribute not exported at x86.
> The boost attribute is exported at [***] only when
> acpi_cpufreq.boost_supported = true. However support for boost at x86
> is evaluated at acpi_cpufreq_boost_init().

I understand why you moved it above cpufreq driver register. I was thinking
if there can be few side effects of this..
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lukasz Majewski July 26, 2013, 9:44 a.m. UTC | #4 
On Fri, 26 Jul 2013 14:54:07 +0530 Viresh Kumar viresh.kumar@linaro.org
wrote,
> On 26 July 2013 13:39, Lukasz Majewski <l.majewski@samsung.com> wrote:
> > On Fri, 26 Jul 2013 12:58:02 +0530 Viresh Kumar wrote,
> >> On 25 July 2013 22:03, Lukasz Majewski <l.majewski@samsung.com>
> >> wrote:
> >> > diff --git a/drivers/cpufreq/acpi-cpufreq.c
> >> > b/drivers/cpufreq/acpi-cpufreq.c
> >>
> >> >  static void __init acpi_cpufreq_boost_init(void)
> >> >  {
> >> > +       acpi_cpufreq_driver.boost_supported = false;
> >             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [*]
> >>
> >> Would be better if we do this in else of below if.
> >
> > We need to set boost_supported = false at [*] for the case when:
> > 1. msrs_alloc fails
> > or
> > 2. acpi_cpufreq is built as a module and can be inserted and removed
> > several times. Without [*] we could end up with wrong (not false)
> > initial state.
> 
> Hmm.. Now that I see the code again, we don't need to set it to false
> as it is a global variable and this field is already set to false..

Ok, so then I will remove this line [*].

> 
> >> >         if (boot_cpu_has(X86_FEATURE_CPB) ||
> >> > boot_cpu_has(X86_FEATURE_IDA)) { msrs = msrs_alloc();
> >>
> >>
> >> > @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void)
> >> >                         *iter = &cpb;
> >> >         }
> >> >  #endif
> >> > +       acpi_cpufreq_boost_init();
> >             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
> >>
> >> We are calling it before registering cpufreq driver. Will this have
> >> any complications?
> >
> > When we call [**] after the cpufreq_register_driver [***] we end up
> > with sysfs boost attribute not exported at x86.
> > The boost attribute is exported at [***] only when
> > acpi_cpufreq.boost_supported = true. However support for boost at
> > x86 is evaluated at acpi_cpufreq_boost_init().
> 
> I understand why you moved it above cpufreq driver register. I was
> thinking if there can be few side effects of this..

As fair as I've tested it, there weren't any side effects. 

Moreover the acpi_cpufreq_boost_init() mostly read state of the HW, so
IMHO can be done before cpufreg_register_driver().
Lukasz Majewski Aug. 12, 2013, 9:12 a.m. UTC | #5 
On Fri, 26 Jul 2013 14:54:07 +0530 Viresh Kumar viresh.kumar@linaro.org
wrote,
> On 26 July 2013 13:39, Lukasz Majewski <l.majewski@samsung.com> wrote:
> > On Fri, 26 Jul 2013 12:58:02 +0530 Viresh Kumar wrote,
> >> On 25 July 2013 22:03, Lukasz Majewski <l.majewski@samsung.com>
> >> wrote:
> >> > diff --git a/drivers/cpufreq/acpi-cpufreq.c
> >> > b/drivers/cpufreq/acpi-cpufreq.c
> >>
> >> >  static void __init acpi_cpufreq_boost_init(void)
> >> >  {
> >> > +       acpi_cpufreq_driver.boost_supported = false;
> >             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [*]
> >>
> >> Would be better if we do this in else of below if.
> >
> > We need to set boost_supported = false at [*] for the case when:
> > 1. msrs_alloc fails
> > or
> > 2. acpi_cpufreq is built as a module and can be inserted and removed
> > several times. Without [*] we could end up with wrong (not false)
> > initial state.
> 
> Hmm.. Now that I see the code again, we don't need to set it to false
> as it is a global variable and this field is already set to false..

Ok, I will delete the line at [*].

> 
> >> >         if (boot_cpu_has(X86_FEATURE_CPB) ||
> >> > boot_cpu_has(X86_FEATURE_IDA)) { msrs = msrs_alloc();
> >>
> >>
> >> > @@ -1021,12 +995,11 @@ static int __init acpi_cpufreq_init(void)
> >> >                         *iter = &cpb;
> >> >         }
> >> >  #endif
> >> > +       acpi_cpufreq_boost_init();
> >             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [**]
> >>
> >> We are calling it before registering cpufreq driver. Will this have
> >> any complications?
> >
> > When we call [**] after the cpufreq_register_driver [***] we end up
> > with sysfs boost attribute not exported at x86.
> > The boost attribute is exported at [***] only when
> > acpi_cpufreq.boost_supported = true. However support for boost at
> > x86 is evaluated at acpi_cpufreq_boost_init().
> 
> I understand why you moved it above cpufreq driver register. I was
> thinking if there can be few side effects of this..

Have any problem with the above change came to your mind?
Viresh Kumar Aug. 12, 2013, 9:14 a.m. UTC | #6 
On 12 August 2013 14:42, Lukasz Majewski <l.majewski@samsung.com> wrote:
> Have any problem with the above change came to your mind?

No.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 3926402..509764c 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -81,7 +81,6 @@  static struct acpi_processor_performance __percpu *acpi_perf_data;
 static struct cpufreq_driver acpi_cpufreq_driver;
 
 static unsigned int acpi_pstate_strict;
-static bool boost_enabled, boost_supported;
 static struct msr __percpu *msrs;
 
 static bool boost_state(unsigned int cpu)
@@ -134,49 +133,16 @@  static void boost_set_msrs(bool enable, const struct cpumask *cpumask)
 	wrmsr_on_cpus(cpumask, msr_addr, msrs);
 }
 
-static ssize_t _store_boost(const char *buf, size_t count)
+static int _store_boost(int val)
 {
-	int ret;
-	unsigned long val = 0;
-
-	if (!boost_supported)
-		return -EINVAL;
-
-	ret = kstrtoul(buf, 10, &val);
-	if (ret || (val > 1))
-		return -EINVAL;
-
-	if ((val && boost_enabled) || (!val && !boost_enabled))
-		return count;
-
 	get_online_cpus();
-
 	boost_set_msrs(val, cpu_online_mask);
-
 	put_online_cpus();
-
-	boost_enabled = val;
 	pr_debug("Core Boosting %sabled.\n", val ? "en" : "dis");
 
-	return count;
-}
-
-static ssize_t store_global_boost(struct kobject *kobj, struct attribute *attr,
-				  const char *buf, size_t count)
-{
-	return _store_boost(buf, count);
-}
-
-static ssize_t show_global_boost(struct kobject *kobj,
-				 struct attribute *attr, char *buf)
-{
-	return sprintf(buf, "%u\n", boost_enabled);
+	return 0;
 }
 
-static struct global_attr global_boost = __ATTR(boost, 0644,
-						show_global_boost,
-						store_global_boost);
-
 static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
 {
 	struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
@@ -187,15 +153,32 @@  static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
 cpufreq_freq_attr_ro(freqdomain_cpus);
 
 #ifdef CONFIG_X86_ACPI_CPUFREQ_CPB
+static ssize_t store_boost(const char *buf, size_t count)
+{
+	int ret;
+	unsigned long val = 0;
+
+	if (!acpi_cpufreq_driver.boost_supported)
+		return -EINVAL;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret || (val > 1))
+		return -EINVAL;
+
+	_store_boost((int) val);
+
+	return count;
+}
+
 static ssize_t store_cpb(struct cpufreq_policy *policy, const char *buf,
 			 size_t count)
 {
-	return _store_boost(buf, count);
+	return store_boost(buf, count);
 }
 
 static ssize_t show_cpb(struct cpufreq_policy *policy, char *buf)
 {
-	return sprintf(buf, "%u\n", boost_enabled);
+	return sprintf(buf, "%u\n", acpi_cpufreq_driver.boost_enabled);
 }
 
 static struct freq_attr cpb = __ATTR(cpb, 0644, show_cpb, store_cpb);
@@ -583,7 +566,7 @@  static int boost_notify(struct notifier_block *nb, unsigned long action,
 	switch (action) {
 	case CPU_UP_PREPARE:
 	case CPU_UP_PREPARE_FROZEN:
-		boost_set_msrs(boost_enabled, cpumask);
+		boost_set_msrs(acpi_cpufreq_driver.boost_enabled, cpumask);
 		break;
 
 	case CPU_DOWN_PREPARE:
@@ -943,43 +926,34 @@  static struct cpufreq_driver acpi_cpufreq_driver = {
 	.name		= "acpi-cpufreq",
 	.owner		= THIS_MODULE,
 	.attr		= acpi_cpufreq_attr,
+	.set_boost      = _store_boost,
 };
 
 static void __init acpi_cpufreq_boost_init(void)
 {
+	acpi_cpufreq_driver.boost_supported = false;
 	if (boot_cpu_has(X86_FEATURE_CPB) || boot_cpu_has(X86_FEATURE_IDA)) {
 		msrs = msrs_alloc();
 
 		if (!msrs)
 			return;
 
-		boost_supported = true;
-		boost_enabled = boost_state(0);
-
+		acpi_cpufreq_driver.boost_supported = true;
+		acpi_cpufreq_driver.boost_enabled = boost_state(0);
 		get_online_cpus();
 
 		/* Force all MSRs to the same value */
-		boost_set_msrs(boost_enabled, cpu_online_mask);
+		boost_set_msrs(acpi_cpufreq_driver.boost_enabled,
+			       cpu_online_mask);
 
 		register_cpu_notifier(&boost_nb);
 
 		put_online_cpus();
-	} else
-		global_boost.attr.mode = 0444;
-
-	/* We create the boost file in any case, though for systems without
-	 * hardware support it will be read-only and hardwired to return 0.
-	 */
-	if (cpufreq_sysfs_create_file(&(global_boost.attr)))
-		pr_warn(PFX "could not register global boost sysfs file\n");
-	else
-		pr_debug("registered global boost sysfs file\n");
+	}
 }
 
 static void __exit acpi_cpufreq_boost_exit(void)
 {
-	cpufreq_sysfs_remove_file(&(global_boost.attr));
-
 	if (msrs) {
 		unregister_cpu_notifier(&boost_nb);
 
@@ -1021,12 +995,11 @@  static int __init acpi_cpufreq_init(void)
 			*iter = &cpb;
 	}
 #endif
+	acpi_cpufreq_boost_init();
 
 	ret = cpufreq_register_driver(&acpi_cpufreq_driver);
 	if (ret)
 		free_acpi_perf_data();
-	else
-		acpi_cpufreq_boost_init();
 
 	return ret;
 }