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

[PATCH v5 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.

v4[4]->v5:
	- Addressed all the comments from Rafael
	- Added support for retention mode(Prashant)
	- Handled acpi_processor_get_power_info return value correctly(Vikas)
	- Dropped __init from arm_cpuidle_init
	- Merged psci prepartory patch into arm64 lpi support

v3[3]->v4:
	- Dropped the preparatory patches that are merged already
	- Added ARM64 arch specific callback implementations
	- Addressed most of the review comments from Rafael

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

Code is also available @[5]

Regards,
Sudeep

[1] http://marc.info/?l=linux-acpi&m=143871041601132&w=2
[2] http://marc.info/?l=linux-acpi&m=144241209800788&w=2
[3] http://marc.info/?l=linux-acpi&m=144906557814813&w=2
[4] http://marc.info/?l=linux-acpi&m=146106902731359&w=2
[5] git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git for_review/arm64_lpi

Sudeep Holla (5):
  ACPI / processor_idle: introduce ACPI_PROCESSOR_CSTATE
  ACPI / processor_idle: Add support for Low Power Idle(LPI) states
  arm64: cpuidle: drop __init section marker to arm_cpuidle_init
  arm64: add support for ACPI Low Power Idle(LPI)
  ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

 arch/arm64/kernel/acpi.c        |  48 ++++
 arch/arm64/kernel/cpuidle.c     |   2 +-
 arch/ia64/Kconfig               |   1 +
 arch/x86/Kconfig                |   1 +
 drivers/acpi/Kconfig            |   6 +-
 drivers/acpi/bus.c              |  14 +-
 drivers/acpi/processor_driver.c |   2 +-
 drivers/acpi/processor_idle.c   | 539 ++++++++++++++++++++++++++++++++++------
 drivers/firmware/psci.c         |  56 +++++
 include/acpi/processor.h        |  26 +-
 include/linux/acpi.h            |   4 +
 11 files changed, 611 insertions(+), 88 deletions(-)

-- 
1.9.1

[PATCH v5 1/5] ACPI / processor_idle: introduce 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 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: x86@kernel.org
Cc: linux-ia64@vger.kernel.org
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          |  4 +++
 drivers/acpi/processor_idle.c | 80 ++++++++++++++++++++++++++++---------------
 include/acpi/processor.h      |  2 +-
 5 files changed, 60 insertions(+), 28 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index b534ebab36ea..e820670d7243 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -15,6 +15,7 @@ config IA64
 	select ARCH_MIGHT_HAVE_PC_SERIO
 	select PCI if (!IA64_HP_SIM)
 	select ACPI if (!IA64_HP_SIM)
+	select ACPI_PROCESSOR_CSTATE if ACPI
 	select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
 	select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI
 	select HAVE_UNSTABLE_SCHED_CLOCK
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2dc18605831f..cb3e14757c9c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -18,6 +18,7 @@ config X86_64
 config X86
 	def_bool y
 	select ACPI_LEGACY_TABLES_LOOKUP	if ACPI
+	select ACPI_PROCESSOR_CSTATE		if ACPI
 	select ACPI_SYSTEM_POWER_STATES_SUPPORT	if ACPI
 	select ANON_INODES
 	select ARCH_CLOCKSOURCE_DATA
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index b7e2e776397d..093bfcc4f9c3 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -213,6 +213,10 @@ config ACPI_CPU_FREQ_PSS
 	bool
 	select THERMAL
 
+config ACPI_PROCESSOR_CSTATE
+	bool
+	depends on IA64 || X86
+
 config ACPI_PROCESSOR_IDLE
 	bool
 	select CPU_IDLE
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 444e3745c8b3..ca0de35d1c3a 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_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,50 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
 	return 0;
 }
 
+static inline void acpi_processor_cstate_first_run_checks(void)
+{
+	acpi_status status;
+	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++;
+
+	if (acpi_gbl_FADT.cst_control && !nocst) {
+		status = acpi_os_write_port(acpi_gbl_FADT.smi_command,
+					    acpi_gbl_FADT.cst_control, 8);
+		if (ACPI_FAILURE(status))
+			ACPI_EXCEPTION((AE_INFO, status,
+					"Notifying BIOS of _CST ability failed"));
+	}
+}
+#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 /* CONFIG_ACPI_PROCESSOR_CSTATE */
+
 int acpi_processor_hotplug(struct acpi_processor *pr)
 {
 	int ret = 0;
@@ -1015,35 +1060,16 @@ static int acpi_processor_registered;
 
 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 =
-		    acpi_os_write_port(acpi_gbl_FADT.smi_command, acpi_gbl_FADT.cst_control, 8);
-		if (ACPI_FAILURE(status)) {
-			ACPI_EXCEPTION((AE_INFO, status,
-					"Notifying BIOS of _CST ability failed"));
-		}
-	}
-
-	acpi_processor_get_power_info(pr);
-	pr->flags.power_setup_done = 1;
+	if (!acpi_processor_get_power_info(pr))
+		pr->flags.power_setup_done = 1;
 
 	/*
 	 * Install the idle handler if processor power management is supported.
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 6f1805dd5d3c..48779d678122 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -242,7 +242,7 @@ 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_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

[PATCH v5 1/5] ACPI / processor_idle: introduce 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 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: x86@kernel.org
Cc: linux-ia64@vger.kernel.org
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          |  4 +++
 drivers/acpi/processor_idle.c | 80 ++++++++++++++++++++++++++++---------------
 include/acpi/processor.h      |  2 +-
 5 files changed, 60 insertions(+), 28 deletions(-)

diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index b534ebab36ea..e820670d7243 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -15,6 +15,7 @@ config IA64
 	select ARCH_MIGHT_HAVE_PC_SERIO
 	select PCI if (!IA64_HP_SIM)
 	select ACPI if (!IA64_HP_SIM)
+	select ACPI_PROCESSOR_CSTATE if ACPI
 	select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
 	select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI
 	select HAVE_UNSTABLE_SCHED_CLOCK
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 2dc18605831f..cb3e14757c9c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -18,6 +18,7 @@ config X86_64
 config X86
 	def_bool y
 	select ACPI_LEGACY_TABLES_LOOKUP	if ACPI
+	select ACPI_PROCESSOR_CSTATE		if ACPI
 	select ACPI_SYSTEM_POWER_STATES_SUPPORT	if ACPI
 	select ANON_INODES
 	select ARCH_CLOCKSOURCE_DATA
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index b7e2e776397d..093bfcc4f9c3 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -213,6 +213,10 @@ config ACPI_CPU_FREQ_PSS
 	bool
 	select THERMAL
 
+config ACPI_PROCESSOR_CSTATE
+	bool
+	depends on IA64 || X86
+
 config ACPI_PROCESSOR_IDLE
 	bool
 	select CPU_IDLE
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 444e3745c8b3..ca0de35d1c3a 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_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,50 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
 	return 0;
 }
 
+static inline void acpi_processor_cstate_first_run_checks(void)
+{
+	acpi_status status;
+	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++;
+
+	if (acpi_gbl_FADT.cst_control && !nocst) {
+		status = acpi_os_write_port(acpi_gbl_FADT.smi_command,
+					    acpi_gbl_FADT.cst_control, 8);
+		if (ACPI_FAILURE(status))
+			ACPI_EXCEPTION((AE_INFO, status,
+					"Notifying BIOS of _CST ability failed"));
+	}
+}
+#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 /* CONFIG_ACPI_PROCESSOR_CSTATE */
+
 int acpi_processor_hotplug(struct acpi_processor *pr)
 {
 	int ret = 0;
@@ -1015,35 +1060,16 @@ static int acpi_processor_registered;
 
 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 -		    acpi_os_write_port(acpi_gbl_FADT.smi_command, acpi_gbl_FADT.cst_control, 8);
-		if (ACPI_FAILURE(status)) {
-			ACPI_EXCEPTION((AE_INFO, status,
-					"Notifying BIOS of _CST ability failed"));
-		}
-	}
-
-	acpi_processor_get_power_info(pr);
-	pr->flags.power_setup_done = 1;
+	if (!acpi_processor_get_power_info(pr))
+		pr->flags.power_setup_done = 1;
 
 	/*
 	 * Install the idle handler if processor power management is supported.
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 6f1805dd5d3c..48779d678122 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -242,7 +242,7 @@ 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_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

[PATCH v5 2/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/bus.c              |  14 +-
 drivers/acpi/processor_driver.c |   2 +-
 drivers/acpi/processor_idle.c   | 463 +++++++++++++++++++++++++++++++++++-----
 include/acpi/processor.h        |  24 ++-
 include/linux/acpi.h            |   4 +
 5 files changed, 447 insertions(+), 60 deletions(-)

diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
index 31e8da648fff..71edf7b07409 100644
--- a/drivers/acpi/bus.c
+++ b/drivers/acpi/bus.c
@@ -302,6 +302,14 @@ acpi_status acpi_run_osc(acpi_handle handle, struct acpi_osc_context *context)
 EXPORT_SYMBOL(acpi_run_osc);
 
 bool osc_sb_apei_support_acked;
+
+/*
+ * ACPI 6.0 Section 8.4.4.2 Idle State Coordination
+ * OSPM supports platform coordinated low power idle(LPI) states
+ */
+bool osc_pc_lpi_support_confirmed;
+EXPORT_SYMBOL_GPL(osc_pc_lpi_support_confirmed);
+
 static u8 sb_uuid_str[] = "0811B06E-4A27-44F9-8D60-3CBBC22E7B48";
 static void acpi_bus_osc_support(void)
 {
@@ -322,6 +330,7 @@ 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;
+	capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_PCLPI_SUPPORT;
 
 	if (!ghes_disable)
 		capbuf[OSC_SUPPORT_DWORD] |= OSC_SB_APEI_SUPPORT;
@@ -329,9 +338,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_confirmed =
+				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 d2fa8cb82d2b..0ca14ac7bb28 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 ca0de35d1c3a..549404a45b45 100644
--- a/drivers/acpi/processor_idle.c
+++ b/drivers/acpi/processor_idle.c
@@ -303,7 +303,6 @@ static int acpi_processor_get_power_info_cst(struct acpi_processor *pr)
 	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
 	union acpi_object *cst;
 
-
 	if (nocst)
 		return -ENODEV;
 
@@ -576,7 +575,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 +809,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 +837,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 +855,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;
@@ -952,7 +914,7 @@ 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;
 }
@@ -963,13 +925,410 @@ static int acpi_processor_setup_cpuidle_cx(struct acpi_processor *pr,
 	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 /* CONFIG_ACPI_PROCESSOR_CSTATE */
 
+struct acpi_lpi_states_array {
+	unsigned int size;
+	struct acpi_lpi_state *entries;
+};
+
+static int obj_get_integer(union acpi_object *obj, u32 *value)
+{
+	if (obj->type != ACPI_TYPE_INTEGER)
+		return -EINVAL;
+
+	*value = obj->integer.value;
+	return 0;
+}
+
+static int acpi_processor_evaluate_lpi(acpi_handle handle,
+				       struct acpi_lpi_states_array *info)
+{
+	acpi_status status;
+	int ret = 0;
+	int pkg_count, state_idx = 1, loop;
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object *lpi_data;
+	struct acpi_lpi_state *lpi_state;
+
+	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_data = buffer.pointer;
+
+	/* There must be at least 4 elements = 3 elements + 1 package */
+	if (!lpi_data || lpi_data->type != ACPI_TYPE_PACKAGE ||
+	    lpi_data->package.count < 4) {
+		pr_debug("not enough elements in _LPI\n");
+		ret = -ENODATA;
+		goto end;
+	}
+
+	pkg_count = lpi_data->package.elements[2].integer.value;
+
+	/* Validate number of power states. */
+	if (pkg_count < 1 || pkg_count != lpi_data->package.count - 3) {
+		pr_debug("count given by _LPI is not valid\n");
+		ret = -ENODATA;
+		goto end;
+	}
+
+	lpi_state = kcalloc(pkg_count, sizeof(*lpi_state), GFP_KERNEL);
+	if (!lpi_state) {
+		ret = -ENOMEM;
+		goto end;
+	}
+
+	info->size = pkg_count;
+	info->entries = lpi_state;
+
+	/* LPI States start at index 3 */
+	for (loop = 3; state_idx <= pkg_count;
+	     loop++, state_idx++, lpi_state++) {
+		union acpi_object *element, *pkg_elem, *obj;
+
+		element = &lpi_data->package.elements[loop];
+		if (element->type != ACPI_TYPE_PACKAGE)
+			continue;
+
+		if (element->package.count < 7)
+			continue;
+
+		pkg_elem = element->package.elements;
+
+		obj = pkg_elem + 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;
+
+			lpi_state->address = reg->address;
+
+			if (reg->space_id == ACPI_ADR_SPACE_FIXED_HARDWARE)
+				lpi_state->entry_method = ACPI_CSTATE_FFH;
+			else
+				lpi_state->entry_method = ACPI_CSTATE_SYSTEMIO;
+		} else if (obj->type == ACPI_TYPE_INTEGER) {
+			lpi_state->entry_method = ACPI_CSTATE_INTEGER;
+			lpi_state->address = obj->integer.value;
+		} else {
+			continue;
+		}
+
+		/* elements[7,8] skipped for now i.e. Residency/Usage counter*/
+
+		obj = pkg_elem + 9;
+		if (obj->type == ACPI_TYPE_STRING)
+			strlcpy(lpi_state->desc, obj->string.pointer,
+				ACPI_CX_DESC_LEN);
+
+		lpi_state->index = state_idx;
+		if (obj_get_integer(pkg_elem + 0, &lpi_state->min_residency)) {
+			pr_debug("No min. residency found, assuming 10 us\n");
+			lpi_state->min_residency = 10;
+		}
+
+		if (obj_get_integer(pkg_elem + 1, &lpi_state->wake_latency)) {
+			pr_debug("No wakeup residency found, assuming 10 us\n");
+			lpi_state->wake_latency = 10;
+		}
+
+		if (obj_get_integer(pkg_elem + 2, &lpi_state->flags))
+			lpi_state->flags = 0;
+
+		if (obj_get_integer(pkg_elem + 3, &lpi_state->arch_flags))
+			lpi_state->arch_flags = 0;
+
+		if (obj_get_integer(pkg_elem + 4, &lpi_state->res_cnt_freq))
+			lpi_state->res_cnt_freq = 1;
+
+		if (obj_get_integer(pkg_elem + 5, &lpi_state->enable_parent_state))
+			lpi_state->enable_parent_state = 0;
+	}
+
+	ACPI_DEBUG_PRINT((ACPI_DB_INFO, "Found %d power states\n",
+			  state_idx));
+end:
+	kfree(buffer.pointer);
+	return ret;
+}
+
+/*
+ * max_leaf_depth - holds the depth of the processor hierarchy/tree
+ * flat_state_cnt - the number of composite LPI states after the process of flattening
+ */
+static int max_leaf_depth, flat_state_cnt;
+
+/**
+ * combine_lpi_states - combine local and parent LPI states to form a composite LPI state
+ *
+ * @local: local LPI state
+ * @parent: parent LPI state
+ * @result: composite LPI state
+ */
+static void combine_lpi_states(struct acpi_lpi_state *local,
+			       struct acpi_lpi_state *parent,
+			       struct acpi_lpi_state *result)
+{
+	result->min_residency = max(local->min_residency, parent->min_residency);
+	result->wake_latency = local->wake_latency + parent->wake_latency;
+	result->enable_parent_state = parent->enable_parent_state;
+	result->entry_method = local->entry_method;
+
+	if (parent->entry_method == ACPI_CSTATE_INTEGER)
+		result->address = local->address + parent->address;
+	else
+		result->address = parent->address;
+
+	result->index = parent->index;
+	result->flags = parent->flags;
+	result->arch_flags = parent->arch_flags;
+
+	strlcpy(result->desc, local->desc, ACPI_CX_DESC_LEN);
+	strlcat(result->desc, "+", ACPI_CX_DESC_LEN);
+	strlcat(result->desc, parent->desc, ACPI_CX_DESC_LEN);
+}
+
+#define ACPI_LPI_STATE_FLAGS_ENABLED			BIT(0)
+
+static int flatten_lpi_states(struct acpi_processor *pr,
+			      struct acpi_lpi_states_array *info,
+			      struct acpi_lpi_state *lpi,
+			      uint32_t depth)
+{
+	int j, state_count = info[depth].size;
+	struct acpi_lpi_state *t = info[depth].entries;
+
+	for (j = 0; j < state_count; j++, t++) {
+		struct acpi_lpi_state *flpi;
+		bool valid = false;
+
+		if (!(t->flags & ACPI_LPI_STATE_FLAGS_ENABLED))
+			continue;
+
+		flpi = &pr->power.lpi_states[flat_state_cnt];
+
+		if (depth == max_leaf_depth) { /* leaf/processor node */
+			memcpy(flpi, t, sizeof(*t));
+			flat_state_cnt++;
+			valid = true;
+		} else if (lpi && t->index <= lpi->enable_parent_state) {
+			combine_lpi_states(lpi, t, flpi);
+			flat_state_cnt++;
+			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_lpi_states_array *info;
+	struct acpi_device *d = NULL;
+	acpi_handle handle = pr->handle, pr_ahandle;
+	acpi_status status;
+
+	if (!osc_pc_lpi_support_confirmed)
+		return -EOPNOTSUPP;
+
+	max_leaf_depth = 0;
+	if (!acpi_has_method(handle, "_LPI"))
+		return -EINVAL;
+	flat_state_cnt = 0;
+
+	while (ACPI_SUCCESS(status = acpi_get_parent(handle, &pr_ahandle))) {
+		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 = pr_ahandle;
+	}
+
+	info = kcalloc(max_leaf_depth + 1, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	pr_ahandle = pr->handle;
+	for (i = max_leaf_depth; i >= 0 && ACPI_SUCCESS(status); i--) {
+		handle = pr_ahandle;
+		ret = acpi_processor_evaluate_lpi(handle, info + i);
+		if (ret)
+			break;
+
+		status = acpi_get_parent(handle, &pr_ahandle);
+	}
+
+	/* flatten all the LPI states in the entire hierarchy */
+	flatten_lpi_states(pr, info, NULL, max_leaf_depth);
+
+	pr->power.count = flat_state_cnt;
+
+	for (i = 0; i <= max_leaf_depth; i++)
+		kfree(info[i].entries);
+
+	kfree(info);
+
+	/* Tell driver that _LPI is supported. */
+	pr->flags.has_lpi = 1;
+	pr->flags.power = 1;
+
+	return 0;
+}
+
+int __weak acpi_processor_ffh_lpi_probe(unsigned int cpu)
+{
+	return -ENODEV;
+}
+
+int __weak acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx)
+{
+	return -ENODEV;
+}
+
+/**
+ * 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_lpi_state *lpi;
+
+	pr = __this_cpu_read(processors);
+
+	if (unlikely(!pr))
+		return -EINVAL;
+
+	lpi = &pr->power.lpi_states[index];
+	if (lpi->entry_method == ACPI_CSTATE_FFH)
+		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_lpi_state *lpi;
+	struct cpuidle_state *state;
+	struct cpuidle_driver *drv = &acpi_idle_driver;
+
+	if (!pr->flags.has_lpi)
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < flat_state_cnt && 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;
+}
+
+/**
+ * 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);
+
+	return acpi_processor_setup_cpuidle_cx(pr, dev);
+}
+
+static int acpi_processor_get_power_info(struct acpi_processor *pr)
+{
+	int ret;
+
+	ret = acpi_processor_get_lpi_info(pr);
+	if (ret)
+		ret = acpi_processor_get_cstate_info(pr);
+
+	return ret;
+}
+
 int acpi_processor_hotplug(struct acpi_processor *pr)
 {
 	int ret = 0;
@@ -978,18 +1337,15 @@ int acpi_processor_hotplug(struct acpi_processor *pr)
 	if (disabled_by_idle_boot_param())
 		return 0;
 
-	if (nocst)
-		return -ENODEV;
-
 	if (!pr->flags.power_setup_done)
 		return -ENODEV;
 
 	dev = per_cpu(acpi_cpuidle_device, pr->id);
 	cpuidle_pause_and_lock();
 	cpuidle_disable_device(dev);
-	acpi_processor_get_power_info(pr);
-	if (pr->flags.power) {
-		acpi_processor_setup_cpuidle_cx(pr, dev);
+	ret = acpi_processor_get_power_info(pr);
+	if (!ret && pr->flags.power) {
+		acpi_processor_setup_cpuidle_dev(pr, dev);
 		ret = cpuidle_enable_device(dev);
 	}
 	cpuidle_resume_and_unlock();
@@ -997,7 +1353,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;
@@ -1006,9 +1362,6 @@ int acpi_processor_cst_has_changed(struct acpi_processor *pr)
 	if (disabled_by_idle_boot_param())
 		return 0;
 
-	if (nocst)
-		return -ENODEV;
-
 	if (!pr->flags.power_setup_done)
 		return -ENODEV;
 
@@ -1045,7 +1398,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);
 			}
 		}
@@ -1092,7 +1445,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 48779d678122..e2dcb0c51554 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_lpi_state {
+	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_lpi_state 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;
@@ -371,7 +389,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)
@@ -384,7 +402,7 @@ 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 288fac5294f5..65754566ae17 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -444,8 +444,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_confirmed;
 
 /* PCI Host Bridge _OSC: Capabilities DWORD 2: Support Field */
 #define OSC_PCI_EXT_CONFIG_SUPPORT		0x00000001
-- 
1.9.1

[PATCH v5 3/5] arm64: cpuidle: drop __init section marker to arm_cpuidle_init

[Sudeep Holla]

Commit ea389daa7fd9 ("arm64: cpuidle: add __init section marker to
arm_cpuidle_init") added the __init annotation to arm_cpuidle_init
as it was not needed after booting which was correct at that time.

However with the introduction of ACPI LPI support, this will be used
from cpuhotplug path in ACPI processor driver.

This patch drops the __init annotation from arm_cpuidle_init to avoid
the following warning:

WARNING: vmlinux.o(.text+0x113c8): Section mismatch in reference from the
	function acpi_processor_ffh_lpi_probe() to the function
	.init.text:arm_cpuidle_init()
The function acpi_processor_ffh_lpi_probe() references
the function __init arm_cpuidle_init().
This is often because acpi_processor_ffh_lpi_probe lacks a __init
annotation or the annotation of arm_cpuidle_init is wrong.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/cpuidle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index 9047cab68fd3..7ce589ca54a4 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -15,7 +15,7 @@
 #include <asm/cpuidle.h>
 #include <asm/cpu_ops.h>
 
-int __init arm_cpuidle_init(unsigned int cpu)
+int arm_cpuidle_init(unsigned int cpu)
 {
 	int ret = -EOPNOTSUPP;
 
-- 
1.9.1

[PATCH v5 3/5] arm64: cpuidle: drop __init section marker to arm_cpuidle_init

[Sudeep Holla]

Commit ea389daa7fd9 ("arm64: cpuidle: add __init section marker to
arm_cpuidle_init") added the __init annotation to arm_cpuidle_init
as it was not needed after booting which was correct at that time.

However with the introduction of ACPI LPI support, this will be used
from cpuhotplug path in ACPI processor driver.

This patch drops the __init annotation from arm_cpuidle_init to avoid
the following warning:

WARNING: vmlinux.o(.text+0x113c8): Section mismatch in reference from the
	function acpi_processor_ffh_lpi_probe() to the function
	.init.text:arm_cpuidle_init()
The function acpi_processor_ffh_lpi_probe() references
the function __init arm_cpuidle_init().
This is often because acpi_processor_ffh_lpi_probe lacks a __init
annotation or the annotation of arm_cpuidle_init is wrong.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/cpuidle.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index 9047cab68fd3..7ce589ca54a4 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -15,7 +15,7 @@
 #include <asm/cpuidle.h>
 #include <asm/cpu_ops.h>
 
-int __init arm_cpuidle_init(unsigned int cpu)
+int arm_cpuidle_init(unsigned int cpu)
 {
 	int ret = -EOPNOTSUPP;
 
-- 
1.9.1

[PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Sudeep Holla]

This patch adds appropriate callbacks to support ACPI Low Power Idle
(LPI) on ARM64.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/acpi.c | 48 +++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/psci.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index d1ce8e2f98b9..bf82ce5c8fce 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -18,6 +18,7 @@
 #include <linux/acpi.h>
 #include <linux/bootmem.h>
 #include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
 #include <linux/init.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
@@ -25,6 +26,9 @@
 #include <linux/of_fdt.h>
 #include <linux/smp.h>
 
+#include <acpi/processor.h>
+
+#include <asm/cpuidle.h>
 #include <asm/cputype.h>
 #include <asm/cpu_ops.h>
 #include <asm/smp_plat.h>
@@ -211,6 +215,50 @@ void __init acpi_boot_table_init(void)
 	}
 }
 
+int acpi_processor_ffh_lpi_probe(unsigned int cpu)
+{
+	return arm_cpuidle_init(cpu);
+}
+
+#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT	BIT(0)
+#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT	BIT(1)
+#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT	BIT(2)
+#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT	BIT(3)
+#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT	\
+	(ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT)
+
+struct acpi_lpi_state *lpi;
+int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx)
+{
+	int ret = 0;
+	bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT;
+
+	if (!idx) {
+		cpu_do_idle();
+		return idx;
+	}
+
+	/* TODO cpu_pm_{enter,exit} can be done in generic code ? */
+	if (save_ctx)
+		ret = cpu_pm_enter();
+	if (!ret) {
+		/*
+		 * Pass idle state index to cpu_suspend which in turn will
+		 * call the CPU ops suspend protocol with idle index as a
+		 * parameter.
+		 */
+		ret = arm_cpuidle_suspend(idx);
+
+		if (save_ctx)
+			cpu_pm_exit();
+	}
+
+	return ret ? -1 : idx;
+}
+
 #ifdef CONFIG_ACPI_APEI
 pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
 {
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index fa4ea22ca12e..e06bfee68e1d 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) "psci: " fmt
 
+#include <linux/acpi.h>
 #include <linux/arm-smccc.h>
 #include <linux/cpuidle.h>
 #include <linux/errno.h>
@@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	return ret;
 }
 
+#ifdef CONFIG_ACPI
+#include <acpi/processor.h>
+
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+	int i, count;
+	u32 *psci_states;
+	struct acpi_processor *pr;
+	struct acpi_lpi_state *lpi;
+
+	pr = per_cpu(processors, cpu);
+	if (unlikely(!pr || !pr->flags.has_lpi))
+		return -EINVAL;
+
+	/*
+	 * If the PSCI cpu_suspend function hook has not been initialized
+	 * idle states must not be enabled, so bail out
+	 */
+	if (!psci_ops.cpu_suspend)
+		return -EOPNOTSUPP;
+
+	count = pr->power.count - 1;
+	if (!count)
+		return -ENODEV;
+
+	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+	if (!psci_states)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		u32 state;
+
+		lpi = &pr->power.lpi_states[i + 1];
+		state = lpi->address & 0xFFFFFFFF;
+		if (!psci_power_state_is_valid(state)) {
+			pr_warn("Invalid PSCI power state %#x\n", state);
+			kfree(psci_states);
+			return -EINVAL;
+		}
+		psci_states[i] = state;
+	}
+	/* Idle states parsed correctly, initialize per-cpu pointer */
+	per_cpu(psci_power_state, cpu) = psci_states;
+	return 0;
+}
+#else
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+	return -EINVAL;
+}
+#endif
+
 int psci_cpu_init_idle(unsigned int cpu)
 {
 	struct device_node *cpu_node;
 	int ret;
 
+	if (!acpi_disabled)
+		return psci_acpi_cpu_init_idle(cpu);
+
 	cpu_node = of_get_cpu_node(cpu, NULL);
 	if (!cpu_node)
 		return -ENODEV;
-- 
1.9.1

[PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Sudeep Holla]

This patch adds appropriate callbacks to support ACPI Low Power Idle
(LPI) on ARM64.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/acpi.c | 48 +++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/psci.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
index d1ce8e2f98b9..bf82ce5c8fce 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -18,6 +18,7 @@
 #include <linux/acpi.h>
 #include <linux/bootmem.h>
 #include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
 #include <linux/init.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
@@ -25,6 +26,9 @@
 #include <linux/of_fdt.h>
 #include <linux/smp.h>
 
+#include <acpi/processor.h>
+
+#include <asm/cpuidle.h>
 #include <asm/cputype.h>
 #include <asm/cpu_ops.h>
 #include <asm/smp_plat.h>
@@ -211,6 +215,50 @@ void __init acpi_boot_table_init(void)
 	}
 }
 
+int acpi_processor_ffh_lpi_probe(unsigned int cpu)
+{
+	return arm_cpuidle_init(cpu);
+}
+
+#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT	BIT(0)
+#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT	BIT(1)
+#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT	BIT(2)
+#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT	BIT(3)
+#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT	\
+	(ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT)
+
+struct acpi_lpi_state *lpi;
+int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx)
+{
+	int ret = 0;
+	bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT;
+
+	if (!idx) {
+		cpu_do_idle();
+		return idx;
+	}
+
+	/* TODO cpu_pm_{enter,exit} can be done in generic code ? */
+	if (save_ctx)
+		ret = cpu_pm_enter();
+	if (!ret) {
+		/*
+		 * Pass idle state index to cpu_suspend which in turn will
+		 * call the CPU ops suspend protocol with idle index as a
+		 * parameter.
+		 */
+		ret = arm_cpuidle_suspend(idx);
+
+		if (save_ctx)
+			cpu_pm_exit();
+	}
+
+	return ret ? -1 : idx;
+}
+
 #ifdef CONFIG_ACPI_APEI
 pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
 {
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index fa4ea22ca12e..e06bfee68e1d 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) "psci: " fmt
 
+#include <linux/acpi.h>
 #include <linux/arm-smccc.h>
 #include <linux/cpuidle.h>
 #include <linux/errno.h>
@@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	return ret;
 }
 
+#ifdef CONFIG_ACPI
+#include <acpi/processor.h>
+
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+	int i, count;
+	u32 *psci_states;
+	struct acpi_processor *pr;
+	struct acpi_lpi_state *lpi;
+
+	pr = per_cpu(processors, cpu);
+	if (unlikely(!pr || !pr->flags.has_lpi))
+		return -EINVAL;
+
+	/*
+	 * If the PSCI cpu_suspend function hook has not been initialized
+	 * idle states must not be enabled, so bail out
+	 */
+	if (!psci_ops.cpu_suspend)
+		return -EOPNOTSUPP;
+
+	count = pr->power.count - 1;
+	if (!count)
+		return -ENODEV;
+
+	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+	if (!psci_states)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		u32 state;
+
+		lpi = &pr->power.lpi_states[i + 1];
+		state = lpi->address & 0xFFFFFFFF;
+		if (!psci_power_state_is_valid(state)) {
+			pr_warn("Invalid PSCI power state %#x\n", state);
+			kfree(psci_states);
+			return -EINVAL;
+		}
+		psci_states[i] = state;
+	}
+	/* Idle states parsed correctly, initialize per-cpu pointer */
+	per_cpu(psci_power_state, cpu) = psci_states;
+	return 0;
+}
+#else
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
+{
+	return -EINVAL;
+}
+#endif
+
 int psci_cpu_init_idle(unsigned int cpu)
 {
 	struct device_node *cpu_node;
 	int ret;
 
+	if (!acpi_disabled)
+		return psci_acpi_cpu_init_idle(cpu);
+
 	cpu_node = of_get_cpu_node(cpu, NULL);
 	if (!cpu_node)
 		return -ENODEV;
-- 
1.9.1

[PATCH v5 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Sudeep Holla]

Now that ACPI processor idle driver supports LPI(Low Power Idle), lets
enable ACPI_PROCESSOR_IDLE for ARM64 too.

This patch just removes the IA64 and X86 dependency on ACPI_PROCESSOR_IDLE

Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 093bfcc4f9c3..de5c7c09de69 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -238,7 +238,7 @@ config ACPI_CPPC_LIB
 config ACPI_PROCESSOR
 	tristate "Processor"
 	depends on X86 || IA64 || ARM64
-	select ACPI_PROCESSOR_IDLE if X86 || IA64
+	select ACPI_PROCESSOR_IDLE
 	select ACPI_CPU_FREQ_PSS if X86 || IA64
 	default y
 	help
-- 
1.9.1

[PATCH v5 5/5] ACPI : enable ACPI_PROCESSOR_IDLE on ARM64

[Sudeep Holla]

Now that ACPI processor idle driver supports LPI(Low Power Idle), lets
enable ACPI_PROCESSOR_IDLE for ARM64 too.

This patch just removes the IA64 and X86 dependency on ACPI_PROCESSOR_IDLE

Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/acpi/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index 093bfcc4f9c3..de5c7c09de69 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -238,7 +238,7 @@ config ACPI_CPPC_LIB
 config ACPI_PROCESSOR
 	tristate "Processor"
 	depends on X86 || IA64 || ARM64
-	select ACPI_PROCESSOR_IDLE if X86 || IA64
+	select ACPI_PROCESSOR_IDLE
 	select ACPI_CPU_FREQ_PSS if X86 || IA64
 	default y
 	help
-- 
1.9.1

Re: [PATCH v5 1/5] ACPI / processor_idle: introduce ACPI_PROCESSOR_CSTATE

[Rafael J. Wysocki]

On Wed, May 11, 2016 at 5:37 PM, Sudeep Holla <sudeep.holla@arm.com> 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 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: x86@kernel.org
> Cc: linux-ia64@vger.kernel.org
> 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          |  4 +++
>  drivers/acpi/processor_idle.c | 80 ++++++++++++++++++++++++++++---------------
>  include/acpi/processor.h      |  2 +-
>  5 files changed, 60 insertions(+), 28 deletions(-)
>
> diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
> index b534ebab36ea..e820670d7243 100644
> --- a/arch/ia64/Kconfig
> +++ b/arch/ia64/Kconfig
> @@ -15,6 +15,7 @@ config IA64
>         select ARCH_MIGHT_HAVE_PC_SERIO
>         select PCI if (!IA64_HP_SIM)
>         select ACPI if (!IA64_HP_SIM)
> +       select ACPI_PROCESSOR_CSTATE if ACPI

You don't need this ->

>         select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
>         select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI
>         select HAVE_UNSTABLE_SCHED_CLOCK
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 2dc18605831f..cb3e14757c9c 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -18,6 +18,7 @@ config X86_64
>  config X86
>         def_bool y
>         select ACPI_LEGACY_TABLES_LOOKUP        if ACPI
> +       select ACPI_PROCESSOR_CSTATE            if ACPI
>         select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
>         select ANON_INODES
>         select ARCH_CLOCKSOURCE_DATA
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index b7e2e776397d..093bfcc4f9c3 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -213,6 +213,10 @@ config ACPI_CPU_FREQ_PSS
>         bool
>         select THERMAL
>
> +config ACPI_PROCESSOR_CSTATE
> +       bool

-> if you do "def_bool y" here.

> +       depends on IA64 || X86
> +
>  config ACPI_PROCESSOR_IDLE
>         bool
>         select CPU_IDLE

Re: [PATCH v5 1/5] ACPI / processor_idle: introduce ACPI_PROCESSOR_CSTATE

[Rafael J. Wysocki]

On Wed, May 11, 2016 at 5:37 PM, Sudeep Holla <sudeep.holla@arm.com> 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 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: x86@kernel.org
> Cc: linux-ia64@vger.kernel.org
> 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          |  4 +++
>  drivers/acpi/processor_idle.c | 80 ++++++++++++++++++++++++++++---------------
>  include/acpi/processor.h      |  2 +-
>  5 files changed, 60 insertions(+), 28 deletions(-)
>
> diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
> index b534ebab36ea..e820670d7243 100644
> --- a/arch/ia64/Kconfig
> +++ b/arch/ia64/Kconfig
> @@ -15,6 +15,7 @@ config IA64
>         select ARCH_MIGHT_HAVE_PC_SERIO
>         select PCI if (!IA64_HP_SIM)
>         select ACPI if (!IA64_HP_SIM)
> +       select ACPI_PROCESSOR_CSTATE if ACPI

You don't need this ->

>         select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
>         select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI
>         select HAVE_UNSTABLE_SCHED_CLOCK
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 2dc18605831f..cb3e14757c9c 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -18,6 +18,7 @@ config X86_64
>  config X86
>         def_bool y
>         select ACPI_LEGACY_TABLES_LOOKUP        if ACPI
> +       select ACPI_PROCESSOR_CSTATE            if ACPI
>         select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
>         select ANON_INODES
>         select ARCH_CLOCKSOURCE_DATA
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index b7e2e776397d..093bfcc4f9c3 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -213,6 +213,10 @@ config ACPI_CPU_FREQ_PSS
>         bool
>         select THERMAL
>
> +config ACPI_PROCESSOR_CSTATE
> +       bool

-> if you do "def_bool y" here.

> +       depends on IA64 || X86
> +
>  config ACPI_PROCESSOR_IDLE
>         bool
>         select CPU_IDLE

Re: [PATCH v5 1/5] ACPI / processor_idle: introduce ACPI_PROCESSOR_CSTATE

[Sudeep Holla]

On 11/05/16 17:23, Rafael J. Wysocki wrote:
> On Wed, May 11, 2016 at 5:37 PM, Sudeep Holla <sudeep.holla@arm.com> 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 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: x86@kernel.org
>> Cc: linux-ia64@vger.kernel.org
>> 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          |  4 +++
>>   drivers/acpi/processor_idle.c | 80 ++++++++++++++++++++++++++++---------------
>>   include/acpi/processor.h      |  2 +-
>>   5 files changed, 60 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
>> index b534ebab36ea..e820670d7243 100644
>> --- a/arch/ia64/Kconfig
>> +++ b/arch/ia64/Kconfig
>> @@ -15,6 +15,7 @@ config IA64
>>          select ARCH_MIGHT_HAVE_PC_SERIO
>>          select PCI if (!IA64_HP_SIM)
>>          select ACPI if (!IA64_HP_SIM)
>> +       select ACPI_PROCESSOR_CSTATE if ACPI
>
> You don't need this ->
>
>>          select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
>>          select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI
>>          select HAVE_UNSTABLE_SCHED_CLOCK
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 2dc18605831f..cb3e14757c9c 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -18,6 +18,7 @@ config X86_64
>>   config X86
>>          def_bool y
>>          select ACPI_LEGACY_TABLES_LOOKUP        if ACPI
>> +       select ACPI_PROCESSOR_CSTATE            if ACPI
>>          select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
>>          select ANON_INODES
>>          select ARCH_CLOCKSOURCE_DATA
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index b7e2e776397d..093bfcc4f9c3 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -213,6 +213,10 @@ config ACPI_CPU_FREQ_PSS
>>          bool
>>          select THERMAL
>>
>> +config ACPI_PROCESSOR_CSTATE
>> +       bool
>
> -> if you do "def_bool y" here.
>

I agree and I did exactly the same, but then was not sure on your
preference. So dropped it :)

-- 
Regards,
Sudeep

Re: [PATCH v5 1/5] ACPI / processor_idle: introduce ACPI_PROCESSOR_CSTATE

[Sudeep Holla]

On 11/05/16 17:23, Rafael J. Wysocki wrote:
> On Wed, May 11, 2016 at 5:37 PM, Sudeep Holla <sudeep.holla@arm.com> 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 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: x86@kernel.org
>> Cc: linux-ia64@vger.kernel.org
>> 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          |  4 +++
>>   drivers/acpi/processor_idle.c | 80 ++++++++++++++++++++++++++++---------------
>>   include/acpi/processor.h      |  2 +-
>>   5 files changed, 60 insertions(+), 28 deletions(-)
>>
>> diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
>> index b534ebab36ea..e820670d7243 100644
>> --- a/arch/ia64/Kconfig
>> +++ b/arch/ia64/Kconfig
>> @@ -15,6 +15,7 @@ config IA64
>>          select ARCH_MIGHT_HAVE_PC_SERIO
>>          select PCI if (!IA64_HP_SIM)
>>          select ACPI if (!IA64_HP_SIM)
>> +       select ACPI_PROCESSOR_CSTATE if ACPI
>
> You don't need this ->
>
>>          select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
>>          select ARCH_MIGHT_HAVE_ACPI_PDC if ACPI
>>          select HAVE_UNSTABLE_SCHED_CLOCK
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index 2dc18605831f..cb3e14757c9c 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -18,6 +18,7 @@ config X86_64
>>   config X86
>>          def_bool y
>>          select ACPI_LEGACY_TABLES_LOOKUP        if ACPI
>> +       select ACPI_PROCESSOR_CSTATE            if ACPI
>>          select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI
>>          select ANON_INODES
>>          select ARCH_CLOCKSOURCE_DATA
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index b7e2e776397d..093bfcc4f9c3 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -213,6 +213,10 @@ config ACPI_CPU_FREQ_PSS
>>          bool
>>          select THERMAL
>>
>> +config ACPI_PROCESSOR_CSTATE
>> +       bool
>
> -> if you do "def_bool y" here.
>

I agree and I did exactly the same, but then was not sure on your
preference. So dropped it :)

-- 
Regards,
Sudeep

Re: [PATCH v5 1/5] ACPI / processor_idle: introduce ACPI_PROCESSOR_CSTATE

[Len Brown]

What is the functional goal/purpose of adding CONFIG_ACPI_PROCESSOR_CSTATE?

If the answer is that it saves code space on an ARM build, how much
space does it save?

Len Brown, Intel Open Source Technology Center

Re: [PATCH v5 1/5] ACPI / processor_idle: introduce ACPI_PROCESSOR_CSTATE

[Len Brown]

What is the functional goal/purpose of adding CONFIG_ACPI_PROCESSOR_CSTATE?

If the answer is that it saves code space on an ARM build, how much
space does it save?

Len Brown, Intel Open Source Technology Center

Re: [PATCH v5 1/5] ACPI / processor_idle: introduce ACPI_PROCESSOR_CSTATE

[Sudeep Holla]

(I seem to have 2 emails, replying on the second)

On 11/05/16 19:28, Len Brown wrote:
> What is the functional goal/purpose of adding CONFIG_ACPI_PROCESSOR_CSTATE?
>

Avoid adding unnecessary dummy implementations of functions and
variables that will never be used on ARM64 and also looks ugly IMO. 
E.g.:	arch_safe_halt
	boot_option_idle_override
	IDLE_NOMWAIT
	acpi_unlazy_tlb
	acpi_processor_cstate_check
	disabled_by_idle_boot_param and more...

> If the answer is that it saves code space on an ARM build, how much
> space does it save?
>

NO, it doesn't even add a kB of code I believe, so that's definitely not
the reason. I am fine to retain if we can find a saner way to solve the
above issue.

-- 
Regards,
Sudeep

Re: [PATCH v5 1/5] ACPI / processor_idle: introduce ACPI_PROCESSOR_CSTATE

[Sudeep Holla]

(I seem to have 2 emails, replying on the second)

On 11/05/16 19:28, Len Brown wrote:
> What is the functional goal/purpose of adding CONFIG_ACPI_PROCESSOR_CSTATE?
>

Avoid adding unnecessary dummy implementations of functions and
variables that will never be used on ARM64 and also looks ugly IMO. 
E.g.:	arch_safe_halt
	boot_option_idle_override
	IDLE_NOMWAIT
	acpi_unlazy_tlb
	acpi_processor_cstate_check
	disabled_by_idle_boot_param and more...

> If the answer is that it saves code space on an ARM build, how much
> space does it save?
>

NO, it doesn't even add a kB of code I believe, so that's definitely not
the reason. I am fine to retain if we can find a saner way to solve the
above issue.

-- 
Regards,
Sudeep

[UPDATE][PATCH v5] ACPI / processor_idle: introduce 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 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>
---
 drivers/acpi/Kconfig          |  4 +++
 drivers/acpi/processor_idle.c | 80 ++++++++++++++++++++++++++++---------------
 include/acpi/processor.h      |  2 +-
 3 files changed, 58 insertions(+), 28 deletions(-)

Updated to def_bool and removed selection from IA64 and X86

diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index b7e2e776397d..1358fb7d7a68 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -213,6 +213,10 @@ config ACPI_CPU_FREQ_PSS
 	bool
 	select THERMAL
 
+config ACPI_PROCESSOR_CSTATE
+	def_bool y
+	depends on IA64 || X86
+
 config ACPI_PROCESSOR_IDLE
 	bool
 	select CPU_IDLE
diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
index 444e3745c8b3..ca0de35d1c3a 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_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,50 @@ static int acpi_processor_setup_cpuidle_states(struct acpi_processor *pr)
 	return 0;
 }
 
+static inline void acpi_processor_cstate_first_run_checks(void)
+{
+	acpi_status status;
+	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++;
+
+	if (acpi_gbl_FADT.cst_control && !nocst) {
+		status = acpi_os_write_port(acpi_gbl_FADT.smi_command,
+					    acpi_gbl_FADT.cst_control, 8);
+		if (ACPI_FAILURE(status))
+			ACPI_EXCEPTION((AE_INFO, status,
+					"Notifying BIOS of _CST ability failed"));
+	}
+}
+#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 /* CONFIG_ACPI_PROCESSOR_CSTATE */
+
 int acpi_processor_hotplug(struct acpi_processor *pr)
 {
 	int ret = 0;
@@ -1015,35 +1060,16 @@ static int acpi_processor_registered;
 
 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 =
-		    acpi_os_write_port(acpi_gbl_FADT.smi_command, acpi_gbl_FADT.cst_control, 8);
-		if (ACPI_FAILURE(status)) {
-			ACPI_EXCEPTION((AE_INFO, status,
-					"Notifying BIOS of _CST ability failed"));
-		}
-	}
-
-	acpi_processor_get_power_info(pr);
-	pr->flags.power_setup_done = 1;
+	if (!acpi_processor_get_power_info(pr))
+		pr->flags.power_setup_done = 1;
 
 	/*
 	 * Install the idle handler if processor power management is supported.
diff --git a/include/acpi/processor.h b/include/acpi/processor.h
index 6f1805dd5d3c..48779d678122 100644
--- a/include/acpi/processor.h
+++ b/include/acpi/processor.h
@@ -242,7 +242,7 @@ 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_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 v5 2/5] ACPI / processor_idle: Add support for Low Power Idle(LPI) states

[Prakash, Prashanth]

Hi Sudeep,

On 5/11/2016 9:37 AM, Sudeep Holla wrote:
> +
> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
> +{
> +	int ret, i;
> +	struct acpi_lpi_states_array *info;
> +	struct acpi_device *d = NULL;
> +	acpi_handle handle = pr->handle, pr_ahandle;
> +	acpi_status status;
> +
> +	if (!osc_pc_lpi_support_confirmed)
> +		return -EOPNOTSUPP;
> +
> +	max_leaf_depth = 0;
> +	if (!acpi_has_method(handle, "_LPI"))
> +		return -EINVAL;
> +	flat_state_cnt = 0;
> +
> +	while (ACPI_SUCCESS(status = acpi_get_parent(handle, &pr_ahandle))) {
> +		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 = pr_ahandle;
> +	}
> +
In the above loop, we break when we find a device with HID == ACPI_PROCESSOR_CONTAINER_HID.
Shouldn't we continue to parse as long as the parent HID == ACPI_PROCESSOR_CONTAINER_HID?
This is required to make sure we parse states in levels higher than cluster level
in processor hierarchy.

Also, I think it might be safe to break out of the loop if we didn't find _LPI
package, instead of continuing. Given  the presence of LPI entry: "Enabled Parent
State", I can't think of a non-ambiguous scenario where we might find LPI packages
in state N and N+2, but not in N+1, as we will not be able to figure out which
state in N enables which states in N+2. Thoughts?

Thanks,
Prashanth

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

[Sudeep Holla]

On 17/05/16 18:46, Prakash, Prashanth wrote:
> Hi Sudeep,
>
> On 5/11/2016 9:37 AM, Sudeep Holla wrote:
>> +
>> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
>> +{
>> +	int ret, i;
>> +	struct acpi_lpi_states_array *info;
>> +	struct acpi_device *d = NULL;
>> +	acpi_handle handle = pr->handle, pr_ahandle;
>> +	acpi_status status;
>> +
>> +	if (!osc_pc_lpi_support_confirmed)
>> +		return -EOPNOTSUPP;
>> +
>> +	max_leaf_depth = 0;
>> +	if (!acpi_has_method(handle, "_LPI"))
>> +		return -EINVAL;
>> +	flat_state_cnt = 0;
>> +
>> +	while (ACPI_SUCCESS(status = acpi_get_parent(handle, &pr_ahandle))) {
>> +		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 = pr_ahandle;
>> +	}
>> +
> In the above loop, we break when we find a device with HID ==
> ACPI_PROCESSOR_CONTAINER_HID. Shouldn't we continue to parse as long as the
> parent HID == ACPI_PROCESSOR_CONTAINER_HID? This is required to make sure we
> parse states in levels higher than cluster level in processor hierarchy.
>

Yes, thanks for pointing that out. With just clusters in _LPI on my dev
board, I missed it.

> Also, I think it might be safe to break out of the loop if we didn't find
> _LPI package, instead of continuing. Given  the presence of LPI entry:
> "Enabled Parent State", I can't think of a non-ambiguous scenario where we
> might find LPI packages in state N and N+2, but not in N+1, as we will not
> be able to figure out which state in N enables which states in N+2.
> Thoughts?

Though I admit I haven't thought in detail on how to deal with the
asymmetric topology, but that was the reason why I continue instead of
breaking.

Excerpts from the spec: "... This example is symmetric but that is not a
requirement. For example, a system may contain a different number of
processors in different containers or an asymmetric hierarchy where one
side of the topology tree is deeper than another...."

-- 
Regards,
Sudeep

-- 
Regards,
Sudeep

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

[Prakash, Prashanth]

On 5/18/2016 11:37 AM, Sudeep Holla wrote:
>
>
> On 17/05/16 18:46, Prakash, Prashanth wrote:
>> Hi Sudeep,
>>
>> On 5/11/2016 9:37 AM, Sudeep Holla wrote:
>>> +
>>> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
>>> +{
>>> +    int ret, i;
>>> +    struct acpi_lpi_states_array *info;
>>> +    struct acpi_device *d = NULL;
>>> +    acpi_handle handle = pr->handle, pr_ahandle;
>>> +    acpi_status status;
>>> +
>>> +    if (!osc_pc_lpi_support_confirmed)
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    max_leaf_depth = 0;
>>> +    if (!acpi_has_method(handle, "_LPI"))
>>> +        return -EINVAL;
>>> +    flat_state_cnt = 0;
>>> +
>>> +    while (ACPI_SUCCESS(status = acpi_get_parent(handle, &pr_ahandle))) {
>>> +        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 = pr_ahandle;
>>> +    }
>>> +
>> In the above loop, we break when we find a device with HID ==
>> ACPI_PROCESSOR_CONTAINER_HID. Shouldn't we continue to parse as long as the
>> parent HID == ACPI_PROCESSOR_CONTAINER_HID? This is required to make sure we
>> parse states in levels higher than cluster level in processor hierarchy.
>>
>
> Yes, thanks for pointing that out. With just clusters in _LPI on my dev
> board, I missed it.
>
Same reason, I failed to notice it all this time :)
>> Also, I think it might be safe to break out of the loop if we didn't find
>> _LPI package, instead of continuing. Given  the presence of LPI entry:
>> "Enabled Parent State", I can't think of a non-ambiguous scenario where we
>> might find LPI packages in state N and N+2, but not in N+1, as we will not
>> be able to figure out which state in N enables which states in N+2.
>> Thoughts?
>
> Though I admit I haven't thought in detail on how to deal with the
> asymmetric topology, but that was the reason why I continue instead of
> breaking.
>
> Excerpts from the spec: "... This example is symmetric but that is not a
> requirement. For example, a system may contain a different number of
> processors in different containers or an asymmetric hierarchy where one
> side of the topology tree is deeper than another...."
>
If it addresses asymmetric topology, sure we can keep as it doesn't impact other
scenarios. Also, we need to set handle=pr_ahandle prior to the continue statement.


Thanks,
Prashanth

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

[Sudeep Holla]

On 18/05/16 20:13, Prakash, Prashanth wrote:
>
>
> On 5/18/2016 11:37 AM, Sudeep Holla wrote:
>>
>>
>> On 17/05/16 18:46, Prakash, Prashanth wrote:
>>> Hi Sudeep,
>>>
>>> On 5/11/2016 9:37 AM, Sudeep Holla wrote:
>>>> +
>>>> +static int acpi_processor_get_lpi_info(struct acpi_processor *pr)
>>>> +{
>>>> +    int ret, i;
>>>> +    struct acpi_lpi_states_array *info;
>>>> +    struct acpi_device *d = NULL;
>>>> +    acpi_handle handle = pr->handle, pr_ahandle;
>>>> +    acpi_status status;
>>>> +
>>>> +    if (!osc_pc_lpi_support_confirmed)
>>>> +        return -EOPNOTSUPP;
>>>> +
>>>> +    max_leaf_depth = 0;
>>>> +    if (!acpi_has_method(handle, "_LPI"))
>>>> +        return -EINVAL;
>>>> +    flat_state_cnt = 0;
>>>> +
>>>> +    while (ACPI_SUCCESS(status = acpi_get_parent(handle, &pr_ahandle))) {
>>>> +        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 = pr_ahandle;
>>>> +    }
>>>> +
>>> In the above loop, we break when we find a device with HID ==
>>> ACPI_PROCESSOR_CONTAINER_HID. Shouldn't we continue to parse as long as the
>>> parent HID == ACPI_PROCESSOR_CONTAINER_HID? This is required to make sure we
>>> parse states in levels higher than cluster level in processor hierarchy.
>>>
>>
>> Yes, thanks for pointing that out. With just clusters in _LPI on my dev
>> board, I missed it.
>>
> Same reason, I failed to notice it all this time :)

No worries.

>>> Also, I think it might be safe to break out of the loop if we didn't find
>>> _LPI package, instead of continuing. Given  the presence of LPI entry:
>>> "Enabled Parent State", I can't think of a non-ambiguous scenario where we
>>> might find LPI packages in state N and N+2, but not in N+1, as we will not
>>> be able to figure out which state in N enables which states in N+2.
>>> Thoughts?
>>
>> Though I admit I haven't thought in detail on how to deal with the
>> asymmetric topology, but that was the reason why I continue instead of
>> breaking.
>>
>> Excerpts from the spec: "... This example is symmetric but that is not a
>> requirement. For example, a system may contain a different number of
>> processors in different containers or an asymmetric hierarchy where one
>> side of the topology tree is deeper than another...."
>>
> If it addresses asymmetric topology, sure we can keep as it doesn't impact other
> scenarios. Also, we need to set handle=pr_ahandle prior to the continue statement.
>

Yes I noticed it yesterday, the more I think, I feel we can break out of
the loop. At any level, we need to have container nodes and that must
contain _LPI irrespective of asymmetricity. So you were right. I have
fixed accordingly and have pushed out on my branch.

-- 
Regards,
Sudeep

Re: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Lorenzo Pieralisi]

[+ Daniel, Kevin]

On Wed, May 11, 2016 at 04:37:41PM +0100, Sudeep Holla wrote:
> This patch adds appropriate callbacks to support ACPI Low Power Idle
> (LPI) on ARM64.
> 
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel@lists.infradead.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/kernel/acpi.c | 48 +++++++++++++++++++++++++++++++++++++++++
>  drivers/firmware/psci.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c

I think we can add this to arm64/kernel/cpuidle.c so that we have all
arch code managing idle in one place.

> index d1ce8e2f98b9..bf82ce5c8fce 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -18,6 +18,7 @@
>  #include <linux/acpi.h>
>  #include <linux/bootmem.h>
>  #include <linux/cpumask.h>
> +#include <linux/cpu_pm.h>
>  #include <linux/init.h>
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
> @@ -25,6 +26,9 @@
>  #include <linux/of_fdt.h>
>  #include <linux/smp.h>
>  
> +#include <acpi/processor.h>
> +
> +#include <asm/cpuidle.h>
>  #include <asm/cputype.h>
>  #include <asm/cpu_ops.h>
>  #include <asm/smp_plat.h>
> @@ -211,6 +215,50 @@ void __init acpi_boot_table_init(void)
>  	}
>  }
>  
> +int acpi_processor_ffh_lpi_probe(unsigned int cpu)
> +{
> +	return arm_cpuidle_init(cpu);
> +}
> +
> +#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT	BIT(0)
> +#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT	BIT(1)
> +#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT	BIT(2)
> +#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT	BIT(3)
> +#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT	\
> +	(ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT |	\
> +	 ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT |	\
> +	 ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT |	\
> +	 ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT)
> +
> +struct acpi_lpi_state *lpi;
> +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx)
> +{
> +	int ret = 0;
> +	bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT;

I am not really that keen on this, as you know. Those flags are
there to say "save these components registers". I see the CPU PM
notifiers as a way to save/restore CPU peripheral state, but
they should *not* carry out any action that affects the power
state itself, that's down to the suspend finisher (eg PSCI),
because that's where the specific idle states are managed.

I agree we have no clue whatsoever on what we *really* need
to save/restore, but that's orthogonal to what you are solving
here.

See eg gic_cpu_if_down(). Do we call it from the GIC CPU PM notifier ?
No. We should not handle the same problem differently.

On top of that, we have no way to solve this problem for DT,
all I am saying is that it is ill-defined and given that LPI
is new I'd rather we got it right from the beginning.

I am open to suggestions here.

> +
> +	if (!idx) {
> +		cpu_do_idle();
> +		return idx;
> +	}
> +
> +	/* TODO cpu_pm_{enter,exit} can be done in generic code ? */
> +	if (save_ctx)
> +		ret = cpu_pm_enter();
> +	if (!ret) {
> +		/*
> +		 * Pass idle state index to cpu_suspend which in turn will
> +		 * call the CPU ops suspend protocol with idle index as a
> +		 * parameter.
> +		 */
> +		ret = arm_cpuidle_suspend(idx);
> +
> +		if (save_ctx)
> +			cpu_pm_exit();
> +	}
> +
> +	return ret ? -1 : idx;

The body of this function (if we remove save_ctx) is identical
to arm_enter_idle_state(), it would be nice if we found a way
where to put this code and share it with the ARM CPUidle driver,
but I am not too fussed about that either.

> +}
> +
>  #ifdef CONFIG_ACPI_APEI
>  pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>  {
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index fa4ea22ca12e..e06bfee68e1d 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -13,6 +13,7 @@
>  
>  #define pr_fmt(fmt) "psci: " fmt
>  
> +#include <linux/acpi.h>
>  #include <linux/arm-smccc.h>
>  #include <linux/cpuidle.h>
>  #include <linux/errno.h>
> @@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_ACPI
> +#include <acpi/processor.h>
> +
> +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
> +{
> +	int i, count;
> +	u32 *psci_states;
> +	struct acpi_processor *pr;
> +	struct acpi_lpi_state *lpi;
> +
> +	pr = per_cpu(processors, cpu);
> +	if (unlikely(!pr || !pr->flags.has_lpi))
> +		return -EINVAL;
> +
> +	/*
> +	 * If the PSCI cpu_suspend function hook has not been initialized
> +	 * idle states must not be enabled, so bail out
> +	 */
> +	if (!psci_ops.cpu_suspend)
> +		return -EOPNOTSUPP;
> +
> +	count = pr->power.count - 1;

Nit: I am not sure this can happen, but you really do not want
count to become == -1.

> +	if (!count)
> +		return -ENODEV;
> +
> +	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> +	if (!psci_states)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < count; i++) {
> +		u32 state;
> +
> +		lpi = &pr->power.lpi_states[i + 1];
> +		state = lpi->address & 0xFFFFFFFF;
> +		if (!psci_power_state_is_valid(state)) {
> +			pr_warn("Invalid PSCI power state %#x\n", state);
> +			kfree(psci_states);
> +			return -EINVAL;
> +		}
> +		psci_states[i] = state;
> +	}
> +	/* Idle states parsed correctly, initialize per-cpu pointer */
> +	per_cpu(psci_power_state, cpu) = psci_states;
> +	return 0;
> +}
> +#else
> +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
>  int psci_cpu_init_idle(unsigned int cpu)
>  {
>  	struct device_node *cpu_node;
>  	int ret;
>  
> +	if (!acpi_disabled)
> +		return psci_acpi_cpu_init_idle(cpu);
> +
>  	cpu_node = of_get_cpu_node(cpu, NULL);
>  	if (!cpu_node)
>  		return -ENODEV;

save_ctx notwithstanding the patch is fine, let's define what to
do with that, remainder of the code is ok.

Thanks,
Lorenzo

[PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Lorenzo Pieralisi]

[+ Daniel, Kevin]

On Wed, May 11, 2016 at 04:37:41PM +0100, Sudeep Holla wrote:
> This patch adds appropriate callbacks to support ACPI Low Power Idle
> (LPI) on ARM64.
> 
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-arm-kernel at lists.infradead.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/kernel/acpi.c | 48 +++++++++++++++++++++++++++++++++++++++++
>  drivers/firmware/psci.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 104 insertions(+)
> 
> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c

I think we can add this to arm64/kernel/cpuidle.c so that we have all
arch code managing idle in one place.

> index d1ce8e2f98b9..bf82ce5c8fce 100644
> --- a/arch/arm64/kernel/acpi.c
> +++ b/arch/arm64/kernel/acpi.c
> @@ -18,6 +18,7 @@
>  #include <linux/acpi.h>
>  #include <linux/bootmem.h>
>  #include <linux/cpumask.h>
> +#include <linux/cpu_pm.h>
>  #include <linux/init.h>
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
> @@ -25,6 +26,9 @@
>  #include <linux/of_fdt.h>
>  #include <linux/smp.h>
>  
> +#include <acpi/processor.h>
> +
> +#include <asm/cpuidle.h>
>  #include <asm/cputype.h>
>  #include <asm/cpu_ops.h>
>  #include <asm/smp_plat.h>
> @@ -211,6 +215,50 @@ void __init acpi_boot_table_init(void)
>  	}
>  }
>  
> +int acpi_processor_ffh_lpi_probe(unsigned int cpu)
> +{
> +	return arm_cpuidle_init(cpu);
> +}
> +
> +#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT	BIT(0)
> +#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT	BIT(1)
> +#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT	BIT(2)
> +#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT	BIT(3)
> +#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT	\
> +	(ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT |	\
> +	 ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT |	\
> +	 ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT |	\
> +	 ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT)
> +
> +struct acpi_lpi_state *lpi;
> +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx)
> +{
> +	int ret = 0;
> +	bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT;

I am not really that keen on this, as you know. Those flags are
there to say "save these components registers". I see the CPU PM
notifiers as a way to save/restore CPU peripheral state, but
they should *not* carry out any action that affects the power
state itself, that's down to the suspend finisher (eg PSCI),
because that's where the specific idle states are managed.

I agree we have no clue whatsoever on what we *really* need
to save/restore, but that's orthogonal to what you are solving
here.

See eg gic_cpu_if_down(). Do we call it from the GIC CPU PM notifier ?
No. We should not handle the same problem differently.

On top of that, we have no way to solve this problem for DT,
all I am saying is that it is ill-defined and given that LPI
is new I'd rather we got it right from the beginning.

I am open to suggestions here.

> +
> +	if (!idx) {
> +		cpu_do_idle();
> +		return idx;
> +	}
> +
> +	/* TODO cpu_pm_{enter,exit} can be done in generic code ? */
> +	if (save_ctx)
> +		ret = cpu_pm_enter();
> +	if (!ret) {
> +		/*
> +		 * Pass idle state index to cpu_suspend which in turn will
> +		 * call the CPU ops suspend protocol with idle index as a
> +		 * parameter.
> +		 */
> +		ret = arm_cpuidle_suspend(idx);
> +
> +		if (save_ctx)
> +			cpu_pm_exit();
> +	}
> +
> +	return ret ? -1 : idx;

The body of this function (if we remove save_ctx) is identical
to arm_enter_idle_state(), it would be nice if we found a way
where to put this code and share it with the ARM CPUidle driver,
but I am not too fussed about that either.

> +}
> +
>  #ifdef CONFIG_ACPI_APEI
>  pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)
>  {
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index fa4ea22ca12e..e06bfee68e1d 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -13,6 +13,7 @@
>  
>  #define pr_fmt(fmt) "psci: " fmt
>  
> +#include <linux/acpi.h>
>  #include <linux/arm-smccc.h>
>  #include <linux/cpuidle.h>
>  #include <linux/errno.h>
> @@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>  	return ret;
>  }
>  
> +#ifdef CONFIG_ACPI
> +#include <acpi/processor.h>
> +
> +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
> +{
> +	int i, count;
> +	u32 *psci_states;
> +	struct acpi_processor *pr;
> +	struct acpi_lpi_state *lpi;
> +
> +	pr = per_cpu(processors, cpu);
> +	if (unlikely(!pr || !pr->flags.has_lpi))
> +		return -EINVAL;
> +
> +	/*
> +	 * If the PSCI cpu_suspend function hook has not been initialized
> +	 * idle states must not be enabled, so bail out
> +	 */
> +	if (!psci_ops.cpu_suspend)
> +		return -EOPNOTSUPP;
> +
> +	count = pr->power.count - 1;

Nit: I am not sure this can happen, but you really do not want
count to become == -1.

> +	if (!count)
> +		return -ENODEV;
> +
> +	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> +	if (!psci_states)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < count; i++) {
> +		u32 state;
> +
> +		lpi = &pr->power.lpi_states[i + 1];
> +		state = lpi->address & 0xFFFFFFFF;
> +		if (!psci_power_state_is_valid(state)) {
> +			pr_warn("Invalid PSCI power state %#x\n", state);
> +			kfree(psci_states);
> +			return -EINVAL;
> +		}
> +		psci_states[i] = state;
> +	}
> +	/* Idle states parsed correctly, initialize per-cpu pointer */
> +	per_cpu(psci_power_state, cpu) = psci_states;
> +	return 0;
> +}
> +#else
> +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu)
> +{
> +	return -EINVAL;
> +}
> +#endif
> +
>  int psci_cpu_init_idle(unsigned int cpu)
>  {
>  	struct device_node *cpu_node;
>  	int ret;
>  
> +	if (!acpi_disabled)
> +		return psci_acpi_cpu_init_idle(cpu);
> +
>  	cpu_node = of_get_cpu_node(cpu, NULL);
>  	if (!cpu_node)
>  		return -ENODEV;

save_ctx notwithstanding the patch is fine, let's define what to
do with that, remainder of the code is ok.

Thanks,
Lorenzo

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

[Sudeep Holla]

Hi Rafael,

On 11/05/16 16:37, 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.
>

I have added support for autopromote states(basically skip flattening or
creating composite state). I have also fixed the bug discussed in this
thread with Prashant. Do you have any other feedback on this version
that I incorporate before posting next version.

-- 
Regards,
Sudeep

RE: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Sajjan, Vikas C]

Hi Sudeep,

-----Original Message-----
From: Sudeep Holla [mailto:sudeep.holla@arm.com] 
Sent: Wednesday, May 11, 2016 9:08 PM
To: linux-acpi@vger.kernel.org; Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-kernel@vger.kernel.org; Sajjan, Vikas C <vikas.cha.sajjan@hpe.com>; Lakshminarasimha, Sunil Vishwanathpur <sunil.vl@hpe.com>; Prashanth Prakash <pprakash@codeaurora.org>; Ashwin Chaugule <ashwin.chaugule@linaro.org>; Al Stone <al.stone@linaro.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Mark Rutland <mark.rutland@arm.com>; linux-arm-kernel@lists.infradead.org
Subject: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

This patch adds appropriate callbacks to support ACPI Low Power Idle
(LPI) on ARM64.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/acpi.c | 48 +++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/psci.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index d1ce8e2f98b9..bf82ce5c8fce 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -18,6 +18,7 @@
 #include <linux/acpi.h>
 #include <linux/bootmem.h>
 #include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
 #include <linux/init.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
@@ -25,6 +26,9 @@
 #include <linux/of_fdt.h>
 #include <linux/smp.h>
 
+#include <acpi/processor.h>
+
+#include <asm/cpuidle.h>
 #include <asm/cputype.h>
 #include <asm/cpu_ops.h>
 #include <asm/smp_plat.h>
@@ -211,6 +215,50 @@ void __init acpi_boot_table_init(void)
 	}
 }
 
+int acpi_processor_ffh_lpi_probe(unsigned int cpu) {
+	return arm_cpuidle_init(cpu);
+}
+
+#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT	BIT(0)
+#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT	BIT(1)
+#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT	BIT(2)
+#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT	BIT(3)
+#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT	\
+	(ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT)
+
+struct acpi_lpi_state *lpi;
+int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx) {
+	int ret = 0;
+	bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT;
+
+	if (!idx) {
+		cpu_do_idle();
+		return idx;
+	}
+
+	/* TODO cpu_pm_{enter,exit} can be done in generic code ? */
+	if (save_ctx)
+		ret = cpu_pm_enter();
+	if (!ret) {
+		/*
+		 * Pass idle state index to cpu_suspend which in turn will
+		 * call the CPU ops suspend protocol with idle index as a
+		 * parameter.
+		 */
+		ret = arm_cpuidle_suspend(idx);
+
+		if (save_ctx)
+			cpu_pm_exit();
+	}
+
+	return ret ? -1 : idx;
+}
+
 #ifdef CONFIG_ACPI_APEI
 pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)  { diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index fa4ea22ca12e..e06bfee68e1d 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) "psci: " fmt
 
+#include <linux/acpi.h>
 #include <linux/arm-smccc.h>
 #include <linux/cpuidle.h>
 #include <linux/errno.h>
@@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	return ret;
 }
 
+#ifdef CONFIG_ACPI
+#include <acpi/processor.h>
+
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) {
+	int i, count;
+	u32 *psci_states;
+	struct acpi_processor *pr;
+	struct acpi_lpi_state *lpi;
+
+	pr = per_cpu(processors, cpu);
+	if (unlikely(!pr || !pr->flags.has_lpi))

Any particular reason for _not_ considering CST flag here.
Or you are planning to add CST support in some other patch set.

+		return -EINVAL;
+
+	/*
+	 * If the PSCI cpu_suspend function hook has not been initialized
+	 * idle states must not be enabled, so bail out
+	 */
+	if (!psci_ops.cpu_suspend)
+		return -EOPNOTSUPP;
+
+	count = pr->power.count - 1;
+	if (!count)
+		return -ENODEV;
+
+	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+	if (!psci_states)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		u32 state;
+
+		lpi = &pr->power.lpi_states[i + 1];

  Same case here too.

+		state = lpi->address & 0xFFFFFFFF;
+		if (!psci_power_state_is_valid(state)) {
+			pr_warn("Invalid PSCI power state %#x\n", state);
+			kfree(psci_states);
+			return -EINVAL;
+		}
+		psci_states[i] = state;
+	}
+	/* Idle states parsed correctly, initialize per-cpu pointer */
+	per_cpu(psci_power_state, cpu) = psci_states;
+	return 0;
+}
+#else
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) {
+	return -EINVAL;
+}
+#endif
+
 int psci_cpu_init_idle(unsigned int cpu)  {
 	struct device_node *cpu_node;
 	int ret;
 
+	if (!acpi_disabled)
+		return psci_acpi_cpu_init_idle(cpu);
+
 	cpu_node = of_get_cpu_node(cpu, NULL);
 	if (!cpu_node)
 		return -ENODEV;
--
1.9.1

RE: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Sajjan, Vikas C]

Hi Sudeep,

-----Original Message-----
From: Sudeep Holla [mailto:sudeep.holla@arm.com] 
Sent: Wednesday, May 11, 2016 9:08 PM
To: linux-acpi@vger.kernel.org; Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-kernel@vger.kernel.org; Sajjan, Vikas C <vikas.cha.sajjan@hpe.com>; Lakshminarasimha, Sunil Vishwanathpur <sunil.vl@hpe.com>; Prashanth Prakash <pprakash@codeaurora.org>; Ashwin Chaugule <ashwin.chaugule@linaro.org>; Al Stone <al.stone@linaro.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Mark Rutland <mark.rutland@arm.com>; linux-arm-kernel@lists.infradead.org
Subject: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

This patch adds appropriate callbacks to support ACPI Low Power Idle
(LPI) on ARM64.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/acpi.c | 48 +++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/psci.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index d1ce8e2f98b9..bf82ce5c8fce 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -18,6 +18,7 @@
 #include <linux/acpi.h>
 #include <linux/bootmem.h>
 #include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
 #include <linux/init.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
@@ -25,6 +26,9 @@
 #include <linux/of_fdt.h>
 #include <linux/smp.h>
 
+#include <acpi/processor.h>
+
+#include <asm/cpuidle.h>
 #include <asm/cputype.h>
 #include <asm/cpu_ops.h>
 #include <asm/smp_plat.h>
@@ -211,6 +215,50 @@ void __init acpi_boot_table_init(void)
 	}
 }
 
+int acpi_processor_ffh_lpi_probe(unsigned int cpu) {
+	return arm_cpuidle_init(cpu);
+}
+
+#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT	BIT(0)
+#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT	BIT(1)
+#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT	BIT(2)
+#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT	BIT(3)
+#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT	\
+	(ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT)
+
+struct acpi_lpi_state *lpi;
+int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx) {
+	int ret = 0;
+	bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT;
+
+	if (!idx) {
+		cpu_do_idle();
+		return idx;
+	}
+
+	/* TODO cpu_pm_{enter,exit} can be done in generic code ? */
+	if (save_ctx)
+		ret = cpu_pm_enter();
+	if (!ret) {
+		/*
+		 * Pass idle state index to cpu_suspend which in turn will
+		 * call the CPU ops suspend protocol with idle index as a
+		 * parameter.
+		 */
+		ret = arm_cpuidle_suspend(idx);
+
+		if (save_ctx)
+			cpu_pm_exit();
+	}
+
+	return ret ? -1 : idx;
+}
+
 #ifdef CONFIG_ACPI_APEI
 pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)  { diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index fa4ea22ca12e..e06bfee68e1d 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) "psci: " fmt
 
+#include <linux/acpi.h>
 #include <linux/arm-smccc.h>
 #include <linux/cpuidle.h>
 #include <linux/errno.h>
@@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	return ret;
 }
 
+#ifdef CONFIG_ACPI
+#include <acpi/processor.h>
+
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) {
+	int i, count;
+	u32 *psci_states;
+	struct acpi_processor *pr;
+	struct acpi_lpi_state *lpi;
+
+	pr = per_cpu(processors, cpu);
+	if (unlikely(!pr || !pr->flags.has_lpi))

Any particular reason for _not_ considering CST flag here.
Or you are planning to add CST support in some other patch set.

+		return -EINVAL;
+
+	/*
+	 * If the PSCI cpu_suspend function hook has not been initialized
+	 * idle states must not be enabled, so bail out
+	 */
+	if (!psci_ops.cpu_suspend)
+		return -EOPNOTSUPP;
+
+	count = pr->power.count - 1;
+	if (!count)
+		return -ENODEV;
+
+	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+	if (!psci_states)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		u32 state;
+
+		lpi = &pr->power.lpi_states[i + 1];

  Same case here too.

+		state = lpi->address & 0xFFFFFFFF;
+		if (!psci_power_state_is_valid(state)) {
+			pr_warn("Invalid PSCI power state %#x\n", state);
+			kfree(psci_states);
+			return -EINVAL;
+		}
+		psci_states[i] = state;
+	}
+	/* Idle states parsed correctly, initialize per-cpu pointer */
+	per_cpu(psci_power_state, cpu) = psci_states;
+	return 0;
+}
+#else
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) {
+	return -EINVAL;
+}
+#endif
+
 int psci_cpu_init_idle(unsigned int cpu)  {
 	struct device_node *cpu_node;
 	int ret;
 
+	if (!acpi_disabled)
+		return psci_acpi_cpu_init_idle(cpu);
+
 	cpu_node = of_get_cpu_node(cpu, NULL);
 	if (!cpu_node)
 		return -ENODEV;
--
1.9.1

[PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Sajjan, Vikas C]

Hi Sudeep,

-----Original Message-----
From: Sudeep Holla [mailto:sudeep.holla at arm.com] 
Sent: Wednesday, May 11, 2016 9:08 PM
To: linux-acpi at vger.kernel.org; Rafael J. Wysocki <rjw@rjwysocki.net>
Cc: Sudeep Holla <sudeep.holla@arm.com>; linux-kernel at vger.kernel.org; Sajjan, Vikas C <vikas.cha.sajjan@hpe.com>; Lakshminarasimha, Sunil Vishwanathpur <sunil.vl@hpe.com>; Prashanth Prakash <pprakash@codeaurora.org>; Ashwin Chaugule <ashwin.chaugule@linaro.org>; Al Stone <al.stone@linaro.org>; Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>; Mark Rutland <mark.rutland@arm.com>; linux-arm-kernel at lists.infradead.org
Subject: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

This patch adds appropriate callbacks to support ACPI Low Power Idle
(LPI) on ARM64.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/acpi.c | 48 +++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/psci.c  | 56 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+)

diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c index d1ce8e2f98b9..bf82ce5c8fce 100644
--- a/arch/arm64/kernel/acpi.c
+++ b/arch/arm64/kernel/acpi.c
@@ -18,6 +18,7 @@
 #include <linux/acpi.h>
 #include <linux/bootmem.h>
 #include <linux/cpumask.h>
+#include <linux/cpu_pm.h>
 #include <linux/init.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
@@ -25,6 +26,9 @@
 #include <linux/of_fdt.h>
 #include <linux/smp.h>
 
+#include <acpi/processor.h>
+
+#include <asm/cpuidle.h>
 #include <asm/cputype.h>
 #include <asm/cpu_ops.h>
 #include <asm/smp_plat.h>
@@ -211,6 +215,50 @@ void __init acpi_boot_table_init(void)
 	}
 }
 
+int acpi_processor_ffh_lpi_probe(unsigned int cpu) {
+	return arm_cpuidle_init(cpu);
+}
+
+#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT	BIT(0)
+#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT	BIT(1)
+#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT	BIT(2)
+#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT	BIT(3)
+#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT	\
+	(ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT |	\
+	 ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT)
+
+struct acpi_lpi_state *lpi;
+int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx) {
+	int ret = 0;
+	bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT;
+
+	if (!idx) {
+		cpu_do_idle();
+		return idx;
+	}
+
+	/* TODO cpu_pm_{enter,exit} can be done in generic code ? */
+	if (save_ctx)
+		ret = cpu_pm_enter();
+	if (!ret) {
+		/*
+		 * Pass idle state index to cpu_suspend which in turn will
+		 * call the CPU ops suspend protocol with idle index as a
+		 * parameter.
+		 */
+		ret = arm_cpuidle_suspend(idx);
+
+		if (save_ctx)
+			cpu_pm_exit();
+	}
+
+	return ret ? -1 : idx;
+}
+
 #ifdef CONFIG_ACPI_APEI
 pgprot_t arch_apei_get_mem_attribute(phys_addr_t addr)  { diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c index fa4ea22ca12e..e06bfee68e1d 100644
--- a/drivers/firmware/psci.c
+++ b/drivers/firmware/psci.c
@@ -13,6 +13,7 @@
 
 #define pr_fmt(fmt) "psci: " fmt
 
+#include <linux/acpi.h>
 #include <linux/arm-smccc.h>
 #include <linux/cpuidle.h>
 #include <linux/errno.h>
@@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
 	return ret;
 }
 
+#ifdef CONFIG_ACPI
+#include <acpi/processor.h>
+
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) {
+	int i, count;
+	u32 *psci_states;
+	struct acpi_processor *pr;
+	struct acpi_lpi_state *lpi;
+
+	pr = per_cpu(processors, cpu);
+	if (unlikely(!pr || !pr->flags.has_lpi))

Any particular reason for _not_ considering CST flag here.
Or you are planning to add CST support in some other patch set.

+		return -EINVAL;
+
+	/*
+	 * If the PSCI cpu_suspend function hook has not been initialized
+	 * idle states must not be enabled, so bail out
+	 */
+	if (!psci_ops.cpu_suspend)
+		return -EOPNOTSUPP;
+
+	count = pr->power.count - 1;
+	if (!count)
+		return -ENODEV;
+
+	psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
+	if (!psci_states)
+		return -ENOMEM;
+
+	for (i = 0; i < count; i++) {
+		u32 state;
+
+		lpi = &pr->power.lpi_states[i + 1];

  Same case here too.

+		state = lpi->address & 0xFFFFFFFF;
+		if (!psci_power_state_is_valid(state)) {
+			pr_warn("Invalid PSCI power state %#x\n", state);
+			kfree(psci_states);
+			return -EINVAL;
+		}
+		psci_states[i] = state;
+	}
+	/* Idle states parsed correctly, initialize per-cpu pointer */
+	per_cpu(psci_power_state, cpu) = psci_states;
+	return 0;
+}
+#else
+static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) {
+	return -EINVAL;
+}
+#endif
+
 int psci_cpu_init_idle(unsigned int cpu)  {
 	struct device_node *cpu_node;
 	int ret;
 
+	if (!acpi_disabled)
+		return psci_acpi_cpu_init_idle(cpu);
+
 	cpu_node = of_get_cpu_node(cpu, NULL);
 	if (!cpu_node)
 		return -ENODEV;
--
1.9.1

Re: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Sudeep Holla]

On 13/06/16 05:47, Sajjan, Vikas C wrote:
> Hi Sudeep,
>

[...]

> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -13,6 +13,7 @@
>
>   #define pr_fmt(fmt) "psci: " fmt
>
> +#include <linux/acpi.h>
>   #include <linux/arm-smccc.h>
>   #include <linux/cpuidle.h>
>   #include <linux/errno.h>
> @@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>   	return ret;
>   }
>
> +#ifdef CONFIG_ACPI
> +#include <acpi/processor.h>
> +
> +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) {
> +	int i, count;
> +	u32 *psci_states;
> +	struct acpi_processor *pr;
> +	struct acpi_lpi_state *lpi;
> +
> +	pr = per_cpu(processors, cpu);
> +	if (unlikely(!pr || !pr->flags.has_lpi))
>
> Any particular reason for _not_ considering CST flag here.
> Or you are planning to add CST support in some other patch set.
>

Are you referring the old C-state objects ? We don't support them on
ARM64. Only LPIs are support. The ARM FFH is not defined for CST, only
for LPIs.

-- 
Regards,
Sudeep

Re: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Sudeep Holla]

On 13/06/16 05:47, Sajjan, Vikas C wrote:
> Hi Sudeep,
>

[...]

> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -13,6 +13,7 @@
>
>   #define pr_fmt(fmt) "psci: " fmt
>
> +#include <linux/acpi.h>
>   #include <linux/arm-smccc.h>
>   #include <linux/cpuidle.h>
>   #include <linux/errno.h>
> @@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>   	return ret;
>   }
>
> +#ifdef CONFIG_ACPI
> +#include <acpi/processor.h>
> +
> +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) {
> +	int i, count;
> +	u32 *psci_states;
> +	struct acpi_processor *pr;
> +	struct acpi_lpi_state *lpi;
> +
> +	pr = per_cpu(processors, cpu);
> +	if (unlikely(!pr || !pr->flags.has_lpi))
>
> Any particular reason for _not_ considering CST flag here.
> Or you are planning to add CST support in some other patch set.
>

Are you referring the old C-state objects ? We don't support them on
ARM64. Only LPIs are support. The ARM FFH is not defined for CST, only
for LPIs.

-- 
Regards,
Sudeep

[PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Sudeep Holla]

On 13/06/16 05:47, Sajjan, Vikas C wrote:
> Hi Sudeep,
>

[...]

> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -13,6 +13,7 @@
>
>   #define pr_fmt(fmt) "psci: " fmt
>
> +#include <linux/acpi.h>
>   #include <linux/arm-smccc.h>
>   #include <linux/cpuidle.h>
>   #include <linux/errno.h>
> @@ -310,11 +311,66 @@ static int psci_dt_cpu_init_idle(struct device_node *cpu_node, int cpu)
>   	return ret;
>   }
>
> +#ifdef CONFIG_ACPI
> +#include <acpi/processor.h>
> +
> +static int __maybe_unused psci_acpi_cpu_init_idle(unsigned int cpu) {
> +	int i, count;
> +	u32 *psci_states;
> +	struct acpi_processor *pr;
> +	struct acpi_lpi_state *lpi;
> +
> +	pr = per_cpu(processors, cpu);
> +	if (unlikely(!pr || !pr->flags.has_lpi))
>
> Any particular reason for _not_ considering CST flag here.
> Or you are planning to add CST support in some other patch set.
>

Are you referring the old C-state objects ? We don't support them on
ARM64. Only LPIs are support. The ARM FFH is not defined for CST, only
for LPIs.

-- 
Regards,
Sudeep

Re: [PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Daniel Lezcano]

On Fri, Jun 10, 2016 at 01:50:28PM +0100, Lorenzo Pieralisi wrote:
> [+ Daniel, Kevin]
> 
> On Wed, May 11, 2016 at 04:37:41PM +0100, Sudeep Holla wrote:
> > This patch adds appropriate callbacks to support ACPI Low Power Idle
> > (LPI) on ARM64.
> > 
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---

[ ... ]

> > +#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT	BIT(0)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT	BIT(1)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT	BIT(2)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT	BIT(3)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT	\
> > +	(ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT |	\
> > +	 ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT |	\
> > +	 ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT |	\
> > +	 ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT)
> > +
> > +struct acpi_lpi_state *lpi;
> > +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx)
> > +{
> > +	int ret = 0;
> > +	bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT;
> 
> I am not really that keen on this, as you know. Those flags are
> there to say "save these components registers". I see the CPU PM
> notifiers as a way to save/restore CPU peripheral state, but
> they should *not* carry out any action that affects the power
> state itself, that's down to the suspend finisher (eg PSCI),
> because that's where the specific idle states are managed.
> 
> I agree we have no clue whatsoever on what we *really* need
> to save/restore, but that's orthogonal to what you are solving
> here.
> 
> See eg gic_cpu_if_down(). Do we call it from the GIC CPU PM notifier ?
> No. We should not handle the same problem differently.
> 
> On top of that, we have no way to solve this problem for DT,
> all I am saying is that it is ill-defined and given that LPI
> is new I'd rather we got it right from the beginning.
> 
> I am open to suggestions here.

There is a part of the idle state flags integer which is reserved for the 
arch specific flag and can be masked with:

	CPUIDLE_DRIVER_FLAGS_MASK()

May be these context flags can be added in the generic cpuidle driver and 
reused.

Concerning the DT, why not use the power domains to tell which context to 
save ? yeah, probably mentionned n-th times :)

> > +
> > +	if (!idx) {
> > +		cpu_do_idle();
> > +		return idx;
> > +	}
> > +
> > +	/* TODO cpu_pm_{enter,exit} can be done in generic code ? */
> > +	if (save_ctx)
> > +		ret = cpu_pm_enter();
> > +	if (!ret) {
> > +		/*
> > +		 * Pass idle state index to cpu_suspend which in turn will
> > +		 * call the CPU ops suspend protocol with idle index as a
> > +		 * parameter.
> > +		 */
> > +		ret = arm_cpuidle_suspend(idx);
> > +
> > +		if (save_ctx)
> > +			cpu_pm_exit();
> > +	}
> > +
> > +	return ret ? -1 : idx;
> 
> The body of this function (if we remove save_ctx) is identical
> to arm_enter_idle_state(), it would be nice if we found a way
> where to put this code and share it with the ARM CPUidle driver,

+1

We don't want to redo another unmaintable acpi idle driver.

  -- Daniel

[PATCH v5 4/5] arm64: add support for ACPI Low Power Idle(LPI)

[Daniel Lezcano]

On Fri, Jun 10, 2016 at 01:50:28PM +0100, Lorenzo Pieralisi wrote:
> [+ Daniel, Kevin]
> 
> On Wed, May 11, 2016 at 04:37:41PM +0100, Sudeep Holla wrote:
> > This patch adds appropriate callbacks to support ACPI Low Power Idle
> > (LPI) on ARM64.
> > 
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: linux-arm-kernel at lists.infradead.org
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---

[ ... ]

> > +#define ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT	BIT(0)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT	BIT(1)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT	BIT(2)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT	BIT(3)
> > +#define ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT	\
> > +	(ACPI_FFH_LPI_ARM_FLAGS_CORE_CONTEXT |	\
> > +	 ACPI_FFH_LPI_ARM_FLAGS_TRACE_CONTEXT |	\
> > +	 ACPI_FFH_LPI_ARM_FLAGS_GICR_CONTEXT |	\
> > +	 ACPI_FFH_LPI_ARM_FLAGS_GICD_CONTEXT)
> > +
> > +struct acpi_lpi_state *lpi;
> > +int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi, int idx)
> > +{
> > +	int ret = 0;
> > +	bool save_ctx = lpi->arch_flags & ACPI_FFH_LPI_ARM_FLAGS_ALL_CONTEXT;
> 
> I am not really that keen on this, as you know. Those flags are
> there to say "save these components registers". I see the CPU PM
> notifiers as a way to save/restore CPU peripheral state, but
> they should *not* carry out any action that affects the power
> state itself, that's down to the suspend finisher (eg PSCI),
> because that's where the specific idle states are managed.
> 
> I agree we have no clue whatsoever on what we *really* need
> to save/restore, but that's orthogonal to what you are solving
> here.
> 
> See eg gic_cpu_if_down(). Do we call it from the GIC CPU PM notifier ?
> No. We should not handle the same problem differently.
> 
> On top of that, we have no way to solve this problem for DT,
> all I am saying is that it is ill-defined and given that LPI
> is new I'd rather we got it right from the beginning.
> 
> I am open to suggestions here.

There is a part of the idle state flags integer which is reserved for the 
arch specific flag and can be masked with:

	CPUIDLE_DRIVER_FLAGS_MASK()

May be these context flags can be added in the generic cpuidle driver and 
reused.

Concerning the DT, why not use the power domains to tell which context to 
save ? yeah, probably mentionned n-th times :)

> > +
> > +	if (!idx) {
> > +		cpu_do_idle();
> > +		return idx;
> > +	}
> > +
> > +	/* TODO cpu_pm_{enter,exit} can be done in generic code ? */
> > +	if (save_ctx)
> > +		ret = cpu_pm_enter();
> > +	if (!ret) {
> > +		/*
> > +		 * Pass idle state index to cpu_suspend which in turn will
> > +		 * call the CPU ops suspend protocol with idle index as a
> > +		 * parameter.
> > +		 */
> > +		ret = arm_cpuidle_suspend(idx);
> > +
> > +		if (save_ctx)
> > +			cpu_pm_exit();
> > +	}
> > +
> > +	return ret ? -1 : idx;
> 
> The body of this function (if we remove save_ctx) is identical
> to arm_enter_idle_state(), it would be nice if we found a way
> where to put this code and share it with the ARM CPUidle driver,

+1

We don't want to redo another unmaintable acpi idle driver.

  -- Daniel

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

[Rafael J. Wysocki]

On Friday, June 10, 2016 06:38:01 PM Sudeep Holla wrote:
> Hi Rafael,

Hi,
 
> On 11/05/16 16:37, 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.
> >
> 
> I have added support for autopromote states(basically skip flattening or
> creating composite state). I have also fixed the bug discussed in this
> thread with Prashant. Do you have any other feedback on this version
> that I incorporate before posting next version.

I'd really preferred it if you posted the next version without waiting for
my feedback to the previous one (as the feedback may not be relevant any
more among other things).

Thanks,
Rafael

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

[Sudeep Holla]

On 13/06/16 22:05, Rafael J. Wysocki wrote:
> On Friday, June 10, 2016 06:38:01 PM Sudeep Holla wrote:
>> Hi Rafael,
>
> Hi,
>
>> On 11/05/16 16:37, 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.
>>>
>>
>> I have added support for autopromote states(basically skip flattening or
>> creating composite state). I have also fixed the bug discussed in this
>> thread with Prashant. Do you have any other feedback on this version
>> that I incorporate before posting next version.
>
> I'd really preferred it if you posted the next version without waiting for
> my feedback to the previous one (as the feedback may not be relevant any
> more among other things).
>

Sure, will do that soon.

-- 
Regards,
Sudeep