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

[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 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 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

[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

[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

[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)

[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