[PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[Sudeep Holla]

ACPI 6.0 introduced LPI(Low Power Idle) states that provides an alternate
method to describe processor idle states. It extends the specification
to allow the expression of idle states like C-states selectable by the
OSPM when a processor goes idle, but may affect more than one processor,
and may affect other system components.

LPI extensions leverages the processor container device(again introduced
in ACPI 6.0) allowing to express which parts of the system are affected
by a given LPI state. It defines the local power states for each node
in a hierarchical processor topology. The OSPM can use _LPI object to
select a local power state for each level of processor hierarchy in the
system. They used to produce a composite power state request that is
presented to the platform by the OSPM.

Since multiple processors affect the idle state for any non-leaf hierarchy
node, coordination of idle state requests between the processors is
required. ACPI supports two different coordination schemes: Platform
coordinated and  OS initiated.

This series aims at providing basic and initial support for platform
coordinated LPI states.

v2[2]->v3:
	- rebased against v4.4-rc3
	- fixed couple of issues reported by Prashanth and review comments
	  from Ashwin

v1[1]->v2[2]:
	- Fixed support for ACPI0010 processor container
	- moved sleep state code out of processor_idle

Note the ARM64 specific changes are not part of this series as it's still
WIP and there are other consolidation happening in there. For reference
and testing, I have pushed a branch[3]

Regards,
Sudeep

[1] https://lkml.org/lkml/2015/8/4/789
[2] https://lkml.org/lkml/2015/9/16/422
[3] git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git arm64_lpi_testing


Sudeep Holla (5):
  ACPI / processor : add support for ACPI0010 processor container
  ACPI / sleep: move acpi_processor_sleep to sleep.c
  ACPI / processor_idle: replace PREFIX with pr_fmt
  ACPI / processor_idle : introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE
  ACPI / processor_idle: Add support for Low Power Idle(LPI) states

 arch/ia64/Kconfig               |   1 +
 arch/x86/Kconfig                |   1 +
 drivers/acpi/Kconfig            |   6 +
 drivers/acpi/acpi_processor.c   |  17 ++
 drivers/acpi/bus.c              |   8 +-
 drivers/acpi/processor_driver.c |   4 +-
 drivers/acpi/processor_idle.c   | 553 ++++++++++++++++++++++++++++++++--------
 drivers/acpi/sleep.c            |  35 +++
 include/acpi/processor.h        |  42 ++-
 include/linux/acpi.h            |   4 +
 10 files changed, 545 insertions(+), 126 deletions(-)

-- 
1.9.1

[PATCH v3 1/5] ACPI / processor : add support for ACPI0010 processor container

[Sudeep Holla]

ACPI 6.0 adds support for optional processor container device which may
contain child objects that are either processor devices or other processor
containers. This allows representing hierarchical processor topologies.

It is declared using the _HID of ACPI0010. It is an abstract container
used to represent CPU topology and should not be used to hotplug
purposes.

This patch enables the support for these ACPI processor containers and
ensures the generic container/module devices are not created for them.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/acpi_processor.c | 17 +++++++++++++++++
 include/acpi/processor.h      |  1 +
 2 files changed, 18 insertions(+)

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 6979186dbd4b..b5e54f2da53d 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -514,7 +514,24 @@ static struct acpi_scan_handler processor_handler = {
 	},
 };
 
+static int acpi_processor_container_attach(struct acpi_device *dev,
+					   const struct acpi_device_id *id)
+{
+	return 1;
+}
+
+static const struct acpi_device_id processor_container_ids[] = {
+	{ ACPI_PROCESSOR_CONTAINER_HID, },
+	{ }
+};
+
+static struct acpi_scan_handler processor_container_handler = {
+	.ids = processor_container_ids,
+	.attach = acpi_processor_container_attach,
+};
+
 void __init acpi_processor_init(void)
 {
 	acpi_scan_add_handler_with_hotplug(&processor_handler, "processor");
+	acpi_scan_add_handler(&processor_container_handler);
 }
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 07fb100bcc68..54d7860cac11 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -9,6 +9,7 @@
 #define ACPI_PROCESSOR_CLASS		"processor"
 #define ACPI_PROCESSOR_DEVICE_NAME	"Processor"
 #define ACPI_PROCESSOR_DEVICE_HID	"ACPI0007"
+#define ACPI_PROCESSOR_CONTAINER_HID	"ACPI0010"
 
 #define ACPI_PROCESSOR_BUSY_METRIC	10
 
-- 
1.9.1

Re: [PATCH v3 1/5] ACPI / processor : add support for ACPI0010 processor container

[Rafael J. Wysocki]

On Wednesday, December 02, 2015 02:10:42 PM Sudeep Holla wrote:
> ACPI 6.0 adds support for optional processor container device which may
> contain child objects that are either processor devices or other processor
> containers. This allows representing hierarchical processor topologies.
> 
> It is declared using the _HID of ACPI0010. It is an abstract container
> used to represent CPU topology and should not be used to hotplug
> purposes.
> 
> This patch enables the support for these ACPI processor containers and
> ensures the generic container/module devices are not created for them.

I wouldn't say that it "enables the support".  What it actually does is
to prevent platform devices from being created for processor container
objects.

I can apply it right away with this changelog modification, though.

Thanks,
Rafael

Re: [PATCH v3 1/5] ACPI / processor : add support for ACPI0010 processor container

[Sudeep Holla]

On 16/02/16 20:10, Rafael J. Wysocki wrote:
> On Wednesday, December 02, 2015 02:10:42 PM Sudeep Holla wrote:
>> ACPI 6.0 adds support for optional processor container device which may
>> contain child objects that are either processor devices or other processor
>> containers. This allows representing hierarchical processor topologies.
>>
>> It is declared using the _HID of ACPI0010. It is an abstract container
>> used to represent CPU topology and should not be used to hotplug
>> purposes.
>>
>> This patch enables the support for these ACPI processor containers and
>> ensures the generic container/module devices are not created for them.
>
> I wouldn't say that it "enables the support".  What it actually does is
> to prevent platform devices from being created for processor container
> objects.
>
> I can apply it right away with this changelog modification, though.
>

Ah my mistake, didn't change the log message from first version.
Will post the patch with update log, you can change it further if required.

-- 
Regards,
Sudeep

[UPDATE] [PATCH v3 1/5] ACPI / processor : add support for ACPI0010 processor container

[Sudeep Holla]

ACPI 6.0 adds support for optional processor container device which may
contain child objects that are either processor devices or other processor
containers. This allows representing hierarchical processor topologies.

It is declared using the _HID of ACPI0010. It is an abstract container
used to represent CPU topology and should not be used to hotplug
purposes.

If no matching handler is found for a device in acpi_scan_attach_handler,
acpi_bus_attach does a default enumeration for those devices with valid
HID in the acpi namespace. This patch adds a scan handler for these ACPI
processor containers to avoid default that enumeration and ensures the
platform devices are not created for them.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/acpi_processor.c | 17 +++++++++++++++++
 include/acpi/processor.h      |  1 +
 2 files changed, 18 insertions(+)

Update:
	- No change in the patch, just updated commit log

diff --git a/drivers/acpi/acpi_processor.c b/drivers/acpi/acpi_processor.c
index 6979186dbd4b..b5e54f2da53d 100644
--- a/drivers/acpi/acpi_processor.c
+++ b/drivers/acpi/acpi_processor.c
@@ -514,7 +514,24 @@ static struct acpi_scan_handler processor_handler = {
 	},
 };
 
+static int acpi_processor_container_attach(struct acpi_device *dev,
+					   const struct acpi_device_id *id)
+{
+	return 1;
+}
+
+static const struct acpi_device_id processor_container_ids[] = {
+	{ ACPI_PROCESSOR_CONTAINER_HID, },
+	{ }
+};
+
+static struct acpi_scan_handler processor_container_handler = {
+	.ids = processor_container_ids,
+	.attach = acpi_processor_container_attach,
+};
+
 void __init acpi_processor_init(void)
 {
 	acpi_scan_add_handler_with_hotplug(&processor_handler, "processor");
+	acpi_scan_add_handler(&processor_container_handler);
 }
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 07fb100bcc68..54d7860cac11 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -9,6 +9,7 @@
 #define ACPI_PROCESSOR_CLASS		"processor"
 #define ACPI_PROCESSOR_DEVICE_NAME	"Processor"
 #define ACPI_PROCESSOR_DEVICE_HID	"ACPI0007"
+#define ACPI_PROCESSOR_CONTAINER_HID	"ACPI0010"
 
 #define ACPI_PROCESSOR_BUSY_METRIC	10
 
-- 
1.9.1

Re: [UPDATE] [PATCH v3 1/5] ACPI / processor : add support for ACPI0010 processor container

[Rafael J. Wysocki]

On Wednesday, February 17, 2016 11:54:19 AM Sudeep Holla wrote:
> ACPI 6.0 adds support for optional processor container device which may
> contain child objects that are either processor devices or other processor
> containers. This allows representing hierarchical processor topologies.
> 
> It is declared using the _HID of ACPI0010. It is an abstract container
> used to represent CPU topology and should not be used to hotplug
> purposes.
> 
> If no matching handler is found for a device in acpi_scan_attach_handler,
> acpi_bus_attach does a default enumeration for those devices with valid
> HID in the acpi namespace. This patch adds a scan handler for these ACPI
> processor containers to avoid default that enumeration and ensures the
> platform devices are not created for them.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Queued up for 4.6, thanks!

Rafael

[PATCH v3 2/5] ACPI / sleep: move acpi_processor_sleep to sleep.c

[Sudeep Holla]

acpi_processor_sleep is neither related nor used by CPUIdle framework.
It's used in system suspend/resume path as a syscore operation. It makes
more sense to move it to acpi/sleep.c where all the S-state transition
(a.k.a. Linux system suspend/hiberate) related code are present.

Also make it depend on CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT so that
it's not compiled on architecture like ARM64 where S-states are not
yet defined in ACPI.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/processor_driver.c |  2 --
 drivers/acpi/processor_idle.c   | 37 -------------------------------------
 drivers/acpi/sleep.c            | 35 +++++++++++++++++++++++++++++++++++
 include/acpi/processor.h        |  8 --------
 4 files changed, 35 insertions(+), 47 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index f4e02ae93f58..29f787b2493f 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -313,7 +313,6 @@ static int __init acpi_processor_driver_init(void)
 	if (result < 0)
 		return result;
 
-	acpi_processor_syscore_init();
 	register_hotcpu_notifier(&acpi_cpu_notifier);
 	acpi_thermal_cpufreq_init();
 	acpi_processor_ppc_init();
@@ -329,7 +328,6 @@ static void __exit acpi_processor_driver_exit(void)
 	acpi_processor_ppc_exit();
 	acpi_thermal_cpufreq_exit();
 	unregister_hotcpu_notifier(&acpi_cpu_notifier);
-	acpi_processor_syscore_exit();
 	driver_unregister(&acpi_processor_driver);
 }
 
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 175c86bee3a9..a24067b850d2 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -30,7 +30,6 @@
 #include <linux/sched.h>       /* need_resched() */
 #include <linux/tick.h>
 #include <linux/cpuidle.h>
-#include <linux/syscore_ops.h>
 #include <acpi/processor.h>
 
 /*
@@ -194,42 +193,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 
 #endif
 
-#ifdef CONFIG_PM_SLEEP
-static u32 saved_bm_rld;
-
-static int acpi_processor_suspend(void)
-{
-	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &saved_bm_rld);
-	return 0;
-}
-
-static void acpi_processor_resume(void)
-{
-	u32 resumed_bm_rld = 0;
-
-	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &resumed_bm_rld);
-	if (resumed_bm_rld == saved_bm_rld)
-		return;
-
-	acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
-}
-
-static struct syscore_ops acpi_processor_syscore_ops = {
-	.suspend = acpi_processor_suspend,
-	.resume = acpi_processor_resume,
-};
-
-void acpi_processor_syscore_init(void)
-{
-	register_syscore_ops(&acpi_processor_syscore_ops);
-}
-
-void acpi_processor_syscore_exit(void)
-{
-	unregister_syscore_ops(&acpi_processor_syscore_ops);
-}
-#endif /* CONFIG_PM_SLEEP */
-
 #if defined(CONFIG_X86)
 static void tsc_check_state(int state)
 {
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 0d94621dc856..73604d8ccfad 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -19,6 +19,7 @@
 #include <linux/reboot.h>
 #include <linux/acpi.h>
 #include <linux/module.h>
+#include <linux/syscore_ops.h>
 #include <asm/io.h>
 #include <trace/events/power.h>
 
@@ -677,6 +678,39 @@ static void acpi_sleep_suspend_setup(void)
 static inline void acpi_sleep_suspend_setup(void) {}
 #endif /* !CONFIG_SUSPEND */
 
+#ifdef CONFIG_PM_SLEEP
+static u32 saved_bm_rld;
+
+static int acpi_processor_suspend(void)
+{
+	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &saved_bm_rld);
+	return 0;
+}
+
+static void acpi_processor_resume(void)
+{
+	u32 resumed_bm_rld = 0;
+
+	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &resumed_bm_rld);
+	if (resumed_bm_rld == saved_bm_rld)
+		return;
+
+	acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
+}
+
+static struct syscore_ops acpi_processor_syscore_ops = {
+	.suspend = acpi_processor_suspend,
+	.resume = acpi_processor_resume,
+};
+
+void acpi_processor_syscore_init(void)
+{
+	register_syscore_ops(&acpi_processor_syscore_ops);
+}
+#else
+static inline void acpi_processor_syscore_init(void) {}
+#endif /* CONFIG_PM_SLEEP */
+
 #ifdef CONFIG_HIBERNATION
 static unsigned long s4_hardware_signature;
 static struct acpi_table_facs *facs;
@@ -839,6 +873,7 @@ int __init acpi_sleep_init(void)
 
 	sleep_states[ACPI_STATE_S0] = 1;
 
+	acpi_processor_syscore_init();
 	acpi_sleep_suspend_setup();
 	acpi_sleep_hibernate_setup();
 
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 54d7860cac11..6f1805dd5d3c 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -395,14 +395,6 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
 }
 #endif /* CONFIG_ACPI_PROCESSOR_IDLE */
 
-#if defined(CONFIG_PM_SLEEP) & defined(CONFIG_ACPI_PROCESSOR_IDLE)
-void acpi_processor_syscore_init(void);
-void acpi_processor_syscore_exit(void);
-#else
-static inline void acpi_processor_syscore_init(void) {}
-static inline void acpi_processor_syscore_exit(void) {}
-#endif
-
 /* in processor_thermal.c */
 int acpi_processor_get_limit_info(struct acpi_processor *pr);
 extern const struct thermal_cooling_device_ops processor_cooling_ops;
-- 
1.9.1

Re: [PATCH v3 2/5] ACPI / sleep: move acpi_processor_sleep to sleep.c

[Rafael J. Wysocki]

On Wednesday, December 02, 2015 02:10:43 PM Sudeep Holla wrote:
> acpi_processor_sleep is neither related nor used by CPUIdle framework.
> It's used in system suspend/resume path as a syscore operation. It makes
> more sense to move it to acpi/sleep.c where all the S-state transition
> (a.k.a. Linux system suspend/hiberate) related code are present.
> 
> Also make it depend on CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT so that
> it's not compiled on architecture like ARM64 where S-states are not
> yet defined in ACPI.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

To me this goes in the right direction, but I'd take it a bit further.

> ---
>  drivers/acpi/processor_driver.c |  2 --
>  drivers/acpi/processor_idle.c   | 37 -------------------------------------
>  drivers/acpi/sleep.c            | 35 +++++++++++++++++++++++++++++++++++
>  include/acpi/processor.h        |  8 --------
>  4 files changed, 35 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index f4e02ae93f58..29f787b2493f 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -313,7 +313,6 @@ static int __init acpi_processor_driver_init(void)
>  	if (result < 0)
>  		return result;
>  
> -	acpi_processor_syscore_init();
>  	register_hotcpu_notifier(&acpi_cpu_notifier);
>  	acpi_thermal_cpufreq_init();
>  	acpi_processor_ppc_init();
> @@ -329,7 +328,6 @@ static void __exit acpi_processor_driver_exit(void)
>  	acpi_processor_ppc_exit();
>  	acpi_thermal_cpufreq_exit();
>  	unregister_hotcpu_notifier(&acpi_cpu_notifier);
> -	acpi_processor_syscore_exit();
>  	driver_unregister(&acpi_processor_driver);
>  }
>  
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 175c86bee3a9..a24067b850d2 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -30,7 +30,6 @@
>  #include <linux/sched.h>       /* need_resched() */
>  #include <linux/tick.h>
>  #include <linux/cpuidle.h>
> -#include <linux/syscore_ops.h>
>  #include <acpi/processor.h>
>  
>  /*
> @@ -194,42 +193,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
>  
>  #endif
>  
> -#ifdef CONFIG_PM_SLEEP
> -static u32 saved_bm_rld;
> -
> -static int acpi_processor_suspend(void)
> -{
> -	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &saved_bm_rld);
> -	return 0;
> -}
> -
> -static void acpi_processor_resume(void)
> -{
> -	u32 resumed_bm_rld = 0;
> -
> -	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &resumed_bm_rld);
> -	if (resumed_bm_rld == saved_bm_rld)
> -		return;
> -
> -	acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
> -}
> -
> -static struct syscore_ops acpi_processor_syscore_ops = {
> -	.suspend = acpi_processor_suspend,
> -	.resume = acpi_processor_resume,
> -};
> -
> -void acpi_processor_syscore_init(void)
> -{
> -	register_syscore_ops(&acpi_processor_syscore_ops);
> -}
> -
> -void acpi_processor_syscore_exit(void)
> -{
> -	unregister_syscore_ops(&acpi_processor_syscore_ops);
> -}
> -#endif /* CONFIG_PM_SLEEP */
> -
>  #if defined(CONFIG_X86)
>  static void tsc_check_state(int state)
>  {
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 0d94621dc856..73604d8ccfad 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -19,6 +19,7 @@
>  #include <linux/reboot.h>
>  #include <linux/acpi.h>
>  #include <linux/module.h>
> +#include <linux/syscore_ops.h>
>  #include <asm/io.h>
>  #include <trace/events/power.h>
>  
> @@ -677,6 +678,39 @@ static void acpi_sleep_suspend_setup(void)
>  static inline void acpi_sleep_suspend_setup(void) {}
>  #endif /* !CONFIG_SUSPEND */
>  
> +#ifdef CONFIG_PM_SLEEP
> +static u32 saved_bm_rld;
> +
> +static int acpi_processor_suspend(void)

Why do we need mention processor in the function name here (and below)?

I'd call it something like acpi_save/restore_bm_rld() (maybe with a short comment
explaining what the BM RLD is).

> +{
> +	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &saved_bm_rld);
> +	return 0;
> +}
> +
> +static void acpi_processor_resume(void)
> +{
> +	u32 resumed_bm_rld = 0;
> +
> +	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &resumed_bm_rld);
> +	if (resumed_bm_rld == saved_bm_rld)
> +		return;
> +
> +	acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
> +}
> +
> +static struct syscore_ops acpi_processor_syscore_ops = {
> +	.suspend = acpi_processor_suspend,
> +	.resume = acpi_processor_resume,
> +};
> +
> +void acpi_processor_syscore_init(void)
> +{
> +	register_syscore_ops(&acpi_processor_syscore_ops);
> +}
> +#else
> +static inline void acpi_processor_syscore_init(void) {}
> +#endif /* CONFIG_PM_SLEEP */
> +
>  #ifdef CONFIG_HIBERNATION
>  static unsigned long s4_hardware_signature;
>  static struct acpi_table_facs *facs;
> @@ -839,6 +873,7 @@ int __init acpi_sleep_init(void)
>  
>  	sleep_states[ACPI_STATE_S0] = 1;
>  
> +	acpi_processor_syscore_init();
>  	acpi_sleep_suspend_setup();
>  	acpi_sleep_hibernate_setup();
>  
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 54d7860cac11..6f1805dd5d3c 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -395,14 +395,6 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
>  }
>  #endif /* CONFIG_ACPI_PROCESSOR_IDLE */
>  
> -#if defined(CONFIG_PM_SLEEP) & defined(CONFIG_ACPI_PROCESSOR_IDLE)
> -void acpi_processor_syscore_init(void);
> -void acpi_processor_syscore_exit(void);
> -#else
> -static inline void acpi_processor_syscore_init(void) {}
> -static inline void acpi_processor_syscore_exit(void) {}
> -#endif
> -
>  /* in processor_thermal.c */
>  int acpi_processor_get_limit_info(struct acpi_processor *pr);
>  extern const struct thermal_cooling_device_ops processor_cooling_ops;
> 

Thanks,
Rafael

Re: [PATCH v3 2/5] ACPI / sleep: move acpi_processor_sleep to sleep.c

[Sudeep Holla]

On 16/02/16 20:13, Rafael J. Wysocki wrote:
> On Wednesday, December 02, 2015 02:10:43 PM Sudeep Holla wrote:
>> acpi_processor_sleep is neither related nor used by CPUIdle framework.
>> It's used in system suspend/resume path as a syscore operation. It makes
>> more sense to move it to acpi/sleep.c where all the S-state transition
>> (a.k.a. Linux system suspend/hiberate) related code are present.
>>
>> Also make it depend on CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT so that
>> it's not compiled on architecture like ARM64 where S-states are not
>> yet defined in ACPI.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> To me this goes in the right direction, but I'd take it a bit further.
>
>> ---
>>   drivers/acpi/processor_driver.c |  2 --
>>   drivers/acpi/processor_idle.c   | 37 -------------------------------------
>>   drivers/acpi/sleep.c            | 35 +++++++++++++++++++++++++++++++++++
>>   include/acpi/processor.h        |  8 --------
>>   4 files changed, 35 insertions(+), 47 deletions(-)
>>

[...]

>> @@ -677,6 +678,39 @@ static void acpi_sleep_suspend_setup(void)
>>   static inline void acpi_sleep_suspend_setup(void) {}
>>   #endif /* !CONFIG_SUSPEND */
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static u32 saved_bm_rld;
>> +
>> +static int acpi_processor_suspend(void)
>
> Why do we need mention processor in the function name here (and below)?
>
> I'd call it something like acpi_save/restore_bm_rld() (maybe with a short comment
> explaining what the BM RLD is).
>

Sure, I had thought so initially and wanted to do that in a separate
patch for easy of review but totally forgot later. Thanks for pointing
it out. Updated patch on it's way.

-- 
Regards,
Sudeep

[UPDATE] [PATCH v3 2/5] ACPI / sleep: move acpi_processor_sleep to sleep.c

[Sudeep Holla]

acpi_processor_sleep is neither related nor used by CPUIdle framework.
It's used in system suspend/resume path as a syscore operation. It makes
more sense to move it to acpi/sleep.c where all the S-state transition
(a.k.a. Linux system suspend/hiberate) related code are present.

Also make it depend on CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT so that
it's not compiled on architecture like ARM64 where S-states are not
yet defined in ACPI.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/processor_driver.c |  2 --
 drivers/acpi/processor_idle.c   | 37 -------------------------------------
 drivers/acpi/sleep.c            | 35 +++++++++++++++++++++++++++++++++++
 include/acpi/processor.h        |  8 --------
 4 files changed, 35 insertions(+), 47 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 11154a330f07..d2fa8cb82d2b 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -314,7 +314,6 @@ static int __init acpi_processor_driver_init(void)
 	if (result < 0)
 		return result;
 
-	acpi_processor_syscore_init();
 	register_hotcpu_notifier(&acpi_cpu_notifier);
 	acpi_thermal_cpufreq_init();
 	acpi_processor_ppc_init();
@@ -330,7 +329,6 @@ static void __exit acpi_processor_driver_exit(void)
 	acpi_processor_ppc_exit();
 	acpi_thermal_cpufreq_exit();
 	unregister_hotcpu_notifier(&acpi_cpu_notifier);
-	acpi_processor_syscore_exit();
 	driver_unregister(&acpi_processor_driver);
 }
 
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index e057aa8fafb0..fadce354d2b7 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -31,7 +31,6 @@
 #include <linux/sched.h>       /* need_resched() */
 #include <linux/tick.h>
 #include <linux/cpuidle.h>
-#include <linux/syscore_ops.h>
 #include <acpi/processor.h>
 
 /*
@@ -193,42 +192,6 @@ static void lapic_timer_state_broadcast(struct acpi_processor *pr,
 
 #endif
 
-#ifdef CONFIG_PM_SLEEP
-static u32 saved_bm_rld;
-
-static int acpi_processor_suspend(void)
-{
-	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &saved_bm_rld);
-	return 0;
-}
-
-static void acpi_processor_resume(void)
-{
-	u32 resumed_bm_rld = 0;
-
-	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &resumed_bm_rld);
-	if (resumed_bm_rld == saved_bm_rld)
-		return;
-
-	acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
-}
-
-static struct syscore_ops acpi_processor_syscore_ops = {
-	.suspend = acpi_processor_suspend,
-	.resume = acpi_processor_resume,
-};
-
-void acpi_processor_syscore_init(void)
-{
-	register_syscore_ops(&acpi_processor_syscore_ops);
-}
-
-void acpi_processor_syscore_exit(void)
-{
-	unregister_syscore_ops(&acpi_processor_syscore_ops);
-}
-#endif /* CONFIG_PM_SLEEP */
-
 #if defined(CONFIG_X86)
 static void tsc_check_state(int state)
 {
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 9cb975200cac..fbfcce3b5227 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -19,6 +19,7 @@
 #include <linux/reboot.h>
 #include <linux/acpi.h>
 #include <linux/module.h>
+#include <linux/syscore_ops.h>
 #include <asm/io.h>
 #include <trace/events/power.h>
 
@@ -677,6 +678,39 @@ static void acpi_sleep_suspend_setup(void)
 static inline void acpi_sleep_suspend_setup(void) {}
 #endif /* !CONFIG_SUSPEND */
 
+#ifdef CONFIG_PM_SLEEP
+static u32 saved_bm_rld;
+
+static int  acpi_save_bm_rld(void)
+{
+	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &saved_bm_rld);
+	return 0;
+}
+
+static void  acpi_restore_bm_rld(void)
+{
+	u32 resumed_bm_rld = 0;
+
+	acpi_read_bit_register(ACPI_BITREG_BUS_MASTER_RLD, &resumed_bm_rld);
+	if (resumed_bm_rld == saved_bm_rld)
+		return;
+
+	acpi_write_bit_register(ACPI_BITREG_BUS_MASTER_RLD, saved_bm_rld);
+}
+
+static struct syscore_ops acpi_sleep_syscore_ops = {
+	.suspend = acpi_save_bm_rld,
+	.resume = acpi_restore_bm_rld,
+};
+
+void acpi_sleep_syscore_init(void)
+{
+	register_syscore_ops(&acpi_sleep_syscore_ops);
+}
+#else
+static inline void acpi_sleep_syscore_init(void) {}
+#endif /* CONFIG_PM_SLEEP */
+
 #ifdef CONFIG_HIBERNATION
 static unsigned long s4_hardware_signature;
 static struct acpi_table_facs *facs;
@@ -839,6 +873,7 @@ int __init acpi_sleep_init(void)
 
 	sleep_states[ACPI_STATE_S0] = 1;
 
+	acpi_sleep_syscore_init();
 	acpi_sleep_suspend_setup();
 	acpi_sleep_hibernate_setup();
 
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 54d7860cac11..6f1805dd5d3c 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -395,14 +395,6 @@ static inline int acpi_processor_hotplug(struct acpi_processor *pr)
 }
 #endif /* CONFIG_ACPI_PROCESSOR_IDLE */
 
-#if defined(CONFIG_PM_SLEEP) & defined(CONFIG_ACPI_PROCESSOR_IDLE)
-void acpi_processor_syscore_init(void);
-void acpi_processor_syscore_exit(void);
-#else
-static inline void acpi_processor_syscore_init(void) {}
-static inline void acpi_processor_syscore_exit(void) {}
-#endif
-
 /* in processor_thermal.c */
 int acpi_processor_get_limit_info(struct acpi_processor *pr);
 extern const struct thermal_cooling_device_ops processor_cooling_ops;
-- 
1.9.1

Re: [UPDATE] [PATCH v3 2/5] ACPI / sleep: move acpi_processor_sleep to sleep.c

[Rafael J. Wysocki]

On Wednesday, February 17, 2016 12:03:23 PM Sudeep Holla wrote:
> acpi_processor_sleep is neither related nor used by CPUIdle framework.
> It's used in system suspend/resume path as a syscore operation. It makes
> more sense to move it to acpi/sleep.c where all the S-state transition
> (a.k.a. Linux system suspend/hiberate) related code are present.
> 
> Also make it depend on CONFIG_ACPI_SYSTEM_POWER_STATES_SUPPORT so that
> it's not compiled on architecture like ARM64 where S-states are not
> yet defined in ACPI.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Queued up for 4.6, thanks!

Rafael

[PATCH v3 3/5] ACPI / processor_idle: replace PREFIX with pr_fmt

[Sudeep Holla]

Like few of the other ACPI modules, replace PREFIX with pr_fmt and
change all the printk call sites to use pr_* companion functions
in processor_idle.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/processor_idle.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index a24067b850d2..fadce354d2b7 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -23,6 +23,7 @@
  *
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
+#define pr_fmt(fmt) "ACPI: " fmt
 
 #include <linux/module.h>
 #include <linux/acpi.h>
@@ -42,8 +43,6 @@
 #include <asm/apic.h>
 #endif
 
-#define PREFIX "ACPI: "
-
 #define ACPI_PROCESSOR_CLASS            "processor"
 #define _COMPONENT              ACPI_PROCESSOR_COMPONENT
 ACPI_MODULE_NAME("processor_idle");
@@ -80,9 +79,9 @@ static int set_max_cstate(const struct dmi_system_id *id)
 	if (max_cstate > ACPI_PROCESSOR_MAX_POWER)
 		return 0;
 
-	printk(KERN_NOTICE PREFIX "%s detected - limiting to C%ld max_cstate."
-	       " Override with \"processor.max_cstate=%d\"\n", id->ident,
-	       (long)id->driver_data, ACPI_PROCESSOR_MAX_POWER + 1);
+	pr_notice("%s detected - limiting to C%ld max_cstate."
+		  " Override with \"processor.max_cstate=%d\"\n", id->ident,
+		  (long)id->driver_data, ACPI_PROCESSOR_MAX_POWER + 1);
 
 	max_cstate = (long)id->driver_data;
 
@@ -314,7 +313,7 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 
 	/* There must be at least 2 elements */
 	if (!cst || (cst->type != ACPI_TYPE_PACKAGE) || cst->package.count < 2) {
-		printk(KERN_ERR PREFIX "not enough elements in _CST\n");
+		pr_err("not enough elements in _CST\n");
 		ret = -EFAULT;
 		goto end;
 	}
@@ -323,7 +322,7 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 
 	/* Validate number of power states. */
 	if (count < 1 || count != cst->package.count - 1) {
-		printk(KERN_ERR PREFIX "count given by _CST is not valid\n");
+		pr_err("count given by _CST is not valid\n");
 		ret = -EFAULT;
 		goto end;
 	}
@@ -432,11 +431,9 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 		 * (From 1 through ACPI_PROCESSOR_MAX_POWER - 1)
 		 */
 		if (current_count >= (ACPI_PROCESSOR_MAX_POWER - 1)) {
-			printk(KERN_WARNING
-			       "Limiting number of power states to max (%d)\n",
-			       ACPI_PROCESSOR_MAX_POWER);
-			printk(KERN_WARNING
-			       "Please increase ACPI_PROCESSOR_MAX_POWER if needed.\n");
+			pr_warn("Limiting number of power states to max (%d)\n",
+				ACPI_PROCESSOR_MAX_POWER);
+			pr_warn("Please increase ACPI_PROCESSOR_MAX_POWER if needed.\n");
 			break;
 		}
 	}
@@ -1060,8 +1057,8 @@ int acpi_processor_power_init(struct acpi_processor *pr)
 			retval = cpuidle_register_driver(&acpi_idle_driver);
 			if (retval)
 				return retval;
-			printk(KERN_DEBUG "ACPI: %s registered with cpuidle\n",
-					acpi_idle_driver.name);
+			pr_debug("%s registered with cpuidle\n",
+				 acpi_idle_driver.name);
 		}
 
 		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
-- 
1.9.1

Re: [PATCH v3 3/5] ACPI / processor_idle: replace PREFIX with pr_fmt

[Rafael J. Wysocki]

On Wednesday, December 02, 2015 02:10:44 PM Sudeep Holla wrote:
> Like few of the other ACPI modules, replace PREFIX with pr_fmt and
> change all the printk call sites to use pr_* companion functions
> in processor_idle.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

I've queued up this one for 4.6.

Thanks,
Rafael

[PATCH v3 4/5] ACPI / processor_idle : introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE

[Sudeep Holla]

ACPI 6.0 adds a new method to specify the CPU idle states(C-states)
called Low Power Idle(LPI) states. Since new architectures like ARM64
use only LPIs, introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE to
encapsulate all the code supporting the old style C-states(_CST)

This patch will help to extend the processor_idle module to support
LPI.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/ia64/Kconfig             |  1 +
 arch/x86/Kconfig              |  1 +
 drivers/acpi/Kconfig          |  3 +++
 drivers/acpi/processor_idle.c | 57 +++++++++++++++++++++++++++++++------------
 include/acpi/processor.h      |  3 ++-
 5 files changed, 49 insertions(+), 16 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index eb0249e37981..dbfa3c3a49e1 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -16,6 +16,7 @@ config IA64
 	select PCI if (!IA64_HP_SIM)
 	select ACPI if (!IA64_HP_SIM)
 	select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
+	select ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE if ACPI
 	select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI
 	select HAVE_UNSTABLE_SCHED_CLOCK
 	select HAVE_IDE
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index db3622f22b61..70cd310a447e 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -34,6 +34,7 @@ config X86
 	select ARCH_MIGHT_HAVE_ACPI_PDC		if ACPI
 	select ARCH_MIGHT_HAVE_PC_PARPORT
 	select ARCH_MIGHT_HAVE_PC_SERIO
+	select ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE if ACPI
 	select ARCH_SUPPORTS_ATOMIC_RMW
 	select ARCH_SUPPORTS_DEFERRED_STRUCT_PAGE_INIT
 	select ARCH_SUPPORTS_INT128		if X86_64
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 25dbb76c02cc..12873f0b8141 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -48,6 +48,9 @@ config ACPI_LEGACY_TABLES_LOOKUP
 config ARCH_MIGHT_HAVE_ACPI_PDC
 	bool
 
+config ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE
+	bool
+
 config ACPI_GENERIC_GSI
 	bool
 
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index fadce354d2b7..9ca840c88f48 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -59,6 +59,12 @@ module_param(latency_factor, uint, 0644);
 
 static DEFINE_PER_CPU(struct cpuidle_device *, acpi_cpuidle_device);
 
+struct cpuidle_driver acpi_idle_driver = {
+	.name =		"acpi_idle",
+	.owner =	THIS_MODULE,
+};
+
+#ifdef CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE
 static DEFINE_PER_CPU(struct acpi_processor_cx * [CPUIDLE_STATE_MAX],
 								acpi_cstate);
 
@@ -804,11 +810,6 @@ static void acpi_idle_enter_freeze(struct cpuidle_device *dev,
 	acpi_idle_do_entry(cx);
 }
 
-struct cpuidle_driver acpi_idle_driver = {
-	.name =		"acpi_idle",
-	.owner =	THIS_MODULE,
-};
-
 /**
  * acpi_processor_setup_cpuidle_cx - prepares and configures CPUIDLE
  * device i.e. per-cpu data
@@ -925,6 +926,41 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
 	return 0;
 }
 
+static inline void acpi_processor_cstate_first_run_checks(void)
+{
+	static int first_run;
+
+	if (first_run)
+		return;
+	dmi_check_system(processor_power_dmi_table);
+	max_cstate = acpi_processor_cstate_check(max_cstate);
+	if (max_cstate < ACPI_C_STATES_MAX)
+		pr_notice("ACPI: processor limited to max C-state %d\n",
+			  max_cstate);
+	first_run++;
+}
+#else
+
+static inline int disabled_by_idle_boot_param(void) { return 0; }
+static inline void acpi_processor_cstate_first_run_checks(void) { }
+static int acpi_processor_get_power_info(struct acpi_processor *pr)
+{
+	return -ENODEV;
+}
+
+static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
+					   struct cpuidle_device *dev)
+{
+	return -EINVAL;
+}
+
+static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
+{
+	return -EINVAL;
+}
+
+#endif
+
 int acpi_processor_hotplug(struct acpi_processor *pr)
 {
 	int ret = 0;
@@ -1018,20 +1054,11 @@ int acpi_processor_power_init(struct acpi_processor *pr)
 	acpi_status status;
 	int retval;
 	struct cpuidle_device *dev;
-	static int first_run;
 
 	if (disabled_by_idle_boot_param())
 		return 0;
 
-	if (!first_run) {
-		dmi_check_system(processor_power_dmi_table);
-		max_cstate = acpi_processor_cstate_check(max_cstate);
-		if (max_cstate < ACPI_C_STATES_MAX)
-			printk(KERN_NOTICE
-			       "ACPI: processor limited to max C-state %d\n",
-			       max_cstate);
-		first_run++;
-	}
+	acpi_processor_cstate_first_run_checks();
 
 	if (acpi_gbl_FADT.cst_control && !nocst) {
 		status =
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 6f1805dd5d3c..50f2423d31fa 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -242,7 +242,8 @@ extern int acpi_processor_get_performance_info(struct acpi_processor *pr);
 DECLARE_PER_CPU(struct acpi_processor *, processors);
 extern struct acpi_processor_errata errata;
 
-#ifdef ARCH_HAS_POWER_INIT
+#if defined(ARCH_HAS_POWER_INIT) && \
+	defined(CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE)
 void acpi_processor_power_init_bm_check(struct acpi_processor_flags *flags,
 					unsigned int cpu);
 int acpi_processor_ffh_cstate_probe(unsigned int cpu,
-- 
1.9.1

Re: [PATCH v3 4/5] ACPI / processor_idle : introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE

[Rafael J. Wysocki]

On Wednesday, December 02, 2015 02:10:45 PM Sudeep Holla wrote:
> ACPI 6.0 adds a new method to specify the CPU idle states(C-states)
> called Low Power Idle(LPI) states. Since new architectures like ARM64
> use only LPIs, introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE to
> encapsulate all the code supporting the old style C-states(_CST)

No.

The way it really should work is to check if the firmware supports LPI
(and what kind of it) and try to use _CST if LPI is not supported.

If LPI is supported by the firmware, use it if the LPI objects are
present as expected or fall back to using _CST otherwise.

This way it all should work without any new Kconfig options.

Thanks,
Rafael

Re: [PATCH v3 4/5] ACPI / processor_idle : introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE

[Sudeep Holla]

On 16/02/16 20:18, Rafael J. Wysocki wrote:
> On Wednesday, December 02, 2015 02:10:45 PM Sudeep Holla wrote:
>> ACPI 6.0 adds a new method to specify the CPU idle states(C-states)
>> called Low Power Idle(LPI) states. Since new architectures like ARM64
>> use only LPIs, introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE to
>> encapsulate all the code supporting the old style C-states(_CST)
>
> No.
>
> The way it really should work is to check if the firmware supports LPI
> (and what kind of it) and try to use _CST if LPI is not supported.
>

Agreed

> If LPI is supported by the firmware, use it if the LPI objects are
> present as expected or fall back to using _CST otherwise.
>

I have something similar but I need to reverse the order I think.

> This way it all should work without any new Kconfig options.
>

I agree with you in terms of avoiding new Kconfig option. However the
main reason for adding it is to avoid declaring dummy functions and
variables on ARM64.

It's hard to justify the maintainers as it's totally useless on ARM64.
E.g. boot_option_idle_override, IDLE_NOMWAIT, acpi_unlazy_tlb,
arch_safe_halt.

Other option is to push those under CONFIG_X86, but then I don't have
much idea on what are all needed for IA64, so took an option that
encapsulates everything under CSTATE feature Kconfig, which is not user
visible and selected by archs supporting it by default.

I am open to any other alternative.

-- 
Regards,
Sudeep

Re: [PATCH v3 4/5] ACPI / processor_idle : introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE

[Sudeep Holla]

Hi Rafael,

On 17/02/16 12:21, Sudeep Holla wrote:
>
>
> On 16/02/16 20:18, Rafael J. Wysocki wrote:

[..]

>
>> This way it all should work without any new Kconfig options.
>>
>
> I agree with you in terms of avoiding new Kconfig option. However the
> main reason for adding it is to avoid declaring dummy functions and
> variables on ARM64.
>
> It's hard to justify the maintainers as it's totally useless on ARM64.
> E.g. boot_option_idle_override, IDLE_NOMWAIT, acpi_unlazy_tlb,
> arch_safe_halt.
>
> Other option is to push those under CONFIG_X86, but then I don't have
> much idea on what are all needed for IA64, so took an option that
> encapsulates everything under CSTATE feature Kconfig, which is not user
> visible and selected by archs supporting it by default.
>
> I am open to any other alternative.
>

Whatever alternative methods I tried so far ended up much horrible than
this. So any suggestions are much appreciated.

-- 
Regards,
Sudeep

Re: [PATCH v3 4/5] ACPI / processor_idle : introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE

[Rafael J. Wysocki]

On Mon, Feb 22, 2016 at 2:46 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> Hi Rafael,
>
> On 17/02/16 12:21, Sudeep Holla wrote:
>>
>>
>>
>> On 16/02/16 20:18, Rafael J. Wysocki wrote:
>
>
> [..]
>
>>
>>> This way it all should work without any new Kconfig options.
>>>
>>
>> I agree with you in terms of avoiding new Kconfig option. However the
>> main reason for adding it is to avoid declaring dummy functions and
>> variables on ARM64.
>>
>> It's hard to justify the maintainers as it's totally useless on ARM64.
>> E.g. boot_option_idle_override, IDLE_NOMWAIT, acpi_unlazy_tlb,
>> arch_safe_halt.
>>
>> Other option is to push those under CONFIG_X86, but then I don't have
>> much idea on what are all needed for IA64, so took an option that
>> encapsulates everything under CSTATE feature Kconfig, which is not user
>> visible and selected by archs supporting it by default.
>>
>> I am open to any other alternative.
>>
>
> Whatever alternative methods I tried so far ended up much horrible than
> this. So any suggestions are much appreciated.

Well, you have to give me some time here, sorry.

I'm still not done with cpufreq at this point and it's going to
consume some more cycles I'm afraid.

Thanks,
Rafael

Re: [PATCH v3 4/5] ACPI / processor_idle : introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE

[Sudeep Holla]

On 22/02/16 23:28, Rafael J. Wysocki wrote:
> On Mon, Feb 22, 2016 at 2:46 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
>> Hi Rafael,
>>
>> On 17/02/16 12:21, Sudeep Holla wrote:

[...]

>>
>> Whatever alternative methods I tried so far ended up much horrible than
>> this. So any suggestions are much appreciated.
>
> Well, you have to give me some time here, sorry.
>

Sorry for the nag.

> I'm still not done with cpufreq at this point and it's going to
> consume some more cycles I'm afraid.
>

I understand, will wait patiently :)

-- 
Regards,
Sudeep

[PATCH v3 5/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states

[Sudeep Holla]

ACPI 6.0 introduced an optional object _LPI that provides an alternate
method to describe Low Power Idle states. It defines the local power
states for each node in a hierarchical processor topology. The OSPM can
use _LPI object to select a local power state for each level of processor
hierarchy in the system. They used to produce a composite power state
request that is presented to the platform by the OSPM.

Since multiple processors affect the idle state for any non-leaf hierarchy
node, coordination of idle state requests between the processors is
required. ACPI supports two different coordination schemes: Platform
coordinated and  OS initiated.

This patch adds initial support for Platform coordination scheme of LPI.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/Kconfig            |   3 +
 drivers/acpi/bus.c              |   8 +-
 drivers/acpi/processor_driver.c |   2 +-
 drivers/acpi/processor_idle.c   | 440 +++++++++++++++++++++++++++++++++++-----
 include/acpi/processor.h        |  30 ++-
 include/linux/acpi.h            |   4 +
 6 files changed, 435 insertions(+), 52 deletions(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 12873f0b8141..89a2d9b81feb 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -51,6 +51,9 @@ config ARCH_MIGHT_HAVE_ACPI_PDC
 config ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE
 	bool
 
+config ARCH_SUPPORTS_ACPI_PROCESSOR_LPI
+	bool
+
 config ACPI_GENERIC_GSI
 	bool
 
diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index a212cefae524..2e9e2e3fde6a 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -301,6 +301,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
 EXPORT_SYMBOL(acpi_run_osc);
 
 bool osc_sb_apei_support_acked;
+bool osc_pc_lpi_support_acked;
 static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
 static void acpi_bus_osc_support(void)
 {
@@ -321,6 +322,8 @@ static void acpi_bus_osc_support(void)
 		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PPC_OST_SUPPORT;
 
 	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_HOTPLUG_OST_SUPPORT;
+	if (IS_ENABLED(CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_LPI))
+		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PCLPI_SUPPORT;
 
 	if (!ghes_disable)
 		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
@@ -328,9 +331,12 @@ static void acpi_bus_osc_support(void)
 		return;
 	if (ACPI_SUCCESS(acpi_run_osc(handle, &context))) {
 		u32 *capbuf_ret = context.ret.pointer;
-		if (context.ret.length > OSC_SUPPORT_DWORD)
+		if (context.ret.length > OSC_SUPPORT_DWORD) {
 			osc_sb_apei_support_acked =
 				capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
+			osc_pc_lpi_support_acked =
+				capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
+		}
 		kfree(context.ret.pointer);
 	}
 	/* do we need to check other returned cap? Sounds no */
diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 29f787b2493f..bfc59de0ce6b 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -90,7 +90,7 @@ static void acpi_processor_notify(acpi_handle handle, u32 event, void *data)
 						  pr->performance_platform_limit);
 		break;
 	case ACPI_PROCESSOR_NOTIFY_POWER:
-		acpi_processor_cst_has_changed(pr);
+		acpi_processor_power_state_has_changed(pr);
 		acpi_bus_generate_netlink_event(device->pnp.device_class,
 						  dev_name(&device->dev), event, 0);
 		break;
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 9ca840c88f48..73192c08bc8c 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -576,7 +576,7 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
 	return (working);
 }
 
-static int acpi_processor_get_power_info(struct acpi_processor *pr)
+static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
 {
 	unsigned int i;
 	int result;
@@ -810,31 +810,12 @@ static void acpi_idle_enter_freeze(struct cpuidle_device *dev,
 	acpi_idle_do_entry(cx);
 }
 
-/**
- * acpi_processor_setup_cpuidle_cx - prepares and configures CPUIDLE
- * device i.e. per-cpu data
- *
- * @pr: the ACPI processor
- * @dev : the cpuidle device
- */
 static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
 					   struct cpuidle_device *dev)
 {
 	int i, count = CPUIDLE_DRIVER_STATE_START;
 	struct acpi_processor_cx *cx;
 
-	if (!pr->flags.power_setup_done)
-		return -EINVAL;
-
-	if (pr->flags.power == 0) {
-		return -EINVAL;
-	}
-
-	if (!dev)
-		return -EINVAL;
-
-	dev->cpu = pr->id;
-
 	if (max_cstate == 0)
 		max_cstate = 1;
 
@@ -857,31 +838,13 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
 	return 0;
 }
 
-/**
- * acpi_processor_setup_cpuidle states- prepares and configures cpuidle
- * global state data i.e. idle routines
- *
- * @pr: the ACPI processor
- */
-static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
+static int acpi_processor_setup_cstates(struct acpi_processor *pr)
 {
 	int i, count = CPUIDLE_DRIVER_STATE_START;
 	struct acpi_processor_cx *cx;
 	struct cpuidle_state *state;
 	struct cpuidle_driver *drv = &acpi_idle_driver;
 
-	if (!pr->flags.power_setup_done)
-		return -EINVAL;
-
-	if (pr->flags.power == 0)
-		return -EINVAL;
-
-	drv->safe_state_index = -1;
-	for (i = CPUIDLE_DRIVER_STATE_START; i < CPUIDLE_STATE_MAX; i++) {
-		drv->states[i].name[0] = '\0';
-		drv->states[i].desc[0] = '\0';
-	}
-
 	if (max_cstate == 0)
 		max_cstate = 1;
 
@@ -893,7 +856,7 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
 
 		state = &drv->states[count];
 		snprintf(state->name, CPUIDLE_NAME_LEN, "C%d", i);
-		strncpy(state->desc, cx->desc, CPUIDLE_DESC_LEN);
+		strlcpy(state->desc, cx->desc, CPUIDLE_DESC_LEN);
 		state->exit_latency = cx->latency;
 		state->target_residency = cx->latency * latency_factor;
 		state->enter = acpi_idle_enter;
@@ -943,24 +906,407 @@ static inline void acpi_processor_cstate_first_run_checks(void)
 
 static inline int disabled_by_idle_boot_param(void) { return 0; }
 static inline void acpi_processor_cstate_first_run_checks(void) { }
-static int acpi_processor_get_power_info(struct acpi_processor *pr)
+static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
 {
 	return -ENODEV;
 }
-
 static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
 					   struct cpuidle_device *dev)
 {
 	return -EINVAL;
 }
 
-static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
+static int acpi_processor_setup_cstates(struct acpi_processor *pr)
+{
+	return -EINVAL;
+}
+#endif
+
+#ifdef CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_LPI
+
+#define ACPI_LPI_STATE_FLAGS_ENABLED			BIT(0)
+
+struct acpi_processor_lpi_info {
+	int state_count;
+	struct acpi_processor_lpi *lpix;
+};
+
+static int acpi_processor_evaluate_lpi(acpi_handle handle,
+				       struct acpi_processor_lpi_info *info)
+{
+	acpi_status status = 0;
+	int ret;
+	int version, level, pkg_count, state_idx = 1, loop;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *lpi;
+	struct acpi_processor_lpi *lpix;
+
+	status = acpi_evaluate_object(handle, "_LPI", NULL, &buffer);
+	if (ACPI_FAILURE(status)) {
+		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No _LPI, giving up\n"));
+		return -ENODEV;
+	}
+
+	lpi = buffer.pointer;
+
+	/* There must be at least 4 elements = 3 elements + 1 package */
+	if (!lpi || lpi->type != ACPI_TYPE_PACKAGE || lpi->package.count < 4) {
+		pr_info("not enough elements in _LPI\n");
+		ret = -EFAULT;
+		goto end;
+	}
+
+	version = lpi->package.elements[0].integer.value;
+	level = lpi->package.elements[1].integer.value;
+	pkg_count = lpi->package.elements[2].integer.value;
+
+	/* Validate number of power states. */
+	if (pkg_count < 1 || pkg_count != lpi->package.count - 3) {
+		pr_err("count given by _LPI is not valid\n");
+		ret = -EFAULT;
+		goto end;
+	}
+
+	lpix = kcalloc(pkg_count, sizeof(*lpix), GFP_KERNEL);
+	if (!lpix) {
+		ret = -ENOMEM;
+		goto end;
+	}
+
+	info->state_count = pkg_count;
+	info->lpix = lpix;
+	/* LPI States start at index 3 */
+	for (loop = 3; state_idx <= pkg_count; loop++, state_idx++, lpix++) {
+		union acpi_object *element, *obj;
+
+		element = &lpi->package.elements[loop];
+		if (element->type != ACPI_TYPE_PACKAGE)
+			continue;
+
+		if (element->package.count < 7)
+			continue;
+
+		/* TODO this long list is looking insane now
+		 * need a cleaner and saner way to read the elements
+		 */
+		obj = &element->package.elements[6];
+		if (obj->type == ACPI_TYPE_BUFFER) {
+			struct acpi_power_register *reg;
+
+			reg = (struct acpi_power_register *)obj->buffer.pointer;
+			if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO &&
+			    (reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE))
+				continue;
+			lpix->address = reg->address;
+			if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE)
+				lpix->entry_method = ACPI_CSTATE_FFH;
+			else
+				lpix->entry_method = ACPI_CSTATE_SYSTEMIO;
+		} else if (obj->type == ACPI_TYPE_INTEGER) {
+			lpix->entry_method = ACPI_CSTATE_INTEGER;
+			lpix->address = obj->integer.value;
+		} else {
+			continue;
+		}
+
+		/* elements[7,8] skipped for now i.e. Residency/Usage counter*/
+
+		obj = &element->package.elements[9];
+		if (obj->type == ACPI_TYPE_STRING)
+			strlcpy(lpix->desc, obj->string.pointer,
+				ACPI_CX_DESC_LEN);
+
+		lpix->index = state_idx;
+
+		obj = &element->package.elements[0];
+		if (obj->type != ACPI_TYPE_INTEGER)
+			continue;
+		lpix->min_residency = obj->integer.value;
+
+		obj = &element->package.elements[1];
+		if (obj->type != ACPI_TYPE_INTEGER)
+			continue;
+		lpix->wake_latency = obj->integer.value;
+
+		obj = &element->package.elements[2];
+		if (obj->type != ACPI_TYPE_INTEGER)
+			continue;
+		lpix->flags = obj->integer.value;
+
+		obj = &element->package.elements[3];
+		if (obj->type != ACPI_TYPE_INTEGER)
+			continue;
+		lpix->arch_flags = obj->integer.value;
+
+		obj = &element->package.elements[4];
+		if (obj->type != ACPI_TYPE_INTEGER)
+			continue;
+		lpix->res_cnt_freq = obj->integer.value;
+
+		obj = &element->package.elements[5];
+		if (obj->type != ACPI_TYPE_INTEGER)
+			continue;
+		lpix->enable_parent_state = obj->integer.value;
+	}
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d power states\n",
+			  state_idx));
+end:
+	kfree(buffer.pointer);
+	return status;
+}
+
+static int max_leaf_depth, fl_scnt;
+/**
+ * combine_lpi_states - combine local and parent LPI states to form a
+ *			composite LPI state
+ * @l_lpi: local LPI state
+ * @p_lpi: parent LPI state
+ * @c_lpi: composite LPI state
+ */
+static void combine_lpi_states(struct acpi_processor_lpi *l_lpi,
+			       struct acpi_processor_lpi *p_lpi,
+			       struct acpi_processor_lpi *c_lpi)
+{
+	c_lpi->min_residency = max(l_lpi->min_residency, p_lpi->min_residency);
+	c_lpi->wake_latency = l_lpi->wake_latency + p_lpi->wake_latency;
+	c_lpi->enable_parent_state = p_lpi->enable_parent_state;
+
+	if (p_lpi->entry_method == ACPI_CSTATE_INTEGER) {
+		c_lpi->entry_method = l_lpi->entry_method;
+		c_lpi->address = l_lpi->address + p_lpi->address;
+	} else {
+		c_lpi->entry_method = l_lpi->entry_method;
+		c_lpi->address = p_lpi->address;
+	}
+
+	c_lpi->index = p_lpi->index;
+	c_lpi->flags = p_lpi->flags;
+	c_lpi->arch_flags = p_lpi->arch_flags;
+
+	strlcpy(c_lpi->desc, l_lpi->desc, ACPI_CX_DESC_LEN);
+	strlcat(c_lpi->desc, "+", ACPI_CX_DESC_LEN);
+	strlcat(c_lpi->desc, p_lpi->desc, ACPI_CX_DESC_LEN);
+}
+
+static int flatten_lpi_states(struct acpi_processor *pr,
+			      struct acpi_processor_lpi_info *info,
+			      struct acpi_processor_lpi *lpi,
+			      uint32_t depth)
+{
+	int j, scount = info[depth].state_count;
+	struct acpi_processor_lpi *t = info[depth].lpix;
+
+	for (j = 0; j < scount; j++, t++) {
+		struct acpi_processor_lpi *flpi;
+		bool valid = false;
+
+		if (!(t->flags & ACPI_LPI_STATE_FLAGS_ENABLED))
+			continue;
+
+		flpi = &pr->power.lpi_states[fl_scnt];
+		if (depth == max_leaf_depth) { /* leaf/processor node */
+			memcpy(flpi, t, sizeof(*t));
+			fl_scnt++;
+			valid = true;
+		} else if (lpi && t->index <= lpi->enable_parent_state) {
+			combine_lpi_states(lpi, t, flpi);
+			fl_scnt++;
+			valid = true;
+		}
+
+		/*
+		 * flatten recursively from leaf until the highest level
+		 * (e.g. system) is reached
+		 */
+		if (valid && depth)
+			flatten_lpi_states(pr, info, flpi, depth - 1);
+	}
+	return 0;
+}
+
+static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
+{
+	int ret, i;
+	struct acpi_processor_lpi_info *info;
+	struct acpi_device *d = NULL;
+	acpi_handle handle = pr->handle, phandle;
+	acpi_status status;
+
+	if (!osc_pc_lpi_support_acked)
+		return -EOPNOTSUPP;
+
+	max_leaf_depth = 0;
+	if (!acpi_has_method(handle, "_LPI"))
+		return -EINVAL;
+	fl_scnt = 0;
+
+	while (ACPI_SUCCESS(status = acpi_get_parent(handle, &phandle))) {
+		if (!acpi_has_method(handle, "_LPI"))
+			continue;
+		acpi_bus_get_device(handle, &d);
+		if (!strcmp(acpi_device_hid(d), ACPI_PROCESSOR_CONTAINER_HID))
+			break;
+		max_leaf_depth++;
+		handle = phandle;
+	}
+
+	info = kcalloc(max_leaf_depth + 1, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	phandle = pr->handle;
+	for (i = max_leaf_depth; i >= 0 && ACPI_SUCCESS(status); i--) {
+		handle = phandle;
+		ret = acpi_processor_evaluate_lpi(handle, info + i);
+		if (ret)
+			break;
+		status = acpi_get_parent(handle, &phandle);
+	}
+
+	/* flatten all the LPI states in the entire hierarchy */
+	flatten_lpi_states(pr, info, NULL, max_leaf_depth);
+
+	pr->power.count = fl_scnt;
+	for (i = 0; i <= max_leaf_depth; i++)
+		kfree(info[i].lpix);
+	kfree(info);
+
+	/* Tell driver that _LPI is supported. */
+	pr->flags.has_lpi = 1;
+	pr->flags.power = 1;
+
+	return 0;
+}
+
+/**
+ * acpi_idle_lpi_enter - enters an ACPI any LPI state
+ * @dev: the target CPU
+ * @drv: cpuidle driver containing cpuidle state info
+ * @index: index of target state
+ *
+ * Return: 0 for success or negative value for error
+ */
+static int acpi_idle_lpi_enter(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv, int index)
 {
+	struct acpi_processor *pr;
+	struct acpi_processor_lpi *lpi;
+
+	pr = __this_cpu_read(processors);
+
+	if (unlikely(!pr))
+		return -EINVAL;
+
+	lpi = &pr->power.lpi_states[index];
+	if (lpi->entry_method == ACPI_CSTATE_FFH)
+		/* Call into architectural FFH based C-state */
+		return acpi_processor_ffh_lpi_enter(lpi, index);
 	return -EINVAL;
 }
 
+static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
+{
+	int i;
+	struct acpi_processor_lpi *lpi;
+	struct cpuidle_state *state;
+	struct cpuidle_driver *drv = &acpi_idle_driver;
+
+	for (i = 0; i < fl_scnt && i < CPUIDLE_STATE_MAX; i++) {
+		lpi = &pr->power.lpi_states[i];
+
+		state = &drv->states[i];
+		snprintf(state->name, CPUIDLE_NAME_LEN, "LPI-%d", i);
+		strlcpy(state->desc, lpi->desc, CPUIDLE_DESC_LEN);
+		state->exit_latency = lpi->wake_latency;
+		state->target_residency = lpi->min_residency;
+		if (lpi->arch_flags)
+			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
+		state->enter = acpi_idle_lpi_enter;
+		drv->safe_state_index = i;
+	}
+
+	drv->state_count = i;
+
+	return 0;
+}
+
+#else
+static int acpi_processor_ffh_lpi_probe(unsigned int cpu) { return -ENODEV; }
+static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
+{
+	return -ENODEV;
+}
+
+static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
+{
+	return -EINVAL;
+}
 #endif
 
+/**
+ * acpi_processor_setup_cpuidle_states- prepares and configures cpuidle
+ * global state data i.e. idle routines
+ *
+ * @pr: the ACPI processor
+ */
+static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
+{
+	int i;
+	struct cpuidle_driver *drv = &acpi_idle_driver;
+
+	if (!pr->flags.power_setup_done)
+		return -EINVAL;
+
+	if (pr->flags.power == 0)
+		return -EINVAL;
+
+	drv->safe_state_index = -1;
+	for (i = CPUIDLE_DRIVER_STATE_START; i < CPUIDLE_STATE_MAX; i++) {
+		drv->states[i].name[0] = '\0';
+		drv->states[i].desc[0] = '\0';
+	}
+
+	if (pr->flags.has_lpi)
+		return acpi_processor_setup_lpi_states(pr);
+	return acpi_processor_setup_cstates(pr);
+}
+
+/**
+ * acpi_processor_setup_cpuidle_dev - prepares and configures CPUIDLE
+ * device i.e. per-cpu data
+ *
+ * @pr: the ACPI processor
+ * @dev : the cpuidle device
+ */
+static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
+					    struct cpuidle_device *dev)
+{
+	if (!pr->flags.power_setup_done)
+		return -EINVAL;
+
+	if (pr->flags.power == 0)
+		return -EINVAL;
+
+	if (!dev)
+		return -EINVAL;
+
+	dev->cpu = pr->id;
+	if (pr->flags.has_lpi)
+		return acpi_processor_ffh_lpi_probe(pr->id);
+	else
+		return acpi_processor_setup_cpuidle_cx(pr, dev);
+}
+
+static int acpi_processor_get_power_info(struct acpi_processor *pr)
+{
+	int ret = 0;
+
+	ret = acpi_processor_get_cstate_info(pr);
+	if (ret)
+		ret = acpi_processor_get_lpi_info(pr);
+	return ret;
+}
+
 int acpi_processor_hotplug(struct acpi_processor *pr)
 {
 	int ret = 0;
@@ -980,7 +1326,7 @@ int acpi_processor_hotplug(struct acpi_processor *pr)
 	cpuidle_disable_device(dev);
 	acpi_processor_get_power_info(pr);
 	if (pr->flags.power) {
-		acpi_processor_setup_cpuidle_cx(pr, dev);
+		acpi_processor_setup_cpuidle_dev(pr, dev);
 		ret = cpuidle_enable_device(dev);
 	}
 	cpuidle_resume_and_unlock();
@@ -988,7 +1334,7 @@ int acpi_processor_hotplug(struct acpi_processor *pr)
 	return ret;
 }
 
-int acpi_processor_cst_has_changed(struct acpi_processor *pr)
+int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
 {
 	int cpu;
 	struct acpi_processor *_pr;
@@ -1036,7 +1382,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 			acpi_processor_get_power_info(_pr);
 			if (_pr->flags.power) {
 				dev = per_cpu(acpi_cpuidle_device, cpu);
-				acpi_processor_setup_cpuidle_cx(_pr, dev);
+				acpi_processor_setup_cpuidle_dev(_pr, dev);
 				cpuidle_enable_device(dev);
 			}
 		}
@@ -1093,7 +1439,7 @@ int acpi_processor_power_init(struct acpi_processor *pr)
 			return -ENOMEM;
 		per_cpu(acpi_cpuidle_device, pr->id) = dev;
 
-		acpi_processor_setup_cpuidle_cx(pr, dev);
+		acpi_processor_setup_cpuidle_dev(pr, dev);
 
 		/* Register per-cpu cpuidle_device. Cpuidle driver
 		 * must already be registered before registering device
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 50f2423d31fa..f3006831d427 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -39,6 +39,7 @@
 #define ACPI_CSTATE_SYSTEMIO	0
 #define ACPI_CSTATE_FFH		1
 #define ACPI_CSTATE_HALT	2
+#define ACPI_CSTATE_INTEGER	3
 
 #define ACPI_CX_DESC_LEN	32
 
@@ -67,9 +68,25 @@ struct acpi_processor_cx {
 	char desc[ACPI_CX_DESC_LEN];
 };
 
+struct acpi_processor_lpi {
+	u32 min_residency;
+	u32 wake_latency; /* worst case */
+	u32 flags;
+	u32 arch_flags;
+	u32 res_cnt_freq;
+	u32 enable_parent_state;
+	u64 address;
+	u8 index;
+	u8 entry_method;
+	char desc[ACPI_CX_DESC_LEN];
+};
+
 struct acpi_processor_power {
 	int count;
-	struct acpi_processor_cx states[ACPI_PROCESSOR_MAX_POWER];
+	union {
+		struct acpi_processor_cx states[ACPI_PROCESSOR_MAX_POWER];
+		struct acpi_processor_lpi lpi_states[ACPI_PROCESSOR_MAX_POWER];
+	};
 	int timer_broadcast_on_state;
 };
 
@@ -189,6 +206,7 @@ struct acpi_processor_flags {
 	u8 bm_control:1;
 	u8 bm_check:1;
 	u8 has_cst:1;
+	u8 has_lpi:1;
 	u8 power_setup_done:1;
 	u8 bm_rld_set:1;
 	u8 need_hotplug_init:1;
@@ -272,6 +290,11 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
 }
 #endif
 
+#ifdef CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_LPI
+int acpi_processor_ffh_lpi_probe(unsigned int cpu);
+int acpi_processor_ffh_lpi_enter(struct acpi_processor_lpi *lpi, int idx);
+#endif
+
 /* in processor_perflib.c */
 
 #ifdef CONFIG_CPU_FREQ
@@ -372,7 +395,7 @@ extern struct cpuidle_driver acpi_idle_driver;
 #ifdef CONFIG_ACPI_PROCESSOR_IDLE
 int acpi_processor_power_init(struct acpi_processor *pr);
 int acpi_processor_power_exit(struct acpi_processor *pr);
-int acpi_processor_cst_has_changed(struct acpi_processor *pr);
+int acpi_processor_power_state_has_changed(struct acpi_processor *pr);
 int acpi_processor_hotplug(struct acpi_processor *pr);
 #else
 static inline int acpi_processor_power_init(struct acpi_processor *pr)
@@ -385,7 +408,8 @@ static inline int acpi_processor_power_exit(struct acpi_processor *pr)
 	return -ENODEV;
 }
 
-static inline int acpi_processor_cst_has_changed(struct acpi_processor *pr)
+static inline int
+acpi_processor_power_state_has_changed(struct acpi_processor *pr)
 {
 	return -ENODEV;
 }
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 054833939995..962a63b266cc 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -380,8 +380,12 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
 #define OSC_SB_HOTPLUG_OST_SUPPORT		0x00000008
 #define OSC_SB_APEI_SUPPORT			0x00000010
 #define OSC_SB_CPC_SUPPORT			0x00000020
+#define OSC_SB_CPCV2_SUPPORT			0x00000040
+#define OSC_SB_PCLPI_SUPPORT			0x00000080
+#define OSC_SB_OSLPI_SUPPORT			0x00000100
 
 extern bool osc_sb_apei_support_acked;
+extern bool osc_pc_lpi_support_acked;
 
 /* PCI Host Bridge _OSC: Capabilities DWORD 2: Support Field */
 #define OSC_PCI_EXT_CONFIG_SUPPORT		0x00000001
-- 
1.9.1

Re: [PATCH v3 5/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states

[Rafael J. Wysocki]

On Wednesday, December 02, 2015 02:10:46 PM Sudeep Holla wrote:
> ACPI 6.0 introduced an optional object _LPI that provides an alternate
> method to describe Low Power Idle states. It defines the local power
> states for each node in a hierarchical processor topology. The OSPM can
> use _LPI object to select a local power state for each level of processor
> hierarchy in the system. They used to produce a composite power state
> request that is presented to the platform by the OSPM.
> 
> Since multiple processors affect the idle state for any non-leaf hierarchy
> node, coordination of idle state requests between the processors is
> required. ACPI supports two different coordination schemes: Platform
> coordinated and  OS initiated.
> 
> This patch adds initial support for Platform coordination scheme of LPI.
> 
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

My first point here would be the same as for [4/5]: please don't introduce
new Kconfig options if you don't have to (and you clearly don't have to in this
case, because it all can be made work without new Kconfig options).

> ---
>  drivers/acpi/Kconfig            |   3 +
>  drivers/acpi/bus.c              |   8 +-
>  drivers/acpi/processor_driver.c |   2 +-
>  drivers/acpi/processor_idle.c   | 440 +++++++++++++++++++++++++++++++++++-----
>  include/acpi/processor.h        |  30 ++-
>  include/linux/acpi.h            |   4 +
>  6 files changed, 435 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 12873f0b8141..89a2d9b81feb 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -51,6 +51,9 @@ config ARCH_MIGHT_HAVE_ACPI_PDC
>  config ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE
>  	bool
>  
> +config ARCH_SUPPORTS_ACPI_PROCESSOR_LPI

Not needed.

> +	bool
> +
>  config ACPI_GENERIC_GSI
>  	bool
>  
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index a212cefae524..2e9e2e3fde6a 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -301,6 +301,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
>  EXPORT_SYMBOL(acpi_run_osc);
>  
>  bool osc_sb_apei_support_acked;
> +bool osc_pc_lpi_support_acked;

Maybe call it osc_pc_lpi_support_confirmed.  And add a comment describing what
"PC LPI" means (a pointer to the relevant spec section might be useful too).

>  static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
>  static void acpi_bus_osc_support(void)
>  {
> @@ -321,6 +322,8 @@ static void acpi_bus_osc_support(void)
>  		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PPC_OST_SUPPORT;
>  
>  	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_HOTPLUG_OST_SUPPORT;
> +	if (IS_ENABLED(CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_LPI))
> +		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PCLPI_SUPPORT;
>  
>  	if (!ghes_disable)
>  		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
> @@ -328,9 +331,12 @@ static void acpi_bus_osc_support(void)
>  		return;
>  	if (ACPI_SUCCESS(acpi_run_osc(handle, &context))) {
>  		u32 *capbuf_ret = context.ret.pointer;
> -		if (context.ret.length > OSC_SUPPORT_DWORD)
> +		if (context.ret.length > OSC_SUPPORT_DWORD) {
>  			osc_sb_apei_support_acked =
>  				capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
> +			osc_pc_lpi_support_acked =
> +				capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
> +		}
>  		kfree(context.ret.pointer);
>  	}
>  	/* do we need to check other returned cap? Sounds no */
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 29f787b2493f..bfc59de0ce6b 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -90,7 +90,7 @@ static void acpi_processor_notify(acpi_handle handle, u32 event, void *data)
>  						  pr->performance_platform_limit);
>  		break;
>  	case ACPI_PROCESSOR_NOTIFY_POWER:
> -		acpi_processor_cst_has_changed(pr);
> +		acpi_processor_power_state_has_changed(pr);
>  		acpi_bus_generate_netlink_event(device->pnp.device_class,
>  						  dev_name(&device->dev), event, 0);
>  		break;
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 9ca840c88f48..73192c08bc8c 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -576,7 +576,7 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
>  	return (working);
>  }
>  
> -static int acpi_processor_get_power_info(struct acpi_processor *pr)
> +static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
>  {
>  	unsigned int i;
>  	int result;
> @@ -810,31 +810,12 @@ static void acpi_idle_enter_freeze(struct cpuidle_device *dev,
>  	acpi_idle_do_entry(cx);
>  }
>  
> -/**
> - * acpi_processor_setup_cpuidle_cx - prepares and configures CPUIDLE
> - * device i.e. per-cpu data
> - *
> - * @pr: the ACPI processor
> - * @dev : the cpuidle device
> - */
>  static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>  					   struct cpuidle_device *dev)
>  {
>  	int i, count = CPUIDLE_DRIVER_STATE_START;
>  	struct acpi_processor_cx *cx;
>  
> -	if (!pr->flags.power_setup_done)
> -		return -EINVAL;
> -
> -	if (pr->flags.power == 0) {
> -		return -EINVAL;
> -	}
> -
> -	if (!dev)
> -		return -EINVAL;
> -
> -	dev->cpu = pr->id;
> -
>  	if (max_cstate == 0)
>  		max_cstate = 1;
>  
> @@ -857,31 +838,13 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>  	return 0;
>  }
>  
> -/**
> - * acpi_processor_setup_cpuidle states- prepares and configures cpuidle
> - * global state data i.e. idle routines
> - *
> - * @pr: the ACPI processor
> - */
> -static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
> +static int acpi_processor_setup_cstates(struct acpi_processor *pr)
>  {
>  	int i, count = CPUIDLE_DRIVER_STATE_START;
>  	struct acpi_processor_cx *cx;
>  	struct cpuidle_state *state;
>  	struct cpuidle_driver *drv = &acpi_idle_driver;
>  
> -	if (!pr->flags.power_setup_done)
> -		return -EINVAL;
> -
> -	if (pr->flags.power == 0)
> -		return -EINVAL;
> -
> -	drv->safe_state_index = -1;
> -	for (i = CPUIDLE_DRIVER_STATE_START; i < CPUIDLE_STATE_MAX; i++) {
> -		drv->states[i].name[0] = '\0';
> -		drv->states[i].desc[0] = '\0';
> -	}
> -
>  	if (max_cstate == 0)
>  		max_cstate = 1;
>  
> @@ -893,7 +856,7 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
>  
>  		state = &drv->states[count];
>  		snprintf(state->name, CPUIDLE_NAME_LEN, "C%d", i);
> -		strncpy(state->desc, cx->desc, CPUIDLE_DESC_LEN);
> +		strlcpy(state->desc, cx->desc, CPUIDLE_DESC_LEN);
>  		state->exit_latency = cx->latency;
>  		state->target_residency = cx->latency * latency_factor;
>  		state->enter = acpi_idle_enter;
> @@ -943,24 +906,407 @@ static inline void acpi_processor_cstate_first_run_checks(void)
>  
>  static inline int disabled_by_idle_boot_param(void) { return 0; }
>  static inline void acpi_processor_cstate_first_run_checks(void) { }
> -static int acpi_processor_get_power_info(struct acpi_processor *pr)
> +static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
>  {
>  	return -ENODEV;
>  }
> -

Unrelated whitespace change.

>  static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>  					   struct cpuidle_device *dev)
>  {
>  	return -EINVAL;
>  }
>  
> -static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
> +static int acpi_processor_setup_cstates(struct acpi_processor *pr)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
> +#ifdef CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_LPI
> +
> +#define ACPI_LPI_STATE_FLAGS_ENABLED			BIT(0)
> +
> +struct acpi_processor_lpi_info {
> +	int state_count;
> +	struct acpi_processor_lpi *lpix;
> +};
> +
> +static int acpi_processor_evaluate_lpi(acpi_handle handle,
> +				       struct acpi_processor_lpi_info *info)
> +{
> +	acpi_status status = 0;
> +	int ret;
> +	int version, level, pkg_count, state_idx = 1, loop;
> +	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +	union acpi_object *lpi;
> +	struct acpi_processor_lpi *lpix;
> +
> +	status = acpi_evaluate_object(handle, "_LPI", NULL, &buffer);
> +	if (ACPI_FAILURE(status)) {
> +		ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No _LPI, giving up\n"));
> +		return -ENODEV;
> +	}
> +
> +	lpi = buffer.pointer;
> +
> +	/* There must be at least 4 elements = 3 elements + 1 package */
> +	if (!lpi || lpi->type != ACPI_TYPE_PACKAGE || lpi->package.count < 4) {
> +		pr_info("not enough elements in _LPI\n");

pr_debug()?  Plus maybe point to the LPI object in question in that message?

> +		ret = -EFAULT;

-ENXIO?  -EFAULT has a specific meaning which doesn't apply here.

> +		goto end;
> +	}
> +
> +	version = lpi->package.elements[0].integer.value;
> +	level = lpi->package.elements[1].integer.value;
> +	pkg_count = lpi->package.elements[2].integer.value;
> +
> +	/* Validate number of power states. */
> +	if (pkg_count < 1 || pkg_count != lpi->package.count - 3) {
> +		pr_err("count given by _LPI is not valid\n");
> +		ret = -EFAULT;

Same comment.

> +		goto end;
> +	}
> +
> +	lpix = kcalloc(pkg_count, sizeof(*lpix), GFP_KERNEL);
> +	if (!lpix) {
> +		ret = -ENOMEM;
> +		goto end;
> +	}
> +
> +	info->state_count = pkg_count;
> +	info->lpix = lpix;
> +	/* LPI States start at index 3 */
> +	for (loop = 3; state_idx <= pkg_count; loop++, state_idx++, lpix++) {
> +		union acpi_object *element, *obj;
> +
> +		element = &lpi->package.elements[loop];
> +		if (element->type != ACPI_TYPE_PACKAGE)
> +			continue;
> +
> +		if (element->package.count < 7)
> +			continue;
> +
> +		/* TODO this long list is looking insane now
> +		 * need a cleaner and saner way to read the elements
> +		 */

Well, I'm not sure about the above comment.  The code is what it is, anyone
can draw their own conclusions. :-)

I would consider storing the whole buffer instead of trying to decode it
upfront, though.  You need to flatten it etc going forward, so maybe
decode it at that time?

Also I'm not sure about silent discarding things that look defective.  I would
at least add a debug message to wherever this is done and maybe we can try
to fix up or fall back to some sane defaults at least in some cases?

> +		obj = &element->package.elements[6];
> +		if (obj->type == ACPI_TYPE_BUFFER) {
> +			struct acpi_power_register *reg;
> +
> +			reg = (struct acpi_power_register *)obj->buffer.pointer;
> +			if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO &&
> +			    (reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE))
> +				continue;
> +			lpix->address = reg->address;
> +			if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE)
> +				lpix->entry_method = ACPI_CSTATE_FFH;
> +			else
> +				lpix->entry_method = ACPI_CSTATE_SYSTEMIO;
> +		} else if (obj->type == ACPI_TYPE_INTEGER) {
> +			lpix->entry_method = ACPI_CSTATE_INTEGER;
> +			lpix->address = obj->integer.value;
> +		} else {
> +			continue;
> +		}
> +
> +		/* elements[7,8] skipped for now i.e. Residency/Usage counter*/
> +
> +		obj = &element->package.elements[9];
> +		if (obj->type == ACPI_TYPE_STRING)
> +			strlcpy(lpix->desc, obj->string.pointer,
> +				ACPI_CX_DESC_LEN);
> +
> +		lpix->index = state_idx;
> +
> +		obj = &element->package.elements[0];
> +		if (obj->type != ACPI_TYPE_INTEGER)
> +			continue;
> +		lpix->min_residency = obj->integer.value;
> +
> +		obj = &element->package.elements[1];
> +		if (obj->type != ACPI_TYPE_INTEGER)
> +			continue;
> +		lpix->wake_latency = obj->integer.value;
> +
> +		obj = &element->package.elements[2];
> +		if (obj->type != ACPI_TYPE_INTEGER)
> +			continue;
> +		lpix->flags = obj->integer.value;
> +
> +		obj = &element->package.elements[3];
> +		if (obj->type != ACPI_TYPE_INTEGER)
> +			continue;
> +		lpix->arch_flags = obj->integer.value;
> +
> +		obj = &element->package.elements[4];
> +		if (obj->type != ACPI_TYPE_INTEGER)
> +			continue;
> +		lpix->res_cnt_freq = obj->integer.value;
> +
> +		obj = &element->package.elements[5];
> +		if (obj->type != ACPI_TYPE_INTEGER)
> +			continue;
> +		lpix->enable_parent_state = obj->integer.value;
> +	}
> +	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d power states\n",
> +			  state_idx));
> +end:
> +	kfree(buffer.pointer);
> +	return status;
> +}
> +
> +static int max_leaf_depth, fl_scnt;
> +/**
> + * combine_lpi_states - combine local and parent LPI states to form a
> + *			composite LPI state
> + * @l_lpi: local LPI state
> + * @p_lpi: parent LPI state
> + * @c_lpi: composite LPI state
> + */
> +static void combine_lpi_states(struct acpi_processor_lpi *l_lpi,
> +			       struct acpi_processor_lpi *p_lpi,
> +			       struct acpi_processor_lpi *c_lpi)
> +{
> +	c_lpi->min_residency = max(l_lpi->min_residency, p_lpi->min_residency);
> +	c_lpi->wake_latency = l_lpi->wake_latency + p_lpi->wake_latency;
> +	c_lpi->enable_parent_state = p_lpi->enable_parent_state;
> +
> +	if (p_lpi->entry_method == ACPI_CSTATE_INTEGER) {
> +		c_lpi->entry_method = l_lpi->entry_method;
> +		c_lpi->address = l_lpi->address + p_lpi->address;
> +	} else {
> +		c_lpi->entry_method = l_lpi->entry_method;
> +		c_lpi->address = p_lpi->address;
> +	}
> +
> +	c_lpi->index = p_lpi->index;
> +	c_lpi->flags = p_lpi->flags;
> +	c_lpi->arch_flags = p_lpi->arch_flags;
> +
> +	strlcpy(c_lpi->desc, l_lpi->desc, ACPI_CX_DESC_LEN);
> +	strlcat(c_lpi->desc, "+", ACPI_CX_DESC_LEN);
> +	strlcat(c_lpi->desc, p_lpi->desc, ACPI_CX_DESC_LEN);
> +}
> +
> +static int flatten_lpi_states(struct acpi_processor *pr,
> +			      struct acpi_processor_lpi_info *info,
> +			      struct acpi_processor_lpi *lpi,
> +			      uint32_t depth)
> +{
> +	int j, scount = info[depth].state_count;
> +	struct acpi_processor_lpi *t = info[depth].lpix;
> +
> +	for (j = 0; j < scount; j++, t++) {
> +		struct acpi_processor_lpi *flpi;
> +		bool valid = false;
> +
> +		if (!(t->flags & ACPI_LPI_STATE_FLAGS_ENABLED))
> +			continue;
> +
> +		flpi = &pr->power.lpi_states[fl_scnt];
> +		if (depth == max_leaf_depth) { /* leaf/processor node */
> +			memcpy(flpi, t, sizeof(*t));
> +			fl_scnt++;
> +			valid = true;
> +		} else if (lpi && t->index <= lpi->enable_parent_state) {
> +			combine_lpi_states(lpi, t, flpi);
> +			fl_scnt++;
> +			valid = true;
> +		}
> +
> +		/*
> +		 * flatten recursively from leaf until the highest level
> +		 * (e.g. system) is reached
> +		 */
> +		if (valid && depth)
> +			flatten_lpi_states(pr, info, flpi, depth - 1);
> +	}
> +	return 0;
> +}
> +
> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
> +{
> +	int ret, i;
> +	struct acpi_processor_lpi_info *info;
> +	struct acpi_device *d = NULL;
> +	acpi_handle handle = pr->handle, phandle;
> +	acpi_status status;
> +
> +	if (!osc_pc_lpi_support_acked)
> +		return -EOPNOTSUPP;
> +
> +	max_leaf_depth = 0;
> +	if (!acpi_has_method(handle, "_LPI"))
> +		return -EINVAL;
> +	fl_scnt = 0;
> +
> +	while (ACPI_SUCCESS(status = acpi_get_parent(handle, &phandle))) {
> +		if (!acpi_has_method(handle, "_LPI"))
> +			continue;
> +		acpi_bus_get_device(handle, &d);
> +		if (!strcmp(acpi_device_hid(d), ACPI_PROCESSOR_CONTAINER_HID))
> +			break;
> +		max_leaf_depth++;
> +		handle = phandle;
> +	}
> +
> +	info = kcalloc(max_leaf_depth + 1, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	phandle = pr->handle;

phandle is easily confised with DT phandles.  I'd avoind using that for an ACPI
object handle variable name.

> +	for (i = max_leaf_depth; i >= 0 && ACPI_SUCCESS(status); i--) {
> +		handle = phandle;
> +		ret = acpi_processor_evaluate_lpi(handle, info + i);
> +		if (ret)
> +			break;
> +		status = acpi_get_parent(handle, &phandle);
> +	}
> +
> +	/* flatten all the LPI states in the entire hierarchy */
> +	flatten_lpi_states(pr, info, NULL, max_leaf_depth);
> +
> +	pr->power.count = fl_scnt;
> +	for (i = 0; i <= max_leaf_depth; i++)
> +		kfree(info[i].lpix);
> +	kfree(info);
> +
> +	/* Tell driver that _LPI is supported. */
> +	pr->flags.has_lpi = 1;
> +	pr->flags.power = 1;
> +
> +	return 0;
> +}
> +
> +/**
> + * acpi_idle_lpi_enter - enters an ACPI any LPI state
> + * @dev: the target CPU
> + * @drv: cpuidle driver containing cpuidle state info
> + * @index: index of target state
> + *
> + * Return: 0 for success or negative value for error
> + */
> +static int acpi_idle_lpi_enter(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv, int index)
>  {
> +	struct acpi_processor *pr;
> +	struct acpi_processor_lpi *lpi;
> +
> +	pr = __this_cpu_read(processors);
> +
> +	if (unlikely(!pr))
> +		return -EINVAL;
> +
> +	lpi = &pr->power.lpi_states[index];
> +	if (lpi->entry_method == ACPI_CSTATE_FFH)
> +		/* Call into architectural FFH based C-state */
> +		return acpi_processor_ffh_lpi_enter(lpi, index);
>  	return -EINVAL;
>  }
>  
> +static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
> +{
> +	int i;
> +	struct acpi_processor_lpi *lpi;
> +	struct cpuidle_state *state;
> +	struct cpuidle_driver *drv = &acpi_idle_driver;
> +
> +	for (i = 0; i < fl_scnt && i < CPUIDLE_STATE_MAX; i++) {
> +		lpi = &pr->power.lpi_states[i];
> +
> +		state = &drv->states[i];
> +		snprintf(state->name, CPUIDLE_NAME_LEN, "LPI-%d", i);
> +		strlcpy(state->desc, lpi->desc, CPUIDLE_DESC_LEN);
> +		state->exit_latency = lpi->wake_latency;
> +		state->target_residency = lpi->min_residency;
> +		if (lpi->arch_flags)
> +			state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> +		state->enter = acpi_idle_lpi_enter;
> +		drv->safe_state_index = i;
> +	}
> +
> +	drv->state_count = i;
> +
> +	return 0;
> +}
> +
> +#else
> +static int acpi_processor_ffh_lpi_probe(unsigned int cpu) { return -ENODEV; }
> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
> +{
> +	return -ENODEV;
> +}
> +
> +static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
> +{
> +	return -EINVAL;
> +}
>  #endif
>  
> +/**
> + * acpi_processor_setup_cpuidle_states- prepares and configures cpuidle
> + * global state data i.e. idle routines
> + *
> + * @pr: the ACPI processor
> + */
> +static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
> +{
> +	int i;
> +	struct cpuidle_driver *drv = &acpi_idle_driver;
> +
> +	if (!pr->flags.power_setup_done)
> +		return -EINVAL;
> +
> +	if (pr->flags.power == 0)
> +		return -EINVAL;
> +
> +	drv->safe_state_index = -1;
> +	for (i = CPUIDLE_DRIVER_STATE_START; i < CPUIDLE_STATE_MAX; i++) {
> +		drv->states[i].name[0] = '\0';
> +		drv->states[i].desc[0] = '\0';
> +	}
> +
> +	if (pr->flags.has_lpi)
> +		return acpi_processor_setup_lpi_states(pr);
> +	return acpi_processor_setup_cstates(pr);

Well, what if it turns out that LPI is missing?  Shouldn't we fall back to using
_CST then?

Of course, the problem here is that the presence of LPI has to be checked for
all CPUs in the system before deciding to fall back (in which case all of them
should be using _CST if present).

> +}
> +
> +/**
> + * acpi_processor_setup_cpuidle_dev - prepares and configures CPUIDLE
> + * device i.e. per-cpu data
> + *
> + * @pr: the ACPI processor
> + * @dev : the cpuidle device
> + */
> +static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
> +					    struct cpuidle_device *dev)
> +{
> +	if (!pr->flags.power_setup_done)
> +		return -EINVAL;
> +
> +	if (pr->flags.power == 0)
> +		return -EINVAL;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	dev->cpu = pr->id;
> +	if (pr->flags.has_lpi)
> +		return acpi_processor_ffh_lpi_probe(pr->id);
> +	else
> +		return acpi_processor_setup_cpuidle_cx(pr, dev);

Fallback here too maybe?

> +}
> +
> +static int acpi_processor_get_power_info(struct acpi_processor *pr)
> +{
> +	int ret = 0;
> +
> +	ret = acpi_processor_get_cstate_info(pr);
> +	if (ret)
> +		ret = acpi_processor_get_lpi_info(pr);

Shouldn't that be done in the reverse order?

> +	return ret;
> +}
> +
>  int acpi_processor_hotplug(struct acpi_processor *pr)
>  {
>  	int ret = 0;
> @@ -980,7 +1326,7 @@ int acpi_processor_hotplug(struct acpi_processor *pr)
>  	cpuidle_disable_device(dev);
>  	acpi_processor_get_power_info(pr);
>  	if (pr->flags.power) {
> -		acpi_processor_setup_cpuidle_cx(pr, dev);
> +		acpi_processor_setup_cpuidle_dev(pr, dev);
>  		ret = cpuidle_enable_device(dev);
>  	}
>  	cpuidle_resume_and_unlock();
> @@ -988,7 +1334,7 @@ int acpi_processor_hotplug(struct acpi_processor *pr)
>  	return ret;
>  }
>  
> -int acpi_processor_cst_has_changed(struct acpi_processor *pr)
> +int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>  {
>  	int cpu;
>  	struct acpi_processor *_pr;
> @@ -1036,7 +1382,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
>  			acpi_processor_get_power_info(_pr);
>  			if (_pr->flags.power) {
>  				dev = per_cpu(acpi_cpuidle_device, cpu);
> -				acpi_processor_setup_cpuidle_cx(_pr, dev);
> +				acpi_processor_setup_cpuidle_dev(_pr, dev);
>  				cpuidle_enable_device(dev);
>  			}
>  		}
> @@ -1093,7 +1439,7 @@ int acpi_processor_power_init(struct acpi_processor *pr)
>  			return -ENOMEM;
>  		per_cpu(acpi_cpuidle_device, pr->id) = dev;
>  
> -		acpi_processor_setup_cpuidle_cx(pr, dev);
> +		acpi_processor_setup_cpuidle_dev(pr, dev);
>  
>  		/* Register per-cpu cpuidle_device. Cpuidle driver
>  		 * must already be registered before registering device
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 50f2423d31fa..f3006831d427 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -39,6 +39,7 @@
>  #define ACPI_CSTATE_SYSTEMIO	0
>  #define ACPI_CSTATE_FFH		1
>  #define ACPI_CSTATE_HALT	2
> +#define ACPI_CSTATE_INTEGER	3

What does this mean?

>  
>  #define ACPI_CX_DESC_LEN	32
>  
> @@ -67,9 +68,25 @@ struct acpi_processor_cx {
>  	char desc[ACPI_CX_DESC_LEN];
>  };
>  
> +struct acpi_processor_lpi {
> +	u32 min_residency;
> +	u32 wake_latency; /* worst case */
> +	u32 flags;
> +	u32 arch_flags;
> +	u32 res_cnt_freq;
> +	u32 enable_parent_state;
> +	u64 address;
> +	u8 index;
> +	u8 entry_method;
> +	char desc[ACPI_CX_DESC_LEN];
> +};
> +
>  struct acpi_processor_power {
>  	int count;
> -	struct acpi_processor_cx states[ACPI_PROCESSOR_MAX_POWER];
> +	union {
> +		struct acpi_processor_cx states[ACPI_PROCESSOR_MAX_POWER];
> +		struct acpi_processor_lpi lpi_states[ACPI_PROCESSOR_MAX_POWER];
> +	};
>  	int timer_broadcast_on_state;
>  };
>  
> @@ -189,6 +206,7 @@ struct acpi_processor_flags {
>  	u8 bm_control:1;
>  	u8 bm_check:1;
>  	u8 has_cst:1;
> +	u8 has_lpi:1;
>  	u8 power_setup_done:1;
>  	u8 bm_rld_set:1;
>  	u8 need_hotplug_init:1;
> @@ -272,6 +290,11 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
>  }
>  #endif
>  
> +#ifdef CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_LPI
> +int acpi_processor_ffh_lpi_probe(unsigned int cpu);
> +int acpi_processor_ffh_lpi_enter(struct acpi_processor_lpi *lpi, int idx);
> +#endif
> +
>  /* in processor_perflib.c */
>  
>  #ifdef CONFIG_CPU_FREQ
> @@ -372,7 +395,7 @@ extern struct cpuidle_driver acpi_idle_driver;
>  #ifdef CONFIG_ACPI_PROCESSOR_IDLE
>  int acpi_processor_power_init(struct acpi_processor *pr);
>  int acpi_processor_power_exit(struct acpi_processor *pr);
> -int acpi_processor_cst_has_changed(struct acpi_processor *pr);
> +int acpi_processor_power_state_has_changed(struct acpi_processor *pr);
>  int acpi_processor_hotplug(struct acpi_processor *pr);
>  #else
>  static inline int acpi_processor_power_init(struct acpi_processor *pr)
> @@ -385,7 +408,8 @@ static inline int acpi_processor_power_exit(struct acpi_processor *pr)
>  	return -ENODEV;
>  }
>  
> -static inline int acpi_processor_cst_has_changed(struct acpi_processor *pr)
> +static inline int
> +acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>  {
>  	return -ENODEV;
>  }
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 054833939995..962a63b266cc 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -380,8 +380,12 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
>  #define OSC_SB_HOTPLUG_OST_SUPPORT		0x00000008
>  #define OSC_SB_APEI_SUPPORT			0x00000010
>  #define OSC_SB_CPC_SUPPORT			0x00000020
> +#define OSC_SB_CPCV2_SUPPORT			0x00000040
> +#define OSC_SB_PCLPI_SUPPORT			0x00000080
> +#define OSC_SB_OSLPI_SUPPORT			0x00000100
>  
>  extern bool osc_sb_apei_support_acked;
> +extern bool osc_pc_lpi_support_acked;
>  
>  /* PCI Host Bridge _OSC: Capabilities DWORD 2: Support Field */
>  #define OSC_PCI_EXT_CONFIG_SUPPORT		0x00000001
> 

Thanks,
Rafael

Re: [PATCH v3 5/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states

[Sudeep Holla]

On 16/02/16 20:46, Rafael J. Wysocki wrote:
> On Wednesday, December 02, 2015 02:10:46 PM Sudeep Holla wrote:
>> ACPI 6.0 introduced an optional object _LPI that provides an alternate
>> method to describe Low Power Idle states. It defines the local power
>> states for each node in a hierarchical processor topology. The OSPM can
>> use _LPI object to select a local power state for each level of processor
>> hierarchy in the system. They used to produce a composite power state
>> request that is presented to the platform by the OSPM.
>>
>> Since multiple processors affect the idle state for any non-leaf hierarchy
>> node, coordination of idle state requests between the processors is
>> required. ACPI supports two different coordination schemes: Platform
>> coordinated and  OS initiated.
>>
>> This patch adds initial support for Platform coordination scheme of LPI.
>>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>
> My first point here would be the same as for [4/5]: please don't introduce
> new Kconfig options if you don't have to (and you clearly don't have to in this
> case, because it all can be made work without new Kconfig options).
>

Right, this was kind of defensive approach initially so as to not break
anything on x86, rather add anything new to x86 code path. I have
removed it now.

>> ---
>>   drivers/acpi/Kconfig            |   3 +
>>   drivers/acpi/bus.c              |   8 +-
>>   drivers/acpi/processor_driver.c |   2 +-
>>   drivers/acpi/processor_idle.c   | 440 +++++++++++++++++++++++++++++++++++-----
>>   include/acpi/processor.h        |  30 ++-
>>   include/linux/acpi.h            |   4 +
>>   6 files changed, 435 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index 12873f0b8141..89a2d9b81feb 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -51,6 +51,9 @@ config ARCH_MIGHT_HAVE_ACPI_PDC
>>   config ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE
>>   	bool
>>
>> +config ARCH_SUPPORTS_ACPI_PROCESSOR_LPI
>
> Not needed.
>

Done

>> +	bool
>> +
>>   config ACPI_GENERIC_GSI
>>   	bool
>>
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index a212cefae524..2e9e2e3fde6a 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -301,6 +301,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
>>   EXPORT_SYMBOL(acpi_run_osc);
>>
>>   bool osc_sb_apei_support_acked;
>> +bool osc_pc_lpi_support_acked;
>
> Maybe call it osc_pc_lpi_support_confirmed.  And add a comment describing what
> "PC LPI" means (a pointer to the relevant spec section might be useful too).
>

Agreed and done

[...]

>> @@ -943,24 +906,407 @@ static inline void acpi_processor_cstate_first_run_checks(void)
>>
>>   static inline int disabled_by_idle_boot_param(void) { return 0; }
>>   static inline void acpi_processor_cstate_first_run_checks(void) { }
>> -static int acpi_processor_get_power_info(struct acpi_processor *pr)
>> +static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
>>   {
>>   	return -ENODEV;
>>   }
>> -
>
> Unrelated whitespace change.
>

Removed

[...]

>> +
>> +	/* There must be at least 4 elements = 3 elements + 1 package */
>> +	if (!lpi || lpi->type != ACPI_TYPE_PACKAGE || lpi->package.count < 4) {
>> +		pr_info("not enough elements in _LPI\n");
>
> pr_debug()?  Plus maybe point to the LPI object in question in that message?
>
>> +		ret = -EFAULT;
>
> -ENXIO?  -EFAULT has a specific meaning which doesn't apply here.
>

Both done

>> +
>> +		/* TODO this long list is looking insane now
>> +		 * need a cleaner and saner way to read the elements
>> +		 */
>
> Well, I'm not sure about the above comment.  The code is what it is, anyone
> can draw their own conclusions. :-)
>

Well that's stray leftover comment. When I started adding LPI changes, I
initially thought they may be better way to do that, but I have now
realized it's only way :). I will remove it


> I would consider storing the whole buffer instead of trying to decode it
> upfront, though.  You need to flatten it etc going forward, so maybe
> decode it at that time?
>

OK, I will look at this now.

> Also I'm not sure about silent discarding things that look defective.  I would
> at least add a debug message to wherever this is done and maybe we can try
> to fix up or fall back to some sane defaults at least in some cases?
>

Agreed.



[...]

>> +	info = kcalloc(max_leaf_depth + 1, sizeof(*info), GFP_KERNEL);
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	phandle = pr->handle;
>
> phandle is easily confised with DT phandles.  I'd avoind using that for an ACPI
> object handle variable name.
>

I changed it to pr_ahandle now(i.e processor acpi handle)


> Well, what if it turns out that LPI is missing?  Shouldn't we fall back to using
> _CST then?
>
> Of course, the problem here is that the presence of LPI has to be checked for
> all CPUs in the system before deciding to fall back (in which case all of them
> should be using _CST if present).
>

I agree, do we do similar check for _CST too ?


>> +	dev->cpu = pr->id;
>> +	if (pr->flags.has_lpi)
>> +		return acpi_processor_ffh_lpi_probe(pr->id);
>> +	else
>> +		return acpi_processor_setup_cpuidle_cx(pr, dev);
>
> Fallback here too maybe?
>

Fixed

>> +}
>> +
>> +static int acpi_processor_get_power_info(struct acpi_processor *pr)
>> +{
>> +	int ret = 0;
>> +
>> +	ret = acpi_processor_get_cstate_info(pr);
>> +	if (ret)
>> +		ret = acpi_processor_get_lpi_info(pr);
>
> Shouldn't that be done in the reverse order?
>

Yes, again kind of defensive approach I took to avoid any change, but
will change it.

>> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
>> index 50f2423d31fa..f3006831d427 100644
>> --- a/include/acpi/processor.h
>> +++ b/include/acpi/processor.h
>> @@ -39,6 +39,7 @@
>>   #define ACPI_CSTATE_SYSTEMIO	0
>>   #define ACPI_CSTATE_FFH		1
>>   #define ACPI_CSTATE_HALT	2
>> +#define ACPI_CSTATE_INTEGER	3
>
> What does this mean?
>

Ah bad naming, I introduced this to communicate to flattening algo that
it's a simple number that needs to used as is. Based on you comment
above, saving buffer and decoding later might remove the need of it.

-- 
Regards,
Sudeep

Re: [PATCH v3 5/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states

[Vikas Sajjan]

Hi Sudeep,


On Wed, Dec 2, 2015 at 7:40 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:
> ACPI 6.0 introduced an optional object _LPI that provides an alternate
> method to describe Low Power Idle states. It defines the local power
> states for each node in a hierarchical processor topology. The OSPM can
> use _LPI object to select a local power state for each level of processor
> hierarchy in the system. They used to produce a composite power state
> request that is presented to the platform by the OSPM.
>
> Since multiple processors affect the idle state for any non-leaf hierarchy
> node, coordination of idle state requests between the processors is
> required. ACPI supports two different coordination schemes: Platform
> coordinated and  OS initiated.
>
> This patch adds initial support for Platform coordination scheme of LPI.
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/acpi/Kconfig            |   3 +
>  drivers/acpi/bus.c              |   8 +-
>  drivers/acpi/processor_driver.c |   2 +-
>  drivers/acpi/processor_idle.c   | 440 +++++++++++++++++++++++++++++++++++-----
>  include/acpi/processor.h        |  30 ++-
>  include/linux/acpi.h            |   4 +
>  6 files changed, 435 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 12873f0b8141..89a2d9b81feb 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -51,6 +51,9 @@ config ARCH_MIGHT_HAVE_ACPI_PDC
>  config ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE
>         bool
>
> +config ARCH_SUPPORTS_ACPI_PROCESSOR_LPI
> +       bool
> +
>  config ACPI_GENERIC_GSI
>         bool
>
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index a212cefae524..2e9e2e3fde6a 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -301,6 +301,7 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
>  EXPORT_SYMBOL(acpi_run_osc);
>
>  bool osc_sb_apei_support_acked;
> +bool osc_pc_lpi_support_acked;
>  static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
>  static void acpi_bus_osc_support(void)
>  {
> @@ -321,6 +322,8 @@ static void acpi_bus_osc_support(void)
>                 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PPC_OST_SUPPORT;
>
>         capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_HOTPLUG_OST_SUPPORT;
> +       if (IS_ENABLED(CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_LPI))
> +               capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PCLPI_SUPPORT;
>
>         if (!ghes_disable)
>                 capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
> @@ -328,9 +331,12 @@ static void acpi_bus_osc_support(void)
>                 return;
>         if (ACPI_SUCCESS(acpi_run_osc(handle, &context))) {
>                 u32 *capbuf_ret = context.ret.pointer;
> -               if (context.ret.length > OSC_SUPPORT_DWORD)
> +               if (context.ret.length > OSC_SUPPORT_DWORD) {
>                         osc_sb_apei_support_acked =
>                                 capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_APEI_SUPPORT;
> +                       osc_pc_lpi_support_acked =
> +                               capbuf_ret[OSC_SUPPORT_DWORD] & OSC_SB_PCLPI_SUPPORT;
> +               }
>                 kfree(context.ret.pointer);
>         }
>         /* do we need to check other returned cap? Sounds no */
> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
> index 29f787b2493f..bfc59de0ce6b 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -90,7 +90,7 @@ static void acpi_processor_notify(acpi_handle handle, u32 event, void *data)
>                                                   pr->performance_platform_limit);
>                 break;
>         case ACPI_PROCESSOR_NOTIFY_POWER:
> -               acpi_processor_cst_has_changed(pr);
> +               acpi_processor_power_state_has_changed(pr);

The function  acpi_processor_power_state_has_changed() has a check as below,

 if (nocst)
           return -ENODEV;

So was wondering if the platform supports only _LPI and  _CST is not
supported, the 'nocst' module param passed will be 1,
and function will return -ENODEV.

Hence, with the introduction of LPI, should we be handling "nocst"
appropriately.
Similar is the case in function  int acpi_processor_hotplug(struct
acpi_processor *pr);

Let me know, if i am missing something here.


>                 acpi_bus_generate_netlink_event(device->pnp.device_class,
>                                                   dev_name(&device->dev), event, 0);
>                 break;
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 9ca840c88f48..73192c08bc8c 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -576,7 +576,7 @@ static int acpi_processor_power_verify(struct acpi_processor *pr)
>         return (working);
>  }
>
> -static int acpi_processor_get_power_info(struct acpi_processor *pr)
> +static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
>  {
>         unsigned int i;
>         int result;
> @@ -810,31 +810,12 @@ static void acpi_idle_enter_freeze(struct cpuidle_device *dev,
>         acpi_idle_do_entry(cx);
>  }
>
> -/**
> - * acpi_processor_setup_cpuidle_cx - prepares and configures CPUIDLE
> - * device i.e. per-cpu data
> - *
> - * @pr: the ACPI processor
> - * @dev : the cpuidle device
> - */
>  static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>                                            struct cpuidle_device *dev)
>  {
>         int i, count = CPUIDLE_DRIVER_STATE_START;
>         struct acpi_processor_cx *cx;
>
> -       if (!pr->flags.power_setup_done)
> -               return -EINVAL;
> -
> -       if (pr->flags.power == 0) {
> -               return -EINVAL;
> -       }
> -
> -       if (!dev)
> -               return -EINVAL;
> -
> -       dev->cpu = pr->id;
> -
>         if (max_cstate == 0)
>                 max_cstate = 1;
>
> @@ -857,31 +838,13 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>         return 0;
>  }
>
> -/**
> - * acpi_processor_setup_cpuidle states- prepares and configures cpuidle
> - * global state data i.e. idle routines
> - *
> - * @pr: the ACPI processor
> - */
> -static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
> +static int acpi_processor_setup_cstates(struct acpi_processor *pr)
>  {
>         int i, count = CPUIDLE_DRIVER_STATE_START;
>         struct acpi_processor_cx *cx;
>         struct cpuidle_state *state;
>         struct cpuidle_driver *drv = &acpi_idle_driver;
>
> -       if (!pr->flags.power_setup_done)
> -               return -EINVAL;
> -
> -       if (pr->flags.power == 0)
> -               return -EINVAL;
> -
> -       drv->safe_state_index = -1;
> -       for (i = CPUIDLE_DRIVER_STATE_START; i < CPUIDLE_STATE_MAX; i++) {
> -               drv->states[i].name[0] = '\0';
> -               drv->states[i].desc[0] = '\0';
> -       }
> -
>         if (max_cstate == 0)
>                 max_cstate = 1;
>
> @@ -893,7 +856,7 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
>
>                 state = &drv->states[count];
>                 snprintf(state->name, CPUIDLE_NAME_LEN, "C%d", i);
> -               strncpy(state->desc, cx->desc, CPUIDLE_DESC_LEN);
> +               strlcpy(state->desc, cx->desc, CPUIDLE_DESC_LEN);
>                 state->exit_latency = cx->latency;
>                 state->target_residency = cx->latency * latency_factor;
>                 state->enter = acpi_idle_enter;
> @@ -943,24 +906,407 @@ static inline void acpi_processor_cstate_first_run_checks(void)
>
>  static inline int disabled_by_idle_boot_param(void) { return 0; }
>  static inline void acpi_processor_cstate_first_run_checks(void) { }
> -static int acpi_processor_get_power_info(struct acpi_processor *pr)
> +static int acpi_processor_get_cstate_info(struct acpi_processor *pr)
>  {
>         return -ENODEV;
>  }
> -
>  static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
>                                            struct cpuidle_device *dev)
>  {
>         return -EINVAL;
>  }
>
> -static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
> +static int acpi_processor_setup_cstates(struct acpi_processor *pr)
> +{
> +       return -EINVAL;
> +}
> +#endif
> +
> +#ifdef CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_LPI
> +
> +#define ACPI_LPI_STATE_FLAGS_ENABLED                   BIT(0)
> +
> +struct acpi_processor_lpi_info {
> +       int state_count;
> +       struct acpi_processor_lpi *lpix;
> +};
> +
> +static int acpi_processor_evaluate_lpi(acpi_handle handle,
> +                                      struct acpi_processor_lpi_info *info)
> +{
> +       acpi_status status = 0;
> +       int ret;
> +       int version, level, pkg_count, state_idx = 1, loop;
> +       struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> +       union acpi_object *lpi;
> +       struct acpi_processor_lpi *lpix;
> +
> +       status = acpi_evaluate_object(handle, "_LPI", NULL, &buffer);
> +       if (ACPI_FAILURE(status)) {
> +               ACPI_DEBUG_PRINT((ACPI_DB_INFO, "No _LPI, giving up\n"));
> +               return -ENODEV;
> +       }
> +
> +       lpi = buffer.pointer;
> +
> +       /* There must be at least 4 elements = 3 elements + 1 package */
> +       if (!lpi || lpi->type != ACPI_TYPE_PACKAGE || lpi->package.count < 4) {
> +               pr_info("not enough elements in _LPI\n");
> +               ret = -EFAULT;
> +               goto end;
> +       }
> +
> +       version = lpi->package.elements[0].integer.value;
> +       level = lpi->package.elements[1].integer.value;
> +       pkg_count = lpi->package.elements[2].integer.value;
> +
> +       /* Validate number of power states. */
> +       if (pkg_count < 1 || pkg_count != lpi->package.count - 3) {
> +               pr_err("count given by _LPI is not valid\n");
> +               ret = -EFAULT;
> +               goto end;
> +       }
> +
> +       lpix = kcalloc(pkg_count, sizeof(*lpix), GFP_KERNEL);
> +       if (!lpix) {
> +               ret = -ENOMEM;
> +               goto end;
> +       }
> +
> +       info->state_count = pkg_count;
> +       info->lpix = lpix;
> +       /* LPI States start at index 3 */
> +       for (loop = 3; state_idx <= pkg_count; loop++, state_idx++, lpix++) {
> +               union acpi_object *element, *obj;
> +
> +               element = &lpi->package.elements[loop];
> +               if (element->type != ACPI_TYPE_PACKAGE)
> +                       continue;
> +
> +               if (element->package.count < 7)
> +                       continue;
> +
> +               /* TODO this long list is looking insane now
> +                * need a cleaner and saner way to read the elements
> +                */
> +               obj = &element->package.elements[6];
> +               if (obj->type == ACPI_TYPE_BUFFER) {
> +                       struct acpi_power_register *reg;
> +
> +                       reg = (struct acpi_power_register *)obj->buffer.pointer;
> +                       if (reg->space_id != ACPI_ADR_SPACE_SYSTEM_IO &&
> +                           (reg->space_id != ACPI_ADR_SPACE_FIXED_HARDWARE))
> +                               continue;
> +                       lpix->address = reg->address;
> +                       if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE)
> +                               lpix->entry_method = ACPI_CSTATE_FFH;
> +                       else
> +                               lpix->entry_method = ACPI_CSTATE_SYSTEMIO;
> +               } else if (obj->type == ACPI_TYPE_INTEGER) {
> +                       lpix->entry_method = ACPI_CSTATE_INTEGER;
> +                       lpix->address = obj->integer.value;
> +               } else {
> +                       continue;
> +               }
> +
> +               /* elements[7,8] skipped for now i.e. Residency/Usage counter*/
> +
> +               obj = &element->package.elements[9];
> +               if (obj->type == ACPI_TYPE_STRING)
> +                       strlcpy(lpix->desc, obj->string.pointer,
> +                               ACPI_CX_DESC_LEN);
> +
> +               lpix->index = state_idx;
> +
> +               obj = &element->package.elements[0];
> +               if (obj->type != ACPI_TYPE_INTEGER)
> +                       continue;
> +               lpix->min_residency = obj->integer.value;
> +
> +               obj = &element->package.elements[1];
> +               if (obj->type != ACPI_TYPE_INTEGER)
> +                       continue;
> +               lpix->wake_latency = obj->integer.value;
> +
> +               obj = &element->package.elements[2];
> +               if (obj->type != ACPI_TYPE_INTEGER)
> +                       continue;
> +               lpix->flags = obj->integer.value;
> +
> +               obj = &element->package.elements[3];
> +               if (obj->type != ACPI_TYPE_INTEGER)
> +                       continue;
> +               lpix->arch_flags = obj->integer.value;
> +
> +               obj = &element->package.elements[4];
> +               if (obj->type != ACPI_TYPE_INTEGER)
> +                       continue;
> +               lpix->res_cnt_freq = obj->integer.value;
> +
> +               obj = &element->package.elements[5];
> +               if (obj->type != ACPI_TYPE_INTEGER)
> +                       continue;
> +               lpix->enable_parent_state = obj->integer.value;
> +       }
> +       ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d power states\n",
> +                         state_idx));
> +end:
> +       kfree(buffer.pointer);
> +       return status;
> +}
> +
> +static int max_leaf_depth, fl_scnt;
> +/**
> + * combine_lpi_states - combine local and parent LPI states to form a
> + *                     composite LPI state
> + * @l_lpi: local LPI state
> + * @p_lpi: parent LPI state
> + * @c_lpi: composite LPI state
> + */
> +static void combine_lpi_states(struct acpi_processor_lpi *l_lpi,
> +                              struct acpi_processor_lpi *p_lpi,
> +                              struct acpi_processor_lpi *c_lpi)
> +{
> +       c_lpi->min_residency = max(l_lpi->min_residency, p_lpi->min_residency);
> +       c_lpi->wake_latency = l_lpi->wake_latency + p_lpi->wake_latency;
> +       c_lpi->enable_parent_state = p_lpi->enable_parent_state;
> +
> +       if (p_lpi->entry_method == ACPI_CSTATE_INTEGER) {
> +               c_lpi->entry_method = l_lpi->entry_method;
> +               c_lpi->address = l_lpi->address + p_lpi->address;
> +       } else {
> +               c_lpi->entry_method = l_lpi->entry_method;
> +               c_lpi->address = p_lpi->address;
> +       }
> +
> +       c_lpi->index = p_lpi->index;
> +       c_lpi->flags = p_lpi->flags;
> +       c_lpi->arch_flags = p_lpi->arch_flags;
> +
> +       strlcpy(c_lpi->desc, l_lpi->desc, ACPI_CX_DESC_LEN);
> +       strlcat(c_lpi->desc, "+", ACPI_CX_DESC_LEN);
> +       strlcat(c_lpi->desc, p_lpi->desc, ACPI_CX_DESC_LEN);
> +}
> +
> +static int flatten_lpi_states(struct acpi_processor *pr,
> +                             struct acpi_processor_lpi_info *info,
> +                             struct acpi_processor_lpi *lpi,
> +                             uint32_t depth)
> +{
> +       int j, scount = info[depth].state_count;
> +       struct acpi_processor_lpi *t = info[depth].lpix;
> +
> +       for (j = 0; j < scount; j++, t++) {
> +               struct acpi_processor_lpi *flpi;
> +               bool valid = false;
> +
> +               if (!(t->flags & ACPI_LPI_STATE_FLAGS_ENABLED))
> +                       continue;
> +
> +               flpi = &pr->power.lpi_states[fl_scnt];
> +               if (depth == max_leaf_depth) { /* leaf/processor node */
> +                       memcpy(flpi, t, sizeof(*t));
> +                       fl_scnt++;
> +                       valid = true;
> +               } else if (lpi && t->index <= lpi->enable_parent_state) {
> +                       combine_lpi_states(lpi, t, flpi);
> +                       fl_scnt++;
> +                       valid = true;
> +               }
> +
> +               /*
> +                * flatten recursively from leaf until the highest level
> +                * (e.g. system) is reached
> +                */
> +               if (valid && depth)
> +                       flatten_lpi_states(pr, info, flpi, depth - 1);
> +       }
> +       return 0;
> +}
> +
> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
> +{
> +       int ret, i;
> +       struct acpi_processor_lpi_info *info;
> +       struct acpi_device *d = NULL;
> +       acpi_handle handle = pr->handle, phandle;
> +       acpi_status status;
> +
> +       if (!osc_pc_lpi_support_acked)
> +               return -EOPNOTSUPP;
> +
> +       max_leaf_depth = 0;
> +       if (!acpi_has_method(handle, "_LPI"))
> +               return -EINVAL;
> +       fl_scnt = 0;
> +
> +       while (ACPI_SUCCESS(status = acpi_get_parent(handle, &phandle))) {
> +               if (!acpi_has_method(handle, "_LPI"))
> +                       continue;
> +               acpi_bus_get_device(handle, &d);
> +               if (!strcmp(acpi_device_hid(d), ACPI_PROCESSOR_CONTAINER_HID))
> +                       break;
> +               max_leaf_depth++;
> +               handle = phandle;
> +       }
> +
> +       info = kcalloc(max_leaf_depth + 1, sizeof(*info), GFP_KERNEL);
> +       if (!info)
> +               return -ENOMEM;
> +
> +       phandle = pr->handle;
> +       for (i = max_leaf_depth; i >= 0 && ACPI_SUCCESS(status); i--) {
> +               handle = phandle;
> +               ret = acpi_processor_evaluate_lpi(handle, info + i);
> +               if (ret)
> +                       break;
> +               status = acpi_get_parent(handle, &phandle);
> +       }
> +
> +       /* flatten all the LPI states in the entire hierarchy */
> +       flatten_lpi_states(pr, info, NULL, max_leaf_depth);
> +
> +       pr->power.count = fl_scnt;
> +       for (i = 0; i <= max_leaf_depth; i++)
> +               kfree(info[i].lpix);
> +       kfree(info);
> +
> +       /* Tell driver that _LPI is supported. */
> +       pr->flags.has_lpi = 1;
> +       pr->flags.power = 1;
> +
> +       return 0;
> +}
> +
> +/**
> + * acpi_idle_lpi_enter - enters an ACPI any LPI state
> + * @dev: the target CPU
> + * @drv: cpuidle driver containing cpuidle state info
> + * @index: index of target state
> + *
> + * Return: 0 for success or negative value for error
> + */
> +static int acpi_idle_lpi_enter(struct cpuidle_device *dev,
> +                              struct cpuidle_driver *drv, int index)
>  {
> +       struct acpi_processor *pr;
> +       struct acpi_processor_lpi *lpi;
> +
> +       pr = __this_cpu_read(processors);
> +
> +       if (unlikely(!pr))
> +               return -EINVAL;
> +
> +       lpi = &pr->power.lpi_states[index];
> +       if (lpi->entry_method == ACPI_CSTATE_FFH)
> +               /* Call into architectural FFH based C-state */
> +               return acpi_processor_ffh_lpi_enter(lpi, index);
>         return -EINVAL;
>  }
>
> +static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
> +{
> +       int i;
> +       struct acpi_processor_lpi *lpi;
> +       struct cpuidle_state *state;
> +       struct cpuidle_driver *drv = &acpi_idle_driver;
> +
> +       for (i = 0; i < fl_scnt && i < CPUIDLE_STATE_MAX; i++) {
> +               lpi = &pr->power.lpi_states[i];
> +
> +               state = &drv->states[i];
> +               snprintf(state->name, CPUIDLE_NAME_LEN, "LPI-%d", i);
> +               strlcpy(state->desc, lpi->desc, CPUIDLE_DESC_LEN);
> +               state->exit_latency = lpi->wake_latency;
> +               state->target_residency = lpi->min_residency;
> +               if (lpi->arch_flags)
> +                       state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> +               state->enter = acpi_idle_lpi_enter;
> +               drv->safe_state_index = i;
> +       }
> +
> +       drv->state_count = i;
> +
> +       return 0;
> +}
> +
> +#else
> +static int acpi_processor_ffh_lpi_probe(unsigned int cpu) { return -ENODEV; }
> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
> +{
> +       return -ENODEV;
> +}
> +
> +static int acpi_processor_setup_lpi_states(struct acpi_processor *pr)
> +{
> +       return -EINVAL;
> +}
>  #endif
>
> +/**
> + * acpi_processor_setup_cpuidle_states- prepares and configures cpuidle
> + * global state data i.e. idle routines
> + *
> + * @pr: the ACPI processor
> + */
> +static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
> +{
> +       int i;
> +       struct cpuidle_driver *drv = &acpi_idle_driver;
> +
> +       if (!pr->flags.power_setup_done)
> +               return -EINVAL;
> +
> +       if (pr->flags.power == 0)
> +               return -EINVAL;
> +
> +       drv->safe_state_index = -1;
> +       for (i = CPUIDLE_DRIVER_STATE_START; i < CPUIDLE_STATE_MAX; i++) {
> +               drv->states[i].name[0] = '\0';
> +               drv->states[i].desc[0] = '\0';
> +       }
> +
> +       if (pr->flags.has_lpi)
> +               return acpi_processor_setup_lpi_states(pr);
> +       return acpi_processor_setup_cstates(pr);
> +}
> +
> +/**
> + * acpi_processor_setup_cpuidle_dev - prepares and configures CPUIDLE
> + * device i.e. per-cpu data
> + *
> + * @pr: the ACPI processor
> + * @dev : the cpuidle device
> + */
> +static int acpi_processor_setup_cpuidle_dev(struct acpi_processor *pr,
> +                                           struct cpuidle_device *dev)
> +{
> +       if (!pr->flags.power_setup_done)
> +               return -EINVAL;
> +
> +       if (pr->flags.power == 0)
> +               return -EINVAL;
> +
> +       if (!dev)
> +               return -EINVAL;
> +
> +       dev->cpu = pr->id;
> +       if (pr->flags.has_lpi)
> +               return acpi_processor_ffh_lpi_probe(pr->id);
> +       else
> +               return acpi_processor_setup_cpuidle_cx(pr, dev);
> +}
> +
> +static int acpi_processor_get_power_info(struct acpi_processor *pr)
> +{
> +       int ret = 0;
> +
> +       ret = acpi_processor_get_cstate_info(pr);
> +       if (ret)
> +               ret = acpi_processor_get_lpi_info(pr);
> +       return ret;
> +}
> +
>  int acpi_processor_hotplug(struct acpi_processor *pr)
>  {
>         int ret = 0;
> @@ -980,7 +1326,7 @@ int acpi_processor_hotplug(struct acpi_processor *pr)
>         cpuidle_disable_device(dev);
>         acpi_processor_get_power_info(pr);
>         if (pr->flags.power) {
> -               acpi_processor_setup_cpuidle_cx(pr, dev);
> +               acpi_processor_setup_cpuidle_dev(pr, dev);
>                 ret = cpuidle_enable_device(dev);
>         }
>         cpuidle_resume_and_unlock();
> @@ -988,7 +1334,7 @@ int acpi_processor_hotplug(struct acpi_processor *pr)
>         return ret;
>  }
>
> -int acpi_processor_cst_has_changed(struct acpi_processor *pr)
> +int acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>  {
>         int cpu;
>         struct acpi_processor *_pr;
> @@ -1036,7 +1382,7 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
>                         acpi_processor_get_power_info(_pr);
>                         if (_pr->flags.power) {
>                                 dev = per_cpu(acpi_cpuidle_device, cpu);
> -                               acpi_processor_setup_cpuidle_cx(_pr, dev);
> +                               acpi_processor_setup_cpuidle_dev(_pr, dev);
>                                 cpuidle_enable_device(dev);
>                         }
>                 }
> @@ -1093,7 +1439,7 @@ int acpi_processor_power_init(struct acpi_processor *pr)
>                         return -ENOMEM;
>                 per_cpu(acpi_cpuidle_device, pr->id) = dev;
>
> -               acpi_processor_setup_cpuidle_cx(pr, dev);
> +               acpi_processor_setup_cpuidle_dev(pr, dev);
>
>                 /* Register per-cpu cpuidle_device. Cpuidle driver
>                  * must already be registered before registering device
> diff --git a/include/acpi/processor.h b/include/acpi/processor.h
> index 50f2423d31fa..f3006831d427 100644
> --- a/include/acpi/processor.h
> +++ b/include/acpi/processor.h
> @@ -39,6 +39,7 @@
>  #define ACPI_CSTATE_SYSTEMIO   0
>  #define ACPI_CSTATE_FFH                1
>  #define ACPI_CSTATE_HALT       2
> +#define ACPI_CSTATE_INTEGER    3
>
>  #define ACPI_CX_DESC_LEN       32
>
> @@ -67,9 +68,25 @@ struct acpi_processor_cx {
>         char desc[ACPI_CX_DESC_LEN];
>  };
>
> +struct acpi_processor_lpi {
> +       u32 min_residency;
> +       u32 wake_latency; /* worst case */
> +       u32 flags;
> +       u32 arch_flags;
> +       u32 res_cnt_freq;
> +       u32 enable_parent_state;
> +       u64 address;
> +       u8 index;
> +       u8 entry_method;
> +       char desc[ACPI_CX_DESC_LEN];
> +};
> +
>  struct acpi_processor_power {
>         int count;
> -       struct acpi_processor_cx states[ACPI_PROCESSOR_MAX_POWER];
> +       union {
> +               struct acpi_processor_cx states[ACPI_PROCESSOR_MAX_POWER];
> +               struct acpi_processor_lpi lpi_states[ACPI_PROCESSOR_MAX_POWER];
> +       };
>         int timer_broadcast_on_state;
>  };
>
> @@ -189,6 +206,7 @@ struct acpi_processor_flags {
>         u8 bm_control:1;
>         u8 bm_check:1;
>         u8 has_cst:1;
> +       u8 has_lpi:1;
>         u8 power_setup_done:1;
>         u8 bm_rld_set:1;
>         u8 need_hotplug_init:1;
> @@ -272,6 +290,11 @@ static inline void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx
>  }
>  #endif
>
> +#ifdef CONFIG_ARCH_SUPPORTS_ACPI_PROCESSOR_LPI
> +int acpi_processor_ffh_lpi_probe(unsigned int cpu);
> +int acpi_processor_ffh_lpi_enter(struct acpi_processor_lpi *lpi, int idx);
> +#endif
> +
>  /* in processor_perflib.c */
>
>  #ifdef CONFIG_CPU_FREQ
> @@ -372,7 +395,7 @@ extern struct cpuidle_driver acpi_idle_driver;
>  #ifdef CONFIG_ACPI_PROCESSOR_IDLE
>  int acpi_processor_power_init(struct acpi_processor *pr);
>  int acpi_processor_power_exit(struct acpi_processor *pr);
> -int acpi_processor_cst_has_changed(struct acpi_processor *pr);
> +int acpi_processor_power_state_has_changed(struct acpi_processor *pr);
>  int acpi_processor_hotplug(struct acpi_processor *pr);
>  #else
>  static inline int acpi_processor_power_init(struct acpi_processor *pr)
> @@ -385,7 +408,8 @@ static inline int acpi_processor_power_exit(struct acpi_processor *pr)
>         return -ENODEV;
>  }
>
> -static inline int acpi_processor_cst_has_changed(struct acpi_processor *pr)
> +static inline int
> +acpi_processor_power_state_has_changed(struct acpi_processor *pr)
>  {
>         return -ENODEV;
>  }
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 054833939995..962a63b266cc 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -380,8 +380,12 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context);
>  #define OSC_SB_HOTPLUG_OST_SUPPORT             0x00000008
>  #define OSC_SB_APEI_SUPPORT                    0x00000010
>  #define OSC_SB_CPC_SUPPORT                     0x00000020
> +#define OSC_SB_CPCV2_SUPPORT                   0x00000040
> +#define OSC_SB_PCLPI_SUPPORT                   0x00000080
> +#define OSC_SB_OSLPI_SUPPORT                   0x00000100
>
>  extern bool osc_sb_apei_support_acked;
> +extern bool osc_pc_lpi_support_acked;
>
>  /* PCI Host Bridge _OSC: Capabilities DWORD 2: Support Field */
>  #define OSC_PCI_EXT_CONFIG_SUPPORT             0x00000001
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 5/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states

[Sudeep Holla]

On 12/04/16 05:06, Vikas Sajjan wrote:
> Hi Sudeep,
>
>
> On Wed, Dec 2, 2015 at 7:40 PM, Sudeep Holla <sudeep.holla@arm.com> wrote:

[...]

>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>> index 29f787b2493f..bfc59de0ce6b 100644
>> --- a/drivers/acpi/processor_driver.c
>> +++ b/drivers/acpi/processor_driver.c
>> @@ -90,7 +90,7 @@ static void acpi_processor_notify(acpi_handle handle, u32 event, void *data)
>>                                                    pr->performance_platform_limit);
>>                  break;
>>          case ACPI_PROCESSOR_NOTIFY_POWER:
>> -               acpi_processor_cst_has_changed(pr);
>> +               acpi_processor_power_state_has_changed(pr);
>
> The function  acpi_processor_power_state_has_changed() has a check as below,
>
>   if (nocst)
>             return -ENODEV;
>
> So was wondering if the platform supports only _LPI and  _CST is not
> supported, the 'nocst' module param passed will be 1,
> and function will return -ENODEV.
>

You are right, it needs to be handled correctly. Thanks for spotting this.

> Hence, with the introduction of LPI, should we be handling "nocst"
> appropriately.
> Similar is the case in function  int acpi_processor_hotplug(struct
> acpi_processor *pr);
>

Correct.

> Let me know, if i am missing something here.
>

I don't think so. Once again thanks for the review.

-- 
Regards,
Sudeep

Re: [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[Prakash, Prashanth]

Reviewed the patch-set and tested it on an ARM platform over the last couple
of days without any issues, so

Tested-by: Prashanth Prakash <pprakash@codeaurora.org>

Thanks,
Prashanth

On 12/2/2015 7:10 AM, Sudeep Holla wrote:
> ACPI 6.0 introduced LPI(Low Power Idle) states that provides an alternate
> method to describe processor idle states. It extends the specification
> to allow the expression of idle states like C-states selectable by the
> OSPM when a processor goes idle, but may affect more than one processor,
> and may affect other system components.
>
> LPI extensions leverages the processor container device(again introduced
> in ACPI 6.0) allowing to express which parts of the system are affected
> by a given LPI state. It defines the local power states for each node
> in a hierarchical processor topology. The OSPM can use _LPI object to
> select a local power state for each level of processor hierarchy in the
> system. They used to produce a composite power state request that is
> presented to the platform by the OSPM.
>
> Since multiple processors affect the idle state for any non-leaf hierarchy
> node, coordination of idle state requests between the processors is
> required. ACPI supports two different coordination schemes: Platform
> coordinated and  OS initiated.
>
> This series aims at providing basic and initial support for platform
> coordinated LPI states.
>
> v2[2]->v3:
> 	- rebased against v4.4-rc3
> 	- fixed couple of issues reported by Prashanth and review comments
> 	  from Ashwin
>
> v1[1]->v2[2]:
> 	- Fixed support for ACPI0010 processor container
> 	- moved sleep state code out of processor_idle
>
> Note the ARM64 specific changes are not part of this series as it's still
> WIP and there are other consolidation happening in there. For reference
> and testing, I have pushed a branch[3]
>
> Regards,
> Sudeep
>
> [1] https://lkml.org/lkml/2015/8/4/789
> [2] https://lkml.org/lkml/2015/9/16/422
> [3] git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git arm64_lpi_testing
>
>
> Sudeep Holla (5):
>   ACPI / processor : add support for ACPI0010 processor container
>   ACPI / sleep: move acpi_processor_sleep to sleep.c
>   ACPI / processor_idle: replace PREFIX with pr_fmt
>   ACPI / processor_idle : introduce ARCH_SUPPORTS_ACPI_PROCESSOR_CSTATE
>   ACPI / processor_idle: Add support for Low Power Idle(LPI) states
>
>  arch/ia64/Kconfig               |   1 +
>  arch/x86/Kconfig                |   1 +
>  drivers/acpi/Kconfig            |   6 +
>  drivers/acpi/acpi_processor.c   |  17 ++
>  drivers/acpi/bus.c              |   8 +-
>  drivers/acpi/processor_driver.c |   4 +-
>  drivers/acpi/processor_idle.c   | 553 ++++++++++++++++++++++++++++++++--------
>  drivers/acpi/sleep.c            |  35 +++
>  include/acpi/processor.h        |  42 ++-
>  include/linux/acpi.h            |   4 +
>  10 files changed, 545 insertions(+), 126 deletions(-)
>

Re: [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[Sudeep Holla]

Hi Prashanth,

On 09/12/15 22:52, Prakash, Prashanth wrote:
> Reviewed the patch-set and tested it on an ARM platform over the last couple
> of days without any issues, so
>
> Tested-by: Prashanth Prakash <pprakash@codeaurora.org>
>

Thanks for testing.

-- 
Regards,
Sudeep

Re: [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[Prakash, Prashanth]

Hi Sudeep,

Any updates on the status of this patch set?

Thanks,
Prashanth

On 12/10/2015 1:48 AM, Sudeep Holla wrote:
> Hi Prashanth,
>
> On 09/12/15 22:52, Prakash, Prashanth wrote:
>> Reviewed the patch-set and tested it on an ARM platform over the last couple
>> of days without any issues, so
>>
>> Tested-by: Prashanth Prakash <pprakash@codeaurora.org>
>>
>
> Thanks for testing.
>

Re: [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[Sudeep Holla]

Hi Prashanth,

On 15/01/16 19:13, Prakash, Prashanth wrote:
> Hi Sudeep,
>
> Any updates on the status of this patch set?
>

Thanks for the follow up. Sorry but there are no updates as such.
I didn't ping Rafael after last merge window due to Xmas holidays.
I will ping or repost after this merge window(ends this Sunday).

Hi Rafael,

Do you prefer a fresh repost or shall I wait for you comments after
merge window ?

-- 
Regards,
Sudeep

Re: [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[Rafael J. Wysocki]

On Monday, January 18, 2016 12:15:45 PM Sudeep Holla wrote:
> Hi Prashanth,
> 
> On 15/01/16 19:13, Prakash, Prashanth wrote:
> > Hi Sudeep,
> >
> > Any updates on the status of this patch set?
> >
> 
> Thanks for the follow up. Sorry but there are no updates as such.
> I didn't ping Rafael after last merge window due to Xmas holidays.
> I will ping or repost after this merge window(ends this Sunday).
> 
> Hi Rafael,
> 
> Do you prefer a fresh repost or shall I wait for you comments after
> merge window ?

If you don't have any updates to the patches, there's no need to resend them.

Thanks,
Rafael

Re: [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[Sudeep Holla]

Hi Rafael,

On 18/01/16 14:53, Rafael J. Wysocki wrote:
> On Monday, January 18, 2016 12:15:45 PM Sudeep Holla wrote:
>> Hi Prashanth,
>>
>> On 15/01/16 19:13, Prakash, Prashanth wrote:
>>> Hi Sudeep,
>>>
>>> Any updates on the status of this patch set?
>>>
>>
>> Thanks for the follow up. Sorry but there are no updates as such.
>> I didn't ping Rafael after last merge window due to Xmas holidays.
>> I will ping or repost after this merge window(ends this Sunday).
>>
>> Hi Rafael,
>>
>> Do you prefer a fresh repost or shall I wait for you comments after
>> merge window ?
>
> If you don't have any updates to the patches, there's no need to resend them.
>

I don't have any updates and so I will wait for your feedback.

-- 
Regards,
Sudeep

Re: [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[Rafael J. Wysocki]

On Wednesday, December 02, 2015 02:10:41 PM Sudeep Holla wrote:
> ACPI 6.0 introduced LPI(Low Power Idle) states that provides an alternate
> method to describe processor idle states. It extends the specification
> to allow the expression of idle states like C-states selectable by the
> OSPM when a processor goes idle, but may affect more than one processor,
> and may affect other system components.
> 
> LPI extensions leverages the processor container device(again introduced
> in ACPI 6.0) allowing to express which parts of the system are affected
> by a given LPI state. It defines the local power states for each node
> in a hierarchical processor topology. The OSPM can use _LPI object to
> select a local power state for each level of processor hierarchy in the
> system. They used to produce a composite power state request that is
> presented to the platform by the OSPM.
> 
> Since multiple processors affect the idle state for any non-leaf hierarchy
> node, coordination of idle state requests between the processors is
> required. ACPI supports two different coordination schemes: Platform
> coordinated and  OS initiated.
> 
> This series aims at providing basic and initial support for platform
> coordinated LPI states.
> 
> v2[2]->v3:
> 	- rebased against v4.4-rc3
> 	- fixed couple of issues reported by Prashanth and review comments
> 	  from Ashwin
> 
> v1[1]->v2[2]:
> 	- Fixed support for ACPI0010 processor container
> 	- moved sleep state code out of processor_idle
> 
> Note the ARM64 specific changes are not part of this series as it's still
> WIP and there are other consolidation happening in there. For reference
> and testing, I have pushed a branch[3]

Sorry for the slow response here.

It doesn't look too bad overall, but there are some things in it I'd like to
be done differenty.  Please see comments on the individual patches.

Thanks,
Rafael

Re: [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[Sudeep Holla]

On 16/02/16 20:08, Rafael J. Wysocki wrote:
> On Wednesday, December 02, 2015 02:10:41 PM Sudeep Holla wrote:
>> ACPI 6.0 introduced LPI(Low Power Idle) states that provides an alternate
>> method to describe processor idle states. It extends the specification
>> to allow the expression of idle states like C-states selectable by the
>> OSPM when a processor goes idle, but may affect more than one processor,
>> and may affect other system components.
>>
>> LPI extensions leverages the processor container device(again introduced
>> in ACPI 6.0) allowing to express which parts of the system are affected
>> by a given LPI state. It defines the local power states for each node
>> in a hierarchical processor topology. The OSPM can use _LPI object to
>> select a local power state for each level of processor hierarchy in the
>> system. They used to produce a composite power state request that is
>> presented to the platform by the OSPM.
>>
>> Since multiple processors affect the idle state for any non-leaf hierarchy
>> node, coordination of idle state requests between the processors is
>> required. ACPI supports two different coordination schemes: Platform
>> coordinated and  OS initiated.
>>
>> This series aims at providing basic and initial support for platform
>> coordinated LPI states.
>>
>> v2[2]->v3:
>> 	- rebased against v4.4-rc3
>> 	- fixed couple of issues reported by Prashanth and review comments
>> 	  from Ashwin
>>
>> v1[1]->v2[2]:
>> 	- Fixed support for ACPI0010 processor container
>> 	- moved sleep state code out of processor_idle
>>
>> Note the ARM64 specific changes are not part of this series as it's still
>> WIP and there are other consolidation happening in there. For reference
>> and testing, I have pushed a branch[3]
>
> Sorry for the slow response here.
>

No problem, I saw you were quite busy with cpufreq timers past couple of
weeks so didn't bother you.

> It doesn't look too bad overall, but there are some things in it I'd like to
> be done differenty.  Please see comments on the individual patches.
>

OK, thanks for the review, will look at them.

-- 
Regards,
Sudeep

Re: [PATCH v3 0/5] ACPI / processor_idle: Add ACPI v6.0 LPI support

[Rafael J. Wysocki]

On Wednesday, February 17, 2016 11:37:51 AM Sudeep Holla wrote:
> 

[cut]

> > Sorry for the slow response here.
> >
> 
> No problem, I saw you were quite busy with cpufreq timers past couple of
> weeks so didn't bother you.

Well, thanks for that!

In fact I'm not done with cpufreq yet for the time being as it turns out.

Thanks,
Rafael