[PATCH 0/5] x86 CPPC usage

[PATCH 0/5] x86 CPPC usage

[Srinivas Pandruvada]

The x86 HWP (Hardware Controlled Performance States) is an implementation
of the ACPI-defined Collaborative Processor Performance Control (CPPC).
The current implementation of HWP doesn't use CPPC, but CPPC brings in
some advanced features. So we will be submitting changes to use CPPC.

The first series only contains ACPI CPPC changes required for x86.

Srinivas Pandruvada (5):
  acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config
  acpi: cpcc: Add integer read support
  acpi: cppc: Add support for function fixed hardware address
  acpi: cppc: Add prefix cppc to cpudata structure name
  acpi: bus: Enable HWP CPPC objects

 drivers/acpi/Kconfig            |  1 -
 drivers/acpi/bus.c              |  7 ++++
 drivers/acpi/cppc_acpi.c        | 88 +++++++++++++++++++++++++++++++++++------
 drivers/acpi/processor_driver.c |  5 ++-
 drivers/cpufreq/cppc_cpufreq.c  | 14 +++----
 include/acpi/cppc_acpi.h        |  4 +-
 6 files changed, 95 insertions(+), 24 deletions(-)

-- 
2.7.4

[PATCH 1/5] acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config

[Srinivas Pandruvada]

Some newer x86 platforms have support for both _CPC and _PSS object. So
kernel config can have both ACPI_CPU_FREQ_PSS and ACPI_CPPC_LIB. So remove
restriction for ACPI_CPPC_LIB to build only when ACPI_CPU_FREQ_PSS is not
defined.
Also for legacy systems with only _PSS, we shouldn't bail out if
acpi_cppc_processor_probe() fails, if ACPI_CPU_FREQ_PSS is also defined.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/acpi/Kconfig            | 1 -
 drivers/acpi/processor_driver.c | 5 ++++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 445ce28..c6bb6aa 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -227,7 +227,6 @@ config ACPI_MCFG
 config ACPI_CPPC_LIB
 	bool
 	depends on ACPI_PROCESSOR
-	depends on !ACPI_CPU_FREQ_PSS
 	select MAILBOX
 	select PCC
 	help
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 0553aee..0e0b629 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -245,8 +245,11 @@ static int __acpi_processor_start(struct acpi_device *device)
 		return 0;
 
 	result = acpi_cppc_processor_probe(pr);
-	if (result)
+	if (result) {
+#ifndef CONFIG_ACPI_CPU_FREQ_PSS
 		return -ENODEV;
+#endif
+	}
 
 	if (!cpuidle_get_driver() || cpuidle_get_driver() == &acpi_idle_driver)
 		acpi_processor_power_init(pr);
-- 
2.7.4

[PATCH 2/5] acpi: cpcc: Add integer read support

[Srinivas Pandruvada]

The _CPC performance limits can also be simple integer not just buffer
with register information. Add support for integer read via cpc_read.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/acpi/cppc_acpi.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 2e98173..34209f5 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -651,11 +651,18 @@ EXPORT_SYMBOL_GPL(acpi_cppc_processor_exit);
  * we can directly write to it.
  */
 
-static int cpc_read(struct cpc_reg *reg, u64 *val)
+static int cpc_read(struct cpc_register_resource *res, u64 *val)
 {
+	struct cpc_reg *reg = &res->cpc_entry.reg;
 	int ret_val = 0;
 
 	*val = 0;
+
+	if (res->type == ACPI_TYPE_INTEGER) {
+		*val = res->cpc_entry.int_value;
+		return 0;
+	}
+
 	if (reg->space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
 		void __iomem *vaddr = GET_PCC_VADDR(reg->address);
 
@@ -754,16 +761,16 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 		}
 	}
 
-	cpc_read(&highest_reg->cpc_entry.reg, &high);
+	cpc_read(highest_reg, &high);
 	perf_caps->highest_perf = high;
 
-	cpc_read(&lowest_reg->cpc_entry.reg, &low);
+	cpc_read(lowest_reg, &low);
 	perf_caps->lowest_perf = low;
 
-	cpc_read(&ref_perf->cpc_entry.reg, &ref);
+	cpc_read(ref_perf, &ref);
 	perf_caps->reference_perf = ref;
 
-	cpc_read(&nom_perf->cpc_entry.reg, &nom);
+	cpc_read(nom_perf, &nom);
 	perf_caps->nominal_perf = nom;
 
 	if (!ref)
@@ -812,8 +819,8 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 		}
 	}
 
-	cpc_read(&delivered_reg->cpc_entry.reg, &delivered);
-	cpc_read(&reference_reg->cpc_entry.reg, &reference);
+	cpc_read(delivered_reg, &delivered);
+	cpc_read(reference_reg, &reference);
 
 	if (!delivered || !reference) {
 		ret = -EFAULT;
-- 
2.7.4

[PATCH 3/5] acpi: cppc: Add support for function fixed hardware address

[Srinivas Pandruvada]

The CPPC registers can also be accessed via function fixed hardware
addresses in X86. Add support by modifying cpc_read and cpc_write
to be able to read/write MSRs on x86 platform. Also with this change,
acpi_cppc_processor_probe doesn't bail out if space id is not equal to
PCC or memory address space.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/acpi/cppc_acpi.c | 77 +++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 66 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 34209f5..939fb5c 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -42,6 +42,10 @@
 #include <linux/ktime.h>
 
 #include <acpi/cppc_acpi.h>
+#ifdef CONFIG_X86
+#include <asm/msr.h>
+#endif
+
 /*
  * Lock to provide mutually exclusive access to the PCC
  * channel. e.g. When the remote updates the shared region
@@ -585,8 +589,9 @@ int acpi_cppc_processor_probe(struct acpi_processor *pr)
 					pr_debug("Mismatched PCC ids.\n");
 					goto out_free;
 				}
-			} else if (gas_t->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY) {
-				/* Support only PCC and SYS MEM type regs */
+			} else if (gas_t->space_id != ACPI_ADR_SPACE_SYSTEM_MEMORY &&
+				   gas_t->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE) {
+				/* Support only PCC, FFH and SYS MEM type regs */
 				pr_debug("Unsupported register type: %d\n", gas_t->space_id);
 				goto out_free;
 			}
@@ -645,13 +650,59 @@ void acpi_cppc_processor_exit(struct acpi_processor *pr)
 }
 EXPORT_SYMBOL_GPL(acpi_cppc_processor_exit);
 
+#ifdef CONFIG_X86
+static int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val)
+{
+	int err;
+
+	err = rdmsrl_on_cpu(cpunum, reg->address, val);
+	if (!err) {
+		u64 mask = GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
+				       reg->bit_offset);
+
+		*val &= mask;
+		*val >>= reg->bit_offset;
+	}
+	return err;
+}
+
+static int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
+{
+	u64 rd_val;
+	int err;
+
+	err = rdmsrl_on_cpu(cpunum, reg->address, &rd_val);
+	if (!err) {
+		u64 mask = GENMASK_ULL(reg->bit_offset + reg->bit_width - 1,
+				       reg->bit_offset);
+
+		val <<= reg->bit_offset;
+		val &= mask;
+		rd_val &= ~mask;
+		rd_val |= val;
+		err = wrmsrl_on_cpu(cpunum, reg->address, rd_val);
+	}
+	return err;
+}
+#else
+static int cpc_read_ffh(int cpunum, struct cpc_reg *reg, u64 *val)
+{
+	return -EINVAL;
+}
+static int cpc_write_ffh(int cpunum, struct cpc_reg *reg, u64 val)
+{
+	return -EINVAL;
+
+}
+#endif
+
 /*
  * Since cpc_read and cpc_write are called while holding pcc_lock, it should be
  * as fast as possible. We have already mapped the PCC subspace during init, so
  * we can directly write to it.
  */
 
-static int cpc_read(struct cpc_register_resource *res, u64 *val)
+static int cpc_read(int cpunum, struct cpc_register_resource *res, u64 *val)
 {
 	struct cpc_reg *reg = &res->cpc_entry.reg;
 	int ret_val = 0;
@@ -684,13 +735,15 @@ static int cpc_read(struct cpc_register_resource *res, u64 *val)
 				reg->bit_width);
 			ret_val = -EFAULT;
 		}
+	} else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		ret_val = cpc_read_ffh(cpunum, reg, val);
 	} else
 		ret_val = acpi_os_read_memory((acpi_physical_address)reg->address,
 					val, reg->bit_width);
 	return ret_val;
 }
 
-static int cpc_write(struct cpc_reg *reg, u64 val)
+static int cpc_write(int cpunum, struct cpc_reg *reg, u64 val)
 {
 	int ret_val = 0;
 
@@ -716,6 +769,8 @@ static int cpc_write(struct cpc_reg *reg, u64 val)
 			ret_val = -EFAULT;
 			break;
 		}
+	} else if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE) {
+		ret_val = cpc_write_ffh(cpunum, reg, val);
 	} else
 		ret_val = acpi_os_write_memory((acpi_physical_address)reg->address,
 				val, reg->bit_width);
@@ -761,16 +816,16 @@ int cppc_get_perf_caps(int cpunum, struct cppc_perf_caps *perf_caps)
 		}
 	}
 
-	cpc_read(highest_reg, &high);
+	cpc_read(cpunum, highest_reg, &high);
 	perf_caps->highest_perf = high;
 
-	cpc_read(lowest_reg, &low);
+	cpc_read(cpunum, lowest_reg, &low);
 	perf_caps->lowest_perf = low;
 
-	cpc_read(ref_perf, &ref);
+	cpc_read(cpunum, ref_perf, &ref);
 	perf_caps->reference_perf = ref;
 
-	cpc_read(nom_perf, &nom);
+	cpc_read(cpunum, nom_perf, &nom);
 	perf_caps->nominal_perf = nom;
 
 	if (!ref)
@@ -819,8 +874,8 @@ int cppc_get_perf_ctrs(int cpunum, struct cppc_perf_fb_ctrs *perf_fb_ctrs)
 		}
 	}
 
-	cpc_read(delivered_reg, &delivered);
-	cpc_read(reference_reg, &reference);
+	cpc_read(cpunum, delivered_reg, &delivered);
+	cpc_read(cpunum, reference_reg, &reference);
 
 	if (!delivered || !reference) {
 		ret = -EFAULT;
@@ -875,7 +930,7 @@ int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls)
 	 * Skip writing MIN/MAX until Linux knows how to come up with
 	 * useful values.
 	 */
-	cpc_write(&desired_reg->cpc_entry.reg, perf_ctrls->desired_perf);
+	cpc_write(cpu, &desired_reg->cpc_entry.reg, perf_ctrls->desired_perf);
 
 	/* Is this a PCC reg ?*/
 	if (desired_reg->cpc_entry.reg.space_id == ACPI_ADR_SPACE_PLATFORM_COMM) {
-- 
2.7.4

[PATCH 4/5] acpi: cppc: Add prefix cppc to cpudata structure name

[Srinivas Pandruvada]

Since struct cpudata is defined in a header file, add prefix cppc_ to
make it not a generic name. Otherwise it causes compile issue in locally
define structure with the same name.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/acpi/cppc_acpi.c       |  4 ++--
 drivers/cpufreq/cppc_cpufreq.c | 14 +++++++-------
 include/acpi/cppc_acpi.h       |  4 ++--
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/acpi/cppc_acpi.c b/drivers/acpi/cppc_acpi.c
index 939fb5c..5ca517d 100644
--- a/drivers/acpi/cppc_acpi.c
+++ b/drivers/acpi/cppc_acpi.c
@@ -276,13 +276,13 @@ end:
  *
  *	Return: 0 for success or negative value for err.
  */
-int acpi_get_psd_map(struct cpudata **all_cpu_data)
+int acpi_get_psd_map(struct cppc_cpudata **all_cpu_data)
 {
 	int count_target;
 	int retval = 0;
 	unsigned int i, j;
 	cpumask_var_t covered_cpus;
-	struct cpudata *pr, *match_pr;
+	struct cppc_cpudata *pr, *match_pr;
 	struct acpi_psd_package *pdomain;
 	struct acpi_psd_package *match_pdomain;
 	struct cpc_desc *cpc_ptr, *match_cpc_ptr;
diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 8882b8e..b1b3549 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -30,13 +30,13 @@
  * performance capabilities, desired performance level
  * requested etc.
  */
-static struct cpudata **all_cpu_data;
+static struct cppc_cpudata **all_cpu_data;
 
 static int cppc_cpufreq_set_target(struct cpufreq_policy *policy,
 		unsigned int target_freq,
 		unsigned int relation)
 {
-	struct cpudata *cpu;
+	struct cppc_cpudata *cpu;
 	struct cpufreq_freqs freqs;
 	int ret = 0;
 
@@ -66,7 +66,7 @@ static int cppc_verify_policy(struct cpufreq_policy *policy)
 static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
 {
 	int cpu_num = policy->cpu;
-	struct cpudata *cpu = all_cpu_data[cpu_num];
+	struct cppc_cpudata *cpu = all_cpu_data[cpu_num];
 	int ret;
 
 	cpu->perf_ctrls.desired_perf = cpu->perf_caps.lowest_perf;
@@ -79,7 +79,7 @@ static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
 
 static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
 {
-	struct cpudata *cpu;
+	struct cppc_cpudata *cpu;
 	unsigned int cpu_num = policy->cpu;
 	int ret = 0;
 
@@ -134,7 +134,7 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
 static int __init cppc_cpufreq_init(void)
 {
 	int i, ret = 0;
-	struct cpudata *cpu;
+	struct cppc_cpudata *cpu;
 
 	if (acpi_disabled)
 		return -ENODEV;
@@ -144,7 +144,7 @@ static int __init cppc_cpufreq_init(void)
 		return -ENOMEM;
 
 	for_each_possible_cpu(i) {
-		all_cpu_data[i] = kzalloc(sizeof(struct cpudata), GFP_KERNEL);
+		all_cpu_data[i] = kzalloc(sizeof(struct cppc_cpudata), GFP_KERNEL);
 		if (!all_cpu_data[i])
 			goto out;
 
@@ -175,7 +175,7 @@ out:
 
 static void __exit cppc_cpufreq_exit(void)
 {
-	struct cpudata *cpu;
+	struct cppc_cpudata *cpu;
 	int i;
 
 	cpufreq_unregister_driver(&cppc_cpufreq_driver);
diff --git a/include/acpi/cppc_acpi.h b/include/acpi/cppc_acpi.h
index 284965c..b7816c2 100644
--- a/include/acpi/cppc_acpi.h
+++ b/include/acpi/cppc_acpi.h
@@ -114,7 +114,7 @@ struct cppc_perf_fb_ctrs {
 };
 
 /* Per CPU container for runtime CPPC management. */
-struct cpudata {
+struct cppc_cpudata {
 	int cpu;
 	struct cppc_perf_caps perf_caps;
 	struct cppc_perf_ctrls perf_ctrls;
@@ -127,6 +127,6 @@ struct cpudata {
 extern int cppc_get_perf_ctrs(int cpu, struct cppc_perf_fb_ctrs *perf_fb_ctrs);
 extern int cppc_set_perf(int cpu, struct cppc_perf_ctrls *perf_ctrls);
 extern int cppc_get_perf_caps(int cpu, struct cppc_perf_caps *caps);
-extern int acpi_get_psd_map(struct cpudata **);
+extern int acpi_get_psd_map(struct cppc_cpudata **);
 
 #endif /* _CPPC_ACPI_H*/
-- 
2.7.4

[PATCH 5/5] acpi: bus: Enable HWP CPPC objects

[Srinivas Pandruvada]

Need to set platform wide _OSC bits CPC version 1 and version 2 bits
so that BIOS presents CPPC objects.
Even though the cppc_acpi supports only version 2, need to set both
_OSC bits for version 1 and version 2, otherwise BIOS ignore _OSC
settings.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/acpi/bus.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 85b7d07..61643a5 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -330,6 +330,13 @@ static void acpi_bus_osc_support(void)
 	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_HOTPLUG_OST_SUPPORT;
 	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PCLPI_SUPPORT;
 
+#ifdef CONFIG_X86
+	if (boot_cpu_has(X86_FEATURE_HWP)) {
+		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT;
+		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT;
+	}
+#endif
+
 	if (!ghes_disable)
 		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
 	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
-- 
2.7.4

Re: [PATCH 1/5] acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config

[Alexey Klimov]

(adding Sudeep and Prashanth in c/c)

On Wed, Aug 10, 2016 at 05:17:22PM -0700, Srinivas Pandruvada wrote:
> Some newer x86 platforms have support for both _CPC and _PSS object. So
> kernel config can have both ACPI_CPU_FREQ_PSS and ACPI_CPPC_LIB. So remove
> restriction for ACPI_CPPC_LIB to build only when ACPI_CPU_FREQ_PSS is not
> defined.
> Also for legacy systems with only _PSS, we shouldn't bail out if
> acpi_cppc_processor_probe() fails, if ACPI_CPU_FREQ_PSS is also defined.
> 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/acpi/Kconfig            | 1 -
>  drivers/acpi/processor_driver.c | 5 ++++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 445ce28..c6bb6aa 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -227,7 +227,6 @@ config ACPI_MCFG
>  config ACPI_CPPC_LIB
>  	bool
>  	depends on ACPI_PROCESSOR
> -	depends on !ACPI_CPU_FREQ_PSS
>  	select MAILBOX
>  	select PCC
>  	help
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 0553aee..0e0b629 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -245,8 +245,11 @@ static int __acpi_processor_start(struct acpi_device *device)
>  		return 0;
>  
>  	result = acpi_cppc_processor_probe(pr);
> -	if (result)
> +	if (result) {
> +#ifndef CONFIG_ACPI_CPU_FREQ_PSS
>  		return -ENODEV;
> +#endif
> +	}
>  
>  	if (!cpuidle_get_driver() || cpuidle_get_driver() == &acpi_idle_driver)
>  		acpi_processor_power_init(pr);

If PSS is not defined and kernel fails to probe CPPC then why we should not
execute acpi_processor_power_init()?

Best regards,
Alexey

Re: [PATCH 1/5] acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config

[Srinivas Pandruvada]

On Fri, 2016-08-12 at 10:13 +0100, Alexey Klimov wrote:
> > 
> (adding Sudeep and Prashanth in c/c)
> 
> On Wed, Aug 10, 2016 at 05:17:22PM -0700, Srinivas Pandruvada wrote:

[...]

> >  	result = acpi_cppc_processor_probe(pr);
> > -	if (result)
> > +	if (result) {
> > +#ifndef CONFIG_ACPI_CPU_FREQ_PSS
> >  		return -ENODEV;
> > +#endif
> > +	}
> >  
> >  	if (!cpuidle_get_driver() || cpuidle_get_driver() ==
> > &acpi_idle_driver)
> >  		acpi_processor_power_init(pr);
> If PSS is not defined and kernel fails to probe CPPC then why we
> should not
> execute acpi_processor_power_init()?
Did I change the current behavior? Currently when 
acpi_cppc_processor_probe() fails, then -ENODEV is returned.

Thanks,
Srinivas

> 
> Best regards,
> Alexey
> 
> --
> 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

Re: [PATCH 1/5] acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config

[Prakash, Prashanth]

Hi Alexey,

On 8/12/2016 3:13 AM, Alexey Klimov wrote:
> (adding Sudeep and Prashanth in c/c)
>
> On Wed, Aug 10, 2016 at 05:17:22PM -0700, Srinivas Pandruvada wrote:
>> Some newer x86 platforms have support for both _CPC and _PSS object. So
>> kernel config can have both ACPI_CPU_FREQ_PSS and ACPI_CPPC_LIB. So remove
>> restriction for ACPI_CPPC_LIB to build only when ACPI_CPU_FREQ_PSS is not
>> defined.
>> Also for legacy systems with only _PSS, we shouldn't bail out if
>> acpi_cppc_processor_probe() fails, if ACPI_CPU_FREQ_PSS is also defined.
>>
>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>> ---
>>  drivers/acpi/Kconfig            | 1 -
>>  drivers/acpi/processor_driver.c | 5 ++++-
>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 445ce28..c6bb6aa 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -227,7 +227,6 @@ config ACPI_MCFG
>>  config ACPI_CPPC_LIB
>>  	bool
>>  	depends on ACPI_PROCESSOR
>> -	depends on !ACPI_CPU_FREQ_PSS
>>  	select MAILBOX
>>  	select PCC
>>  	help
>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 0553aee..0e0b629 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -245,8 +245,11 @@ static int __acpi_processor_start(struct acpi_device *device)
>>  		return 0;
>>  
>>  	result = acpi_cppc_processor_probe(pr);
>> -	if (result)
>> +	if (result) {
>> +#ifndef CONFIG_ACPI_CPU_FREQ_PSS
>>  		return -ENODEV;
>> +#endif
>> +	}
>>  
>>  	if (!cpuidle_get_driver() || cpuidle_get_driver() == &acpi_idle_driver)
>>  		acpi_processor_power_init(pr);
> If PSS is not defined and kernel fails to probe CPPC then why we should not
> execute acpi_processor_power_init()?
Returning on cppc probe failure looks like a bug. We can just print
a warning and continue to acpi_processor_power_init().

Thanks,
Prashanth

Re: [PATCH 1/5] acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config

[Hoan Tran]

Hi,

On Fri, Aug 12, 2016 at 9:04 AM, Prakash, Prashanth
<pprakash@codeaurora.org> wrote:
> Hi Alexey,
>
> On 8/12/2016 3:13 AM, Alexey Klimov wrote:
>> (adding Sudeep and Prashanth in c/c)
>>
>> On Wed, Aug 10, 2016 at 05:17:22PM -0700, Srinivas Pandruvada wrote:
>>> Some newer x86 platforms have support for both _CPC and _PSS object. So
>>> kernel config can have both ACPI_CPU_FREQ_PSS and ACPI_CPPC_LIB. So remove
>>> restriction for ACPI_CPPC_LIB to build only when ACPI_CPU_FREQ_PSS is not
>>> defined.
>>> Also for legacy systems with only _PSS, we shouldn't bail out if
>>> acpi_cppc_processor_probe() fails, if ACPI_CPU_FREQ_PSS is also defined.
>>>
>>> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>>> ---
>>>  drivers/acpi/Kconfig            | 1 -
>>>  drivers/acpi/processor_driver.c | 5 ++++-
>>>  2 files changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>>> index 445ce28..c6bb6aa 100644
>>> --- a/drivers/acpi/Kconfig
>>> +++ b/drivers/acpi/Kconfig
>>> @@ -227,7 +227,6 @@ config ACPI_MCFG
>>>  config ACPI_CPPC_LIB
>>>      bool
>>>      depends on ACPI_PROCESSOR
>>> -    depends on !ACPI_CPU_FREQ_PSS
>>>      select MAILBOX
>>>      select PCC
>>>      help
>>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>>> index 0553aee..0e0b629 100644
>>> --- a/drivers/acpi/processor_driver.c
>>> +++ b/drivers/acpi/processor_driver.c
>>> @@ -245,8 +245,11 @@ static int __acpi_processor_start(struct acpi_device *device)
>>>              return 0;
>>>
>>>      result = acpi_cppc_processor_probe(pr);
>>> -    if (result)
>>> +    if (result) {
>>> +#ifndef CONFIG_ACPI_CPU_FREQ_PSS
>>>              return -ENODEV;
>>> +#endif
>>> +    }
>>>
>>>      if (!cpuidle_get_driver() || cpuidle_get_driver() == &acpi_idle_driver)
>>>              acpi_processor_power_init(pr);
>> If PSS is not defined and kernel fails to probe CPPC then why we should not
>> execute acpi_processor_power_init()?
> Returning on cppc probe failure looks like a bug. We can just print
> a warning and continue to acpi_processor_power_init().

Yes, it is. We should continue. I saw an issue about that. If the CPPC
probe fails, CPUidle can NOT be registered.

Thanks
Hoan

>
> Thanks,
> Prashanth
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config

[Hoan Tran]

Hi Srinivas,

On Wed, Aug 10, 2016 at 5:17 PM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> Some newer x86 platforms have support for both _CPC and _PSS object. So
> kernel config can have both ACPI_CPU_FREQ_PSS and ACPI_CPPC_LIB. So remove
> restriction for ACPI_CPPC_LIB to build only when ACPI_CPU_FREQ_PSS is not
> defined.
> Also for legacy systems with only _PSS, we shouldn't bail out if
> acpi_cppc_processor_probe() fails, if ACPI_CPU_FREQ_PSS is also defined.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/acpi/Kconfig            | 1 -
>  drivers/acpi/processor_driver.c | 5 ++++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 445ce28..c6bb6aa 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -227,7 +227,6 @@ config ACPI_MCFG
>  config ACPI_CPPC_LIB
>         bool
>         depends on ACPI_PROCESSOR
> -       depends on !ACPI_CPU_FREQ_PSS

>From ACPI 6.1 spec, if _CPC is present, its use supersedes the use of
PSS. So I think, config ACPI_CPU_FREQ_PSS should depend on
!ACPI_CPPC_LIB.

Thanks
Hoan

>         select MAILBOX
>         select PCC
>         help
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 0553aee..0e0b629 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -245,8 +245,11 @@ static int __acpi_processor_start(struct acpi_device *device)
>                 return 0;
>
>         result = acpi_cppc_processor_probe(pr);
> -       if (result)
> +       if (result) {
> +#ifndef CONFIG_ACPI_CPU_FREQ_PSS
>                 return -ENODEV;
> +#endif
> +       }
>
>         if (!cpuidle_get_driver() || cpuidle_get_driver() == &acpi_idle_driver)
>                 acpi_processor_power_init(pr);
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config

[Srinivas Pandruvada]

On Fri, 2016-08-12 at 09:35 -0700, Hoan Tran wrote:
> Hi Srinivas,
> 
> On Wed, Aug 10, 2016 at 5:17 PM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > Some newer x86 platforms have support for both _CPC and _PSS
> > object. So
> > kernel config can have both ACPI_CPU_FREQ_PSS and ACPI_CPPC_LIB. So
> > remove
> > restriction for ACPI_CPPC_LIB to build only when ACPI_CPU_FREQ_PSS
> > is not
> > defined.
> > Also for legacy systems with only _PSS, we shouldn't bail out if
> > acpi_cppc_processor_probe() fails, if ACPI_CPU_FREQ_PSS is also
> > defined.
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
> > .com>
> > ---
> >  drivers/acpi/Kconfig            | 1 -
> >  drivers/acpi/processor_driver.c | 5 ++++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > index 445ce28..c6bb6aa 100644
> > --- a/drivers/acpi/Kconfig
> > +++ b/drivers/acpi/Kconfig
> > @@ -227,7 +227,6 @@ config ACPI_MCFG
> >  config ACPI_CPPC_LIB
> >         bool
> >         depends on ACPI_PROCESSOR
> > -       depends on !ACPI_CPU_FREQ_PSS
> From ACPI 6.1 spec, if _CPC is present, its use supersedes the use of
> PSS. So I think, config ACPI_CPU_FREQ_PSS should depend on
> !ACPI_CPPC_LIB.
> 
Distro want to have a single binary kernel image, so they will turn on
all configs. So this is not a compile time decision.
On runtime if the ACPI contains both tables than _CPC should be used
(but that also if the kernel is capable of handling _CPC, as legacy
kernel will not).

Thanks,
Srinivas

> Thanks
> Hoan
> 
> > 
> >         select MAILBOX
> >         select PCC
> >         help
> > diff --git a/drivers/acpi/processor_driver.c
> > b/drivers/acpi/processor_driver.c
> > index 0553aee..0e0b629 100644
> > --- a/drivers/acpi/processor_driver.c
> > +++ b/drivers/acpi/processor_driver.c
> > @@ -245,8 +245,11 @@ static int __acpi_processor_start(struct
> > acpi_device *device)
> >                 return 0;
> > 
> >         result = acpi_cppc_processor_probe(pr);
> > -       if (result)
> > +       if (result) {
> > +#ifndef CONFIG_ACPI_CPU_FREQ_PSS
> >                 return -ENODEV;
> > +#endif
> > +       }
> > 
> >         if (!cpuidle_get_driver() || cpuidle_get_driver() ==
> > &acpi_idle_driver)
> >                 acpi_processor_power_init(pr);
> > --
> > 2.7.4
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> 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

Re: [PATCH 1/5] acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config

[Srinivas Pandruvada]

On Fri, 2016-08-12 at 09:32 -0700, Hoan Tran wrote:
> Hi,
> 
> On Fri, Aug 12, 2016 at 9:04 AM, Prakash, Prashanth
> <pprakash@codeaurora.org> wrote:
> > 
> > Hi Alexey,
> > 
> > On 8/12/2016 3:13 AM, Alexey Klimov wrote:
> > > 
> > > (adding Sudeep and Prashanth in c/c)
> > > 
> > > On Wed, Aug 10, 2016 at 05:17:22PM -0700, Srinivas Pandruvada
> > > wrote:
> > > > 
> > > > Some newer x86 platforms have support for both _CPC and _PSS
> > > > object. So
> > > > kernel config can have both ACPI_CPU_FREQ_PSS and
> > > > ACPI_CPPC_LIB. So remove
> > > > restriction for ACPI_CPPC_LIB to build only when
> > > > ACPI_CPU_FREQ_PSS is not
> > > > defined.
> > > > Also for legacy systems with only _PSS, we shouldn't bail out
> > > > if
> > > > acpi_cppc_processor_probe() fails, if ACPI_CPU_FREQ_PSS is also
> > > > defined.
> > > > 
> > > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.i
> > > > ntel.com>
> > > > ---
> > > >  drivers/acpi/Kconfig            | 1 -
> > > >  drivers/acpi/processor_driver.c | 5 ++++-
> > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > > > index 445ce28..c6bb6aa 100644
> > > > --- a/drivers/acpi/Kconfig
> > > > +++ b/drivers/acpi/Kconfig
> > > > @@ -227,7 +227,6 @@ config ACPI_MCFG
> > > >  config ACPI_CPPC_LIB
> > > >      bool
> > > >      depends on ACPI_PROCESSOR
> > > > -    depends on !ACPI_CPU_FREQ_PSS
> > > >      select MAILBOX
> > > >      select PCC
> > > >      help
> > > > diff --git a/drivers/acpi/processor_driver.c
> > > > b/drivers/acpi/processor_driver.c
> > > > index 0553aee..0e0b629 100644
> > > > --- a/drivers/acpi/processor_driver.c
> > > > +++ b/drivers/acpi/processor_driver.c
> > > > @@ -245,8 +245,11 @@ static int __acpi_processor_start(struct
> > > > acpi_device *device)
> > > >              return 0;
> > > > 
> > > >      result = acpi_cppc_processor_probe(pr);
> > > > -    if (result)
> > > > +    if (result) {
> > > > +#ifndef CONFIG_ACPI_CPU_FREQ_PSS
> > > >              return -ENODEV;
> > > > +#endif
> > > > +    }
> > > > 
> > > >      if (!cpuidle_get_driver() || cpuidle_get_driver() ==
> > > > &acpi_idle_driver)
> > > >              acpi_processor_power_init(pr);
> > > If PSS is not defined and kernel fails to probe CPPC then why we
> > > should not
> > > execute acpi_processor_power_init()?
> > Returning on cppc probe failure looks like a bug. We can just print
> > a warning and continue to acpi_processor_power_init().
> Yes, it is. We should continue. I saw an issue about that. If the
> CPPC
> probe fails, CPUidle can NOT be registered.

I wanted to keep the existing functionality as is. But I can submit
another patch on top of it to ignore cppc probe failure.

Thanks,
Srinivas


> Thanks
> Hoan
> 
> > 
> > 
> > Thanks,
> > Prashanth
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > acpi" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config

[Hoan Tran]

Hi Srinivas,

On Fri, Aug 12, 2016 at 9:52 AM, Srinivas Pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
> On Fri, 2016-08-12 at 09:35 -0700, Hoan Tran wrote:
>> Hi Srinivas,
>>
>> On Wed, Aug 10, 2016 at 5:17 PM, Srinivas Pandruvada
>> <srinivas.pandruvada@linux.intel.com> wrote:
>> >
>> > Some newer x86 platforms have support for both _CPC and _PSS
>> > object. So
>> > kernel config can have both ACPI_CPU_FREQ_PSS and ACPI_CPPC_LIB. So
>> > remove
>> > restriction for ACPI_CPPC_LIB to build only when ACPI_CPU_FREQ_PSS
>> > is not
>> > defined.
>> > Also for legacy systems with only _PSS, we shouldn't bail out if
>> > acpi_cppc_processor_probe() fails, if ACPI_CPU_FREQ_PSS is also
>> > defined.
>> >
>> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
>> > .com>
>> > ---
>> >  drivers/acpi/Kconfig            | 1 -
>> >  drivers/acpi/processor_driver.c | 5 ++++-
>> >  2 files changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> > index 445ce28..c6bb6aa 100644
>> > --- a/drivers/acpi/Kconfig
>> > +++ b/drivers/acpi/Kconfig
>> > @@ -227,7 +227,6 @@ config ACPI_MCFG
>> >  config ACPI_CPPC_LIB
>> >         bool
>> >         depends on ACPI_PROCESSOR
>> > -       depends on !ACPI_CPU_FREQ_PSS
>> From ACPI 6.1 spec, if _CPC is present, its use supersedes the use of
>> PSS. So I think, config ACPI_CPU_FREQ_PSS should depend on
>> !ACPI_CPPC_LIB.
>>
> Distro want to have a single binary kernel image, so they will turn on
> all configs. So this is not a compile time decision.
> On runtime if the ACPI contains both tables than _CPC should be used
> (but that also if the kernel is capable of handling _CPC, as legacy
> kernel will not).

Agreed. If so, this "depends on" is not need, right ? And a runtime
code will be added to decide which is used instead ?

Thanks
Hoan

>
> Thanks,
> Srinivas
>
>> Thanks
>> Hoan
>>
>> >
>> >         select MAILBOX
>> >         select PCC
>> >         help
>> > diff --git a/drivers/acpi/processor_driver.c
>> > b/drivers/acpi/processor_driver.c
>> > index 0553aee..0e0b629 100644
>> > --- a/drivers/acpi/processor_driver.c
>> > +++ b/drivers/acpi/processor_driver.c
>> > @@ -245,8 +245,11 @@ static int __acpi_processor_start(struct
>> > acpi_device *device)
>> >                 return 0;
>> >
>> >         result = acpi_cppc_processor_probe(pr);
>> > -       if (result)
>> > +       if (result) {
>> > +#ifndef CONFIG_ACPI_CPU_FREQ_PSS
>> >                 return -ENODEV;
>> > +#endif
>> > +       }
>> >
>> >         if (!cpuidle_get_driver() || cpuidle_get_driver() ==
>> > &acpi_idle_driver)
>> >                 acpi_processor_power_init(pr);
>> > --
>> > 2.7.4
>> >
>> > --
>> > To unsubscribe from this list: send the line "unsubscribe linux-
>> > acpi" in
>> > the body of a message to majordomo@vger.kernel.org
>> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> --
>> 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

Re: [PATCH 1/5] acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config

[Srinivas Pandruvada]

On Fri, 2016-08-12 at 09:58 -0700, Hoan Tran wrote:
> Hi Srinivas,
> 
> On Fri, Aug 12, 2016 at 9:52 AM, Srinivas Pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > On Fri, 2016-08-12 at 09:35 -0700, Hoan Tran wrote:
> > > 
> > > Hi Srinivas,
> > > 
> > > On Wed, Aug 10, 2016 at 5:17 PM, Srinivas Pandruvada
> > > <srinivas.pandruvada@linux.intel.com> wrote:
> > > > 
> > > > 
> > > > Some newer x86 platforms have support for both _CPC and _PSS
> > > > object. So
> > > > kernel config can have both ACPI_CPU_FREQ_PSS and
> > > > ACPI_CPPC_LIB. So
> > > > remove
> > > > restriction for ACPI_CPPC_LIB to build only when
> > > > ACPI_CPU_FREQ_PSS
> > > > is not
> > > > defined.
> > > > Also for legacy systems with only _PSS, we shouldn't bail out
> > > > if
> > > > acpi_cppc_processor_probe() fails, if ACPI_CPU_FREQ_PSS is also
> > > > defined.
> > > > 
> > > > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.i
> > > > ntel
> > > > .com>
> > > > ---
> > > >  drivers/acpi/Kconfig            | 1 -
> > > >  drivers/acpi/processor_driver.c | 5 ++++-
> > > >  2 files changed, 4 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> > > > index 445ce28..c6bb6aa 100644
> > > > --- a/drivers/acpi/Kconfig
> > > > +++ b/drivers/acpi/Kconfig
> > > > @@ -227,7 +227,6 @@ config ACPI_MCFG
> > > >  config ACPI_CPPC_LIB
> > > >         bool
> > > >         depends on ACPI_PROCESSOR
> > > > -       depends on !ACPI_CPU_FREQ_PSS
> > > From ACPI 6.1 spec, if _CPC is present, its use supersedes the
> > > use of
> > > PSS. So I think, config ACPI_CPU_FREQ_PSS should depend on
> > > !ACPI_CPPC_LIB.
> > > 
> > Distro want to have a single binary kernel image, so they will turn
> > on
> > all configs. So this is not a compile time decision.
> > On runtime if the ACPI contains both tables than _CPC should be
> > used
> > (but that also if the kernel is capable of handling _CPC, as legacy
> > kernel will not).
> Agreed. If so, this "depends on" is not need, right ? And a runtime
> code will be added to decide which is used instead ?
Yes, _CPC will be used if present. 

Thanks,
Srinivas

> 
> Thanks
> Hoan
> 
> > 
> > 
> > Thanks,
> > Srinivas
> > 
> > > 
> > > Thanks
> > > Hoan
> > > 
> > > > 
> > > > 
> > > >         select MAILBOX
> > > >         select PCC
> > > >         help
> > > > diff --git a/drivers/acpi/processor_driver.c
> > > > b/drivers/acpi/processor_driver.c
> > > > index 0553aee..0e0b629 100644
> > > > --- a/drivers/acpi/processor_driver.c
> > > > +++ b/drivers/acpi/processor_driver.c
> > > > @@ -245,8 +245,11 @@ static int __acpi_processor_start(struct
> > > > acpi_device *device)
> > > >                 return 0;
> > > > 
> > > >         result = acpi_cppc_processor_probe(pr);
> > > > -       if (result)
> > > > +       if (result) {
> > > > +#ifndef CONFIG_ACPI_CPU_FREQ_PSS
> > > >                 return -ENODEV;
> > > > +#endif
> > > > +       }
> > > > 
> > > >         if (!cpuidle_get_driver() || cpuidle_get_driver() ==
> > > > &acpi_idle_driver)
> > > >                 acpi_processor_power_init(pr);
> > > > --
> > > > 2.7.4
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe
> > > > linux-
> > > > acpi" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.h
> > > > tml
> > > --
> > > 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.htm
> > > l
> --
> 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

Re: [PATCH 5/5] acpi: bus: Enable HWP CPPC objects

[Alexey Klimov]

Hi Srinivas,

On Wed, Aug 10, 2016 at 05:17:26PM -0700, Srinivas Pandruvada wrote:
> Need to set platform wide _OSC bits CPC version 1 and version 2 bits
> so that BIOS presents CPPC objects.
> Even though the cppc_acpi supports only version 2, need to set both
> _OSC bits for version 1 and version 2, otherwise BIOS ignore _OSC
> settings.

Does such behaviour go against ACPI specs?
If yes, it's better to add comment in the code.
 
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
>  drivers/acpi/bus.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index 85b7d07..61643a5 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -330,6 +330,13 @@ static void acpi_bus_osc_support(void)
>  	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_HOTPLUG_OST_SUPPORT;
>  	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PCLPI_SUPPORT;
>  
> +#ifdef CONFIG_X86
> +	if (boot_cpu_has(X86_FEATURE_HWP)) {
> +		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT;
> +		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT;
> +	}
> +#endif

Just to check if I understand things correctly.
I thought that OSC method is kind of 'handshake'. OS describes features
that it supports and then platform indicates back what features it can
provide/support.
Here in this patch I see only first part and I don't see where you check
confirmation from platform that it supports/enabled CPPC.
You don't need that, do you?
It will be nice to see explanation.

Best regards,
Alexey.

>  	if (!ghes_disable)
>  		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
>  	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))
> -- 
> 2.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 5/5] acpi: bus: Enable HWP CPPC objects

[Srinivas Pandruvada]

Hi Alexey,

On Tue, 2016-08-16 at 15:32 +0100, Alexey Klimov wrote:
> Hi Srinivas,
> 
> On Wed, Aug 10, 2016 at 05:17:26PM -0700, Srinivas Pandruvada wrote:
> > 
> > Need to set platform wide _OSC bits CPC version 1 and version 2
> > bits
> > so that BIOS presents CPPC objects.
> > Even though the cppc_acpi supports only version 2, need to set both
> > _OSC bits for version 1 and version 2, otherwise BIOS ignore _OSC
> > settings.
> Does such behaviour go against ACPI specs?
> If yes, it's better to add comment in the code.
I need to fix the commit description here. The bit 5 is generic CPPC
support and bit 6 is CPPC_V2 support capability. So if we set only bit
5 then we can support only CPPC v1. If we set bit 6, we also support
CPPC v2. So we have to set both bits for CPPC v2 (Table 6-176 Platform-
Wide _OSC Capabilities DWORD 2 ACPI 6.0 spec). 
 
> > 
> > Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel
> > .com>
> > ---
> >  drivers/acpi/bus.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> > index 85b7d07..61643a5 100644
> > --- a/drivers/acpi/bus.c
> > +++ b/drivers/acpi/bus.c
> > @@ -330,6 +330,13 @@ static void acpi_bus_osc_support(void)
> >  	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_HOTPLUG_OST_SUPPORT;
> >  	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PCLPI_SUPPORT;
> >  
> > +#ifdef CONFIG_X86
> > +	if (boot_cpu_has(X86_FEATURE_HWP)) {
> > +		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPC_SUPPORT;
> > +		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_CPCV2_SUPPORT;
> > +	}
> > +#endif
> Just to check if I understand things correctly.
> I thought that OSC method is kind of 'handshake'. OS describes
> features
> that it supports and then platform indicates back what features it
> can
> provide/support.
> Here in this patch I see only first part and I don't see where you
> check
> confirmation from platform that it supports/enabled CPPC.
> You don't need that, do you?
Once we set these bits the BIOS will expose the CPPC tables. This will
result in acpi_cppc_processor_probe will be successful. Also _OSC
response will set bit 5 and 6 here to confirm the acceptance, but I
can't do any meaningful processing with that confirmation till
acpi_cppc_processor_probe() returns successfully, which is the
confirmation.
How the CPPC data is used in x86 as I indicated in the cover page will
be a separate series. I submitted CPPC ACPI changes before so that they
can be reviewed first.

Thanks,
Srinivas


> It will be nice to see explanation.
> 
> Best regards,
> Alexey.
> 
> > 
> >  	if (!ghes_disable)
> >  		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
> >  	if (ACPI_FAILURE(acpi_get_handle(NULL, "\\_SB", &handle)))

Re: [PATCH 0/5] x86 CPPC usage

[Srinivas Pandruvada]

On Wed, 2016-08-10 at 17:17 -0700, Srinivas Pandruvada wrote:
> The x86 HWP (Hardware Controlled Performance States) is an
> implementation
> of the ACPI-defined Collaborative Processor Performance Control
> (CPPC).
> The current implementation of HWP doesn't use CPPC, but CPPC brings
> in
> some advanced features. So we will be submitting changes to use CPPC.
> 
> The first series only contains ACPI CPPC changes required for x86.
> 
Since I didn't see any major objection to this series, I will resubmit
these changes along with the use case of CPPC on x86 to support turbo
feature.

Thanks,
Srinivas

> Srinivas Pandruvada (5):
>   acpi: cppc: Allow build with ACPI_CPU_FREQ_PSS config
>   acpi: cpcc: Add integer read support
>   acpi: cppc: Add support for function fixed hardware address
>   acpi: cppc: Add prefix cppc to cpudata structure name
>   acpi: bus: Enable HWP CPPC objects
> 
>  drivers/acpi/Kconfig            |  1 -
>  drivers/acpi/bus.c              |  7 ++++
>  drivers/acpi/cppc_acpi.c        | 88
> +++++++++++++++++++++++++++++++++++------
>  drivers/acpi/processor_driver.c |  5 ++-
>  drivers/cpufreq/cppc_cpufreq.c  | 14 +++----
>  include/acpi/cppc_acpi.h        |  4 +-
>  6 files changed, 95 insertions(+), 24 deletions(-)
>