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

[PATCH v6 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 e11857fce05f..06786fdaadeb 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;
 
-- 
2.7.4

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

Now that arm_enter_idle_state is exactly same in both generic ARM{32,64}
CPUIdle driver and ARM64 backend for ACPI processor idle driver, we can
unify it and move to cpuidle-arm.h header.

Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: linux-arm-kernel at lists.infradead.org
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 arch/arm64/kernel/cpuidle.c   | 17 +++++++++++++
 drivers/cpuidle/cpuidle-arm.c | 23 ++----------------
 drivers/firmware/psci.c       | 56 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/cpuidle-arm.h   | 30 +++++++++++++++++++++++
 4 files changed, 105 insertions(+), 21 deletions(-)
 create mode 100644 include/linux/cpuidle-arm.h

diff --git a/arch/arm64/kernel/cpuidle.c b/arch/arm64/kernel/cpuidle.c
index 06786fdaadeb..6874d01531ae 100644
--- a/arch/arm64/kernel/cpuidle.c
+++ b/arch/arm64/kernel/cpuidle.c
@@ -9,6 +9,8 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/acpi.h>
+#include <linux/cpuidle-arm.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 
@@ -39,3 +41,18 @@ int arm_cpuidle_suspend(int index)
 
 	return cpu_ops[cpu]->cpu_suspend(index);
 }
+
+#ifdef CONFIG_ACPI
+
+#include <acpi/processor.h>
+
+int acpi_processor_ffh_lpi_probe(unsigned int cpu)
+{
+	return arm_cpuidle_init(cpu);
+}
+
+int acpi_processor_ffh_lpi_enter(struct acpi_lpi_state *lpi)
+{
+	return arm_generic_enter_idle_state(lpi->index);
+}
+#endif
diff --git a/drivers/cpuidle/cpuidle-arm.c b/drivers/cpuidle/cpuidle-arm.c
index e342565e8715..75178ba4a1b2 100644
--- a/drivers/cpuidle/cpuidle-arm.c
+++ b/drivers/cpuidle/cpuidle-arm.c
@@ -12,8 +12,8 @@
 #define pr_fmt(fmt) "CPUidle arm: " fmt
 
 #include <linux/cpuidle.h>
+#include <linux/cpuidle-arm.h>
 #include <linux/cpumask.h>
-#include <linux/cpu_pm.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
@@ -36,26 +36,7 @@
 static int arm_enter_idle_state(struct cpuidle_device *dev,
 				struct cpuidle_driver *drv, int idx)
 {
-	int ret;
-
-	if (!idx) {
-		cpu_do_idle();
-		return idx;
-	}
-
-	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);
-
-		cpu_pm_exit();
-	}
-
-	return ret ? -1 : idx;
+	return arm_generic_enter_idle_state(idx);
 }
 
 static struct cpuidle_driver arm_idle_driver = {
diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
index 03e04582791c..c6caa863d156 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 <= 0)
+		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;
diff --git a/include/linux/cpuidle-arm.h b/include/linux/cpuidle-arm.h
new file mode 100644
index 000000000000..b99bcb3f43dd
--- /dev/null
+++ b/include/linux/cpuidle-arm.h
@@ -0,0 +1,30 @@
+#include <linux/cpu_pm.h>
+
+#include <asm/cpuidle.h>
+
+/*
+ * arm_enter_idle_state - Programs CPU to enter the specified state
+ */
+static int arm_generic_enter_idle_state(int idx)
+{
+	int ret;
+
+	if (!idx) {
+		cpu_do_idle();
+		return idx;
+	}
+
+	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);
+
+		cpu_pm_exit();
+	}
+
+	return ret ? -1 : idx;
+}
-- 
2.7.4

[PATCH v6 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
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
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 1358fb7d7a68..d74275c0f374 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
-- 
2.7.4

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

[Lorenzo Pieralisi]

Hi Sudeep,

On Tue, Jun 14, 2016 at 03:48:38PM +0100, Sudeep Holla wrote:
> This patch adds appropriate callbacks to support ACPI Low Power Idle
> (LPI) on ARM64.
> 
> Now that arm_enter_idle_state is exactly same in both generic ARM{32,64}
> CPUIdle driver and ARM64 backend for ACPI processor idle driver, we can
> unify it and move to cpuidle-arm.h header.
> 
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: linux-arm-kernel at lists.infradead.org
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  arch/arm64/kernel/cpuidle.c   | 17 +++++++++++++
>  drivers/cpuidle/cpuidle-arm.c | 23 ++----------------
>  drivers/firmware/psci.c       | 56 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/cpuidle-arm.h   | 30 +++++++++++++++++++++++
>  4 files changed, 105 insertions(+), 21 deletions(-)
>  create mode 100644 include/linux/cpuidle-arm.h

This patch seems fine by me, it would be good if Daniel can have
a look too.

Some minor comments below.

[...]

> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index 03e04582791c..c6caa863d156 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 <= 0)
> +		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;

Most of the code in this function is FW independent, it would be nice
to factor it out and add the respective ACPI/DT helper functions to
retrieve idle states count and parameters, we can update it later
anyway it is fine by me to leave it as it is.

> +}
> +#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;
> diff --git a/include/linux/cpuidle-arm.h b/include/linux/cpuidle-arm.h
> new file mode 100644
> index 000000000000..b99bcb3f43dd
> --- /dev/null
> +++ b/include/linux/cpuidle-arm.h

arm-cpuidle.h for consistency with other (ARM) include/linux files ?

> @@ -0,0 +1,30 @@
> +#include <linux/cpu_pm.h>
> +
> +#include <asm/cpuidle.h>
> +
> +/*
> + * arm_enter_idle_state - Programs CPU to enter the specified state
> + */
> +static int arm_generic_enter_idle_state(int idx)
> +{
> +	int ret;
> +
> +	if (!idx) {
> +		cpu_do_idle();
> +		return idx;
> +	}
> +
> +	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);
> +
> +		cpu_pm_exit();
> +	}
> +
> +	return ret ? -1 : idx;
> +}

Either you do this, or we have to add it somehow somewhere in
drivers/cpuidle to avoid duplicating it.

@Daniel: do you have an opinion on this please ?

Thanks,
Lorenzo

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

[Lorenzo Pieralisi]

On Tue, Jun 14, 2016 at 03:48:37PM +0100, Sudeep Holla wrote:
> 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>

Depending on acceptance of other patches in this series:

Acked-by: Lorenzo Pieralisi <lorenzo.pieralisi@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 e11857fce05f..06786fdaadeb 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;
>  
> -- 
> 2.7.4
> 

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

[Daniel Lezcano]

On 06/22/2016 04:17 PM, Lorenzo Pieralisi wrote:
> Hi Sudeep,
>
> On Tue, Jun 14, 2016 at 03:48:38PM +0100, Sudeep Holla wrote:
>> This patch adds appropriate callbacks to support ACPI Low Power Idle
>> (LPI) on ARM64.
>>
>> Now that arm_enter_idle_state is exactly same in both generic ARM{32,64}
>> CPUIdle driver and ARM64 backend for ACPI processor idle driver, we can
>> unify it and move to cpuidle-arm.h header.
>>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: linux-arm-kernel at lists.infradead.org
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>   arch/arm64/kernel/cpuidle.c   | 17 +++++++++++++
>>   drivers/cpuidle/cpuidle-arm.c | 23 ++----------------
>>   drivers/firmware/psci.c       | 56 +++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/cpuidle-arm.h   | 30 +++++++++++++++++++++++
>>   4 files changed, 105 insertions(+), 21 deletions(-)
>>   create mode 100644 include/linux/cpuidle-arm.h
>
> This patch seems fine by me, it would be good if Daniel can have
> a look too.
>
> Some minor comments below.
>
> [...]
>
>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>> index 03e04582791c..c6caa863d156 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 <= 0)
>> +		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;

Why is needed to mask 'address' ?

>> +		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;
>
> Most of the code in this function is FW independent, it would be nice
> to factor it out and add the respective ACPI/DT helper functions to
> retrieve idle states count and parameters, we can update it later
> anyway it is fine by me to leave it as it is.
>
>> +}
>> +#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);

Is it possible the case where there is information in both the DT and in 
ACPI ? So ACPI is enabled without idle information which is in the DT ?

>>   	cpu_node = of_get_cpu_node(cpu, NULL);
>>   	if (!cpu_node)
>>   		return -ENODEV;
>> diff --git a/include/linux/cpuidle-arm.h b/include/linux/cpuidle-arm.h
>> new file mode 100644
>> index 000000000000..b99bcb3f43dd
>> --- /dev/null
>> +++ b/include/linux/cpuidle-arm.h
>
> arm-cpuidle.h for consistency with other (ARM) include/linux files ?
>
>> @@ -0,0 +1,30 @@
>> +#include <linux/cpu_pm.h>
>> +
>> +#include <asm/cpuidle.h>
>> +
>> +/*
>> + * arm_enter_idle_state - Programs CPU to enter the specified state
>> + */
>> +static int arm_generic_enter_idle_state(int idx)
>> +{
>> +	int ret;
>> +
>> +	if (!idx) {
>> +		cpu_do_idle();
>> +		return idx;
>> +	}
>> +
>> +	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);
>> +
>> +		cpu_pm_exit();
>> +	}
>> +
>> +	return ret ? -1 : idx;
>> +}
>
> Either you do this, or we have to add it somehow somewhere in
> drivers/cpuidle to avoid duplicating it.
>
> @Daniel: do you have an opinion on this please ?

Yes, this function should be added to avoid duplication.

  -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Rafael J. Wysocki]

On Friday, June 24, 2016 11:04:07 PM Daniel Lezcano wrote:
> On 06/22/2016 04:17 PM, Lorenzo Pieralisi wrote:
> > Hi Sudeep,
> >
> > On Tue, Jun 14, 2016 at 03:48:38PM +0100, Sudeep Holla wrote:
> >> This patch adds appropriate callbacks to support ACPI Low Power Idle
> >> (LPI) on ARM64.
> >>
> >> Now that arm_enter_idle_state is exactly same in both generic ARM{32,64}
> >> CPUIdle driver and ARM64 backend for ACPI processor idle driver, we can
> >> unify it and move to cpuidle-arm.h header.
> >>
> >> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >> Cc: Mark Rutland <mark.rutland@arm.com>
> >> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >> Cc: linux-arm-kernel at lists.infradead.org
> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> >> ---
> >>   arch/arm64/kernel/cpuidle.c   | 17 +++++++++++++
> >>   drivers/cpuidle/cpuidle-arm.c | 23 ++----------------
> >>   drivers/firmware/psci.c       | 56 +++++++++++++++++++++++++++++++++++++++++++
> >>   include/linux/cpuidle-arm.h   | 30 +++++++++++++++++++++++
> >>   4 files changed, 105 insertions(+), 21 deletions(-)
> >>   create mode 100644 include/linux/cpuidle-arm.h
> >
> > This patch seems fine by me, it would be good if Daniel can have
> > a look too.
> >
> > Some minor comments below.
> >
> > [...]
> >
> >> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> >> index 03e04582791c..c6caa863d156 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 <= 0)
> >> +		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;
> 
> Why is needed to mask 'address' ?
> 
> >> +		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;
> >
> > Most of the code in this function is FW independent, it would be nice
> > to factor it out and add the respective ACPI/DT helper functions to
> > retrieve idle states count and parameters, we can update it later
> > anyway it is fine by me to leave it as it is.
> >
> >> +}
> >> +#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);
> 
> Is it possible the case where there is information in both the DT and in 
> ACPI ?

No, it isn't.

It is either-or, never both at the same time.

Thanks,
Rafael

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

[Daniel Lezcano]

On 06/25/2016 12:47 AM, Rafael J. Wysocki wrote:

[ ... ]

>>>> +	if (!acpi_disabled)
>>>> +		return psci_acpi_cpu_init_idle(cpu);
>>
>> Is it possible the case where there is information in both the DT and in
>> ACPI ?
>
> No, it isn't.
>
> It is either-or, never both at the same time.

Ok, thanks for the clarification.

   -- Daniel


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Sudeep Holla]

Hi, Daniel,

On 24/06/16 22:04, Daniel Lezcano wrote:

[...]

>>> +
>>> +    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;
>
> Why is needed to mask 'address' ?
>

This is as per Section 3.1.1 FFH Usage in LPI state entry methods in [1]

[...]

>>>   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);
>
> Is it possible the case where there is information in both the DT and in
> ACPI ? So ACPI is enabled without idle information which is in the DT ?
>

No, as Rafael mentioned aready.

>>
>> Either you do this, or we have to add it somehow somewhere in
>> drivers/cpuidle to avoid duplicating it.
>>
>> @Daniel: do you have an opinion on this please ?
>
> Yes, this function should be added to avoid duplication.
>

So, I assume you are happy with the way it's handled in this patch ?
(I will rename the file as suggested by Lorenzo)

-- 
Regards,
Sudeep

[1] 
http://infocenter.arm.com/help/topic/com.arm.doc.den0048a/DEN0048A_ARM_FFH_Specification.pdf

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

[Daniel Lezcano]

On 06/14/2016 04:48 PM, Sudeep Holla wrote:
> 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
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---

Hi Sudeep,

now that ACPI processor supports ARM64 did you check the 
CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?

I deleted the patch 2/5 but there is a place where:

if (max_cstate=0)
	max_cstate=1;

Probably this is because the POLL state is inserted, so there is always 
an idle state. But for ARM, that is not the case.

Also, there are some places where the idle state index begins to 1. I 
think it should be 0 for ARM.

>   drivers/acpi/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index 1358fb7d7a68..d74275c0f374 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
>


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Sudeep Holla]

Hi Daniel,

On 27/06/16 15:33, Daniel Lezcano wrote:
> On 06/14/2016 04:48 PM, Sudeep Holla wrote:
>> 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
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>
> Hi Sudeep,
>
> now that ACPI processor supports ARM64 did you check the
> CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?
>

No, that is used only for C-State and ARM64 doesn't support it.
Patch 1/5 puts all the C-State code under #ifdef so that it's not
compiled on ARM64.

> I deleted the patch 2/5 but there is a place where:
>

Sorry, I don't follow what you mean by that.

> if (max_cstate=0)
>      max_cstate=1;
>
> Probably this is because the POLL state is inserted, so there is always
> an idle state. But for ARM, that is not the case.
>

Yes

> Also, there are some places where the idle state index begins to 1. I
> think it should be 0 for ARM.
>

Yes for LPI, it does start from 0.

-- 
Regards,
Sudeep

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

[Daniel Lezcano]

On 06/27/2016 05:03 PM, Sudeep Holla wrote:
> Hi Daniel,
>
> On 27/06/16 15:33, Daniel Lezcano wrote:
>> On 06/14/2016 04:48 PM, Sudeep Holla wrote:
>>> 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
>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>
>> Hi Sudeep,
>>
>> now that ACPI processor supports ARM64 did you check the
>> CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?
>>
>
> No, that is used only for C-State and ARM64 doesn't support it.
> Patch 1/5 puts all the C-State code under #ifdef so that it's not
> compiled on ARM64.
>
>> I deleted the patch 2/5 but there is a place where:
>>
>
> Sorry, I don't follow what you mean by that.

I meant I just deleted from my mailbox the patch 2/5, so I can't do 
inline comment.

>> if (max_cstate=0)
>>      max_cstate=1;
>>
>> Probably this is because the POLL state is inserted, so there is always
>> an idle state. But for ARM, that is not the case.
>>
>
> Yes
>
>> Also, there are some places where the idle state index begins to 1. I
>> think it should be 0 for ARM.
>>
>
> Yes for LPI, it does start from 0.
>


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Sudeep Holla]

On 27/06/16 16:05, Daniel Lezcano wrote:
> On 06/27/2016 05:03 PM, Sudeep Holla wrote:
>> Hi Daniel,
>>
>> On 27/06/16 15:33, Daniel Lezcano wrote:
>>> On 06/14/2016 04:48 PM, Sudeep Holla wrote:
>>>> 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
>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>> ---
>>>
>>> Hi Sudeep,
>>>
>>> now that ACPI processor supports ARM64 did you check the
>>> CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?
>>>
>>
>> No, that is used only for C-State and ARM64 doesn't support it.
>> Patch 1/5 puts all the C-State code under #ifdef so that it's not
>> compiled on ARM64.
>>
>>> I deleted the patch 2/5 but there is a place where:
>>>
>>
>> Sorry, I don't follow what you mean by that.
>
> I meant I just deleted from my mailbox the patch 2/5, so I can't do
> inline comment.
>

Ah ok, anyways LPI always starts from index 0. IIUC that was your main
concern.

-- 
Regards,
Sudeep

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

[Daniel Lezcano]

On 06/27/2016 05:06 PM, Sudeep Holla wrote:
>
>
> On 27/06/16 16:05, Daniel Lezcano wrote:
>> On 06/27/2016 05:03 PM, Sudeep Holla wrote:
>>> Hi Daniel,
>>>
>>> On 27/06/16 15:33, Daniel Lezcano wrote:
>>>> On 06/14/2016 04:48 PM, Sudeep Holla wrote:
>>>>> 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
>>>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>>>> ---
>>>>
>>>> Hi Sudeep,
>>>>
>>>> now that ACPI processor supports ARM64 did you check the
>>>> CPUIDLE_DRIVER_STATE_START trick in the code and its derivative ?
>>>>
>>>
>>> No, that is used only for C-State and ARM64 doesn't support it.
>>> Patch 1/5 puts all the C-State code under #ifdef so that it's not
>>> compiled on ARM64.
>>>
>>>> I deleted the patch 2/5 but there is a place where:
>>>>
>>>
>>> Sorry, I don't follow what you mean by that.
>>
>> I meant I just deleted from my mailbox the patch 2/5, so I can't do
>> inline comment.
>>
>
> Ah ok, anyways LPI always starts from index 0. IIUC that was your main
> concern.

Do you have a repo where I can see the code ?

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

[Sudeep Holla]

On 27/06/16 16:08, Daniel Lezcano wrote:
> On 06/27/2016 05:06 PM, Sudeep Holla wrote:

[...]

>> Ah ok, anyways LPI always starts from index 0. IIUC that was your main
>> concern.
>
> Do you have a repo where I can see the code ?
>

Yes, you can get it from [1]

--
Regards,
Sudeep

[1] http://git.kernel.org/sudeep.holla/linux/h/for_review/arm64_lpi

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

[Daniel Lezcano]

On 06/27/2016 05:11 PM, Sudeep Holla wrote:
>
>
> On 27/06/16 16:08, Daniel Lezcano wrote:
>> On 06/27/2016 05:06 PM, Sudeep Holla wrote:
>
> [...]
>
>>> Ah ok, anyways LPI always starts from index 0. IIUC that was your main
>>> concern.
>>
>> Do you have a repo where I can see the code ?
>>
>
> Yes, you can get it from [1]

Great. Thanks Sudeep !

> Regards,
> Sudeep
>
> [1] http://git.kernel.org/sudeep.holla/linux/h/for_review/arm64_lpi


-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Daniel Lezcano]

On 06/22/2016 04:17 PM, Lorenzo Pieralisi wrote:
> Hi Sudeep,
>
> On Tue, Jun 14, 2016 at 03:48:38PM +0100, Sudeep Holla wrote:
>> This patch adds appropriate callbacks to support ACPI Low Power Idle
>> (LPI) on ARM64.
>>
>> Now that arm_enter_idle_state is exactly same in both generic ARM{32,64}
>> CPUIdle driver and ARM64 backend for ACPI processor idle driver, we can
>> unify it and move to cpuidle-arm.h header.
>>
>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>> Cc: linux-arm-kernel at lists.infradead.org
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>   arch/arm64/kernel/cpuidle.c   | 17 +++++++++++++
>>   drivers/cpuidle/cpuidle-arm.c | 23 ++----------------
>>   drivers/firmware/psci.c       | 56 +++++++++++++++++++++++++++++++++++++++++++
>>   include/linux/cpuidle-arm.h   | 30 +++++++++++++++++++++++
>>   4 files changed, 105 insertions(+), 21 deletions(-)
>>   create mode 100644 include/linux/cpuidle-arm.h
>
> This patch seems fine by me, it would be good if Daniel can have
> a look too.
>
> Some minor comments below.
>
> [...]
>
>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>> index 03e04582791c..c6caa863d156 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 <= 0)
>> +		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;

Why mask 'address' if 'state' is u32 ?

>> +		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;

The ACPI and the PSCI code are not self contained here.

It would be nice to move this function to the ACPI code.

>> +}
>> +#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);
>> +

acpi_disabled - acpi_disabled - acpi_disabled everywhere :/

The enable-method approach is not straightforward and now it is polluted 
by acpi-disabled.

So IIUC,

smp_init_cpus (contains acpi_disabled)
   smp_cpu_setup
     cpu_read_ops
       cpu_read_enable_method (contains acpi_disabled)
         acpi_get_enable_method (returns 'psci' after checking 
psci_is_present)

Then psci_cpu_init_idle is called... and check again acpi_disabled.

IMO, the circumlocution with the psci vs acpi vs acpi_disabled is 
getting unnecessary too complex, is prone to error and will lead to 
unmaintainable code very soon.

I suggest to sort out encapsulation and self-contained code before 
adding more feature in this area.


>>   	cpu_node = of_get_cpu_node(cpu, NULL);
>>   	if (!cpu_node)
>>   		return -ENODEV;
>> diff --git a/include/linux/cpuidle-arm.h b/include/linux/cpuidle-arm.h
>> new file mode 100644
>> index 000000000000..b99bcb3f43dd
>> --- /dev/null
>> +++ b/include/linux/cpuidle-arm.h
>
> arm-cpuidle.h for consistency with other (ARM) include/linux files ?
>
>> @@ -0,0 +1,30 @@
>> +#include <linux/cpu_pm.h>
>> +
>> +#include <asm/cpuidle.h>
>> +
>> +/*
>> + * arm_enter_idle_state - Programs CPU to enter the specified state
>> + */
>> +static int arm_generic_enter_idle_state(int idx)
>> +{
>> +	int ret;
>> +
>> +	if (!idx) {
>> +		cpu_do_idle();
>> +		return idx;
>> +	}
>> +
>> +	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);
>> +
>> +		cpu_pm_exit();
>> +	}
>> +
>> +	return ret ? -1 : idx;
>> +}
>
> Either you do this, or we have to add it somehow somewhere in
> drivers/cpuidle to avoid duplicating it.
>
> @Daniel: do you have an opinion on this please ?

I don't like the idea to add an ARM arch specific header in 
include/linux. I thought this directory was supposed to contain as much 
as possible arch agnostic headers.

May be the name can be changed to something more generic:

eg.

int cpuidle_generic_enter(int idx);

and then add an option:

HAVE_CPUIDLE_GENERIC_ENTER

, then in the generic header:

#ifdef HAVE_CPUIDLE_GENERIC_ENTER
int cpuidle_generic_enter(int idx);
#endif

, change the function name in cpuidle-arm .c

and finally add in the ARM and ARM64 Kconfig's option 
HAVE_CPUIDLE_GENERIC_ENTER.


   -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Sudeep Holla]

On 27/06/16 17:29, Daniel Lezcano wrote:
> On 06/22/2016 04:17 PM, Lorenzo Pieralisi wrote:
>> Hi Sudeep,
>>
>> On Tue, Jun 14, 2016 at 03:48:38PM +0100, Sudeep Holla wrote:
>>> This patch adds appropriate callbacks to support ACPI Low Power Idle
>>> (LPI) on ARM64.
>>>
>>> Now that arm_enter_idle_state is exactly same in both generic ARM{32,64}
>>> CPUIdle driver and ARM64 backend for ACPI processor idle driver, we can
>>> unify it and move to cpuidle-arm.h header.
>>>
>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
>>> Cc: linux-arm-kernel at lists.infradead.org
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>>   arch/arm64/kernel/cpuidle.c   | 17 +++++++++++++
>>>   drivers/cpuidle/cpuidle-arm.c | 23 ++----------------
>>>   drivers/firmware/psci.c       | 56
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/cpuidle-arm.h   | 30 +++++++++++++++++++++++
>>>   4 files changed, 105 insertions(+), 21 deletions(-)
>>>   create mode 100644 include/linux/cpuidle-arm.h
>>
>> This patch seems fine by me, it would be good if Daniel can have
>> a look too.
>>
>> Some minor comments below.
>>
>> [...]
>>
>>> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
>>> index 03e04582791c..c6caa863d156 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 <= 0)
>>> +        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;
>
> Why mask 'address' if 'state' is u32 ?
>

Agreed, I overlooked it.

>>> +        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;
>
> The ACPI and the PSCI code are not self contained here.
>
> It would be nice to move this function to the ACPI code.
>
>>> +}
>>> +#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);
>>> +
>
> acpi_disabled - acpi_disabled - acpi_disabled everywhere :/
>
> The enable-method approach is not straightforward and now it is polluted
> by acpi-disabled.
>
> So IIUC,
>
> smp_init_cpus (contains acpi_disabled)
>    smp_cpu_setup
>      cpu_read_ops
>        cpu_read_enable_method (contains acpi_disabled)
>          acpi_get_enable_method (returns 'psci' after checking
> psci_is_present)
>
> Then psci_cpu_init_idle is called... and check again acpi_disabled.
>
> IMO, the circumlocution with the psci vs acpi vs acpi_disabled is
> getting unnecessary too complex, is prone to error and will lead to
> unmaintainable code very soon.
>
> I suggest to sort out encapsulation and self-contained code before
> adding more feature in this area.
>

I understand your concern but I have no idea on how to clean up. Lorenzo
asked to factor our common code between psci_{dt,acpi}_cpu_init_idle and
I think you might not like the refactoring[1]. I didn't want to change
cpuidle_ops and hence psci_dt_cpu_init_idle parameters. I will see if
changing that simplifies things.

>>>       cpu_node = of_get_cpu_node(cpu, NULL);
>>>       if (!cpu_node)
>>>           return -ENODEV;
>>> diff --git a/include/linux/cpuidle-arm.h b/include/linux/cpuidle-arm.h
>>> new file mode 100644
>>> index 000000000000..b99bcb3f43dd
>>> --- /dev/null
>>> +++ b/include/linux/cpuidle-arm.h
>>
>> arm-cpuidle.h for consistency with other (ARM) include/linux files ?
>>
>>> @@ -0,0 +1,30 @@
>>> +#include <linux/cpu_pm.h>
>>> +
>>> +#include <asm/cpuidle.h>
>>> +
>>> +/*
>>> + * arm_enter_idle_state - Programs CPU to enter the specified state
>>> + */
>>> +static int arm_generic_enter_idle_state(int idx)
>>> +{
>>> +    int ret;
>>> +
>>> +    if (!idx) {
>>> +        cpu_do_idle();
>>> +        return idx;
>>> +    }
>>> +
>>> +    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);
>>> +
>>> +        cpu_pm_exit();
>>> +    }
>>> +
>>> +    return ret ? -1 : idx;
>>> +}
>>
>> Either you do this, or we have to add it somehow somewhere in
>> drivers/cpuidle to avoid duplicating it.
>>
>> @Daniel: do you have an opinion on this please ?
>
> I don't like the idea to add an ARM arch specific header in
> include/linux. I thought this directory was supposed to contain as much
> as possible arch agnostic headers.
>

While I agree, but what we have is ARM specific code here and calling it
generic might not make it any usable on other architecture unless they 
have the same semantics. I am fine if you and Rafael are OK with that.

> May be the name can be changed to something more generic:
>
> eg.
>
> int cpuidle_generic_enter(int idx);
>
> and then add an option:
>
> HAVE_CPUIDLE_GENERIC_ENTER
>
> , then in the generic header:
>
> #ifdef HAVE_CPUIDLE_GENERIC_ENTER
> int cpuidle_generic_enter(int idx);
> #endif
>
> , change the function name in cpuidle-arm .c
>
> and finally add in the ARM and ARM64 Kconfig's option
> HAVE_CPUIDLE_GENERIC_ENTER.
>
>
>    -- Daniel
>

[1] 
http://git.kernel.org/cgit/linux/kernel/git/sudeep.holla/linux.git/commit/?h=for_review/arm64_lpi&id=9b516ad4442b4168e962ba4ca87bd568d605053b
-- 
Regards,
Sudeep

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

[Sudeep Holla]

On 27/06/16 18:07, Sudeep Holla wrote:
>
>
> On 27/06/16 17:29, Daniel Lezcano wrote:

[...]

>>
>> acpi_disabled - acpi_disabled - acpi_disabled everywhere :/
>>
>> The enable-method approach is not straightforward and now it is polluted
>> by acpi-disabled.
>>
>> So IIUC,
>>
>> smp_init_cpus (contains acpi_disabled)
>>    smp_cpu_setup
>>      cpu_read_ops
>>        cpu_read_enable_method (contains acpi_disabled)
>>          acpi_get_enable_method (returns 'psci' after checking
>> psci_is_present)
>>
>> Then psci_cpu_init_idle is called... and check again acpi_disabled.
>>
>> IMO, the circumlocution with the psci vs acpi vs acpi_disabled is
>> getting unnecessary too complex, is prone to error and will lead to
>> unmaintainable code very soon.
>>
>> I suggest to sort out encapsulation and self-contained code before
>> adding more feature in this area.
>>
>
> I understand your concern but I have no idea on how to clean up. Lorenzo
> asked to factor our common code between psci_{dt,acpi}_cpu_init_idle and
> I think you might not like the refactoring[1]. I didn't want to change
> cpuidle_ops and hence psci_dt_cpu_init_idle parameters. I will see if
> changing that simplifies things.
>

One of the reasons for adding acpi_disabled check is to keep the other
logic at the same place. Otherwise we end up duplicating that code which
is what I have done in psci_{dt,acpi}_cpu_init_idle at the first place.

-- 
Regards,
Sudeep