ARM: qcom: cpuidle support for 8064, 8074, 8084

From: Daniel Lezcano <[email protected]>

Some architectures have some cpus which does not support idle states.

Let the underlying low level code to return -ENOSYS when it is not possible to set an idle state.

Signed-off-by Daniel Lezcano <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[Minor clean ups and fixes of the per-cpu variable]

---
 drivers/cpuidle/cpuidle-arm.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

drivers/cpuidle/cpuidle-arm.c +14 -0

index 1c94b88..f7cdb73 100644
@@ -17,11 +17,14 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/slab.h>
 
 #include <asm/cpuidle.h>
 
 #include "dt_idle_states.h"
 
+static DEFINE_PER_CPU(struct cpuidle_device, *cpuidle_arm_dev);
+
 /*
  * arm_enter_idle_state - Programs CPU to enter the specified state
  *

@@ -93,6 +96,7 @@ static const struct of_device_id arm_idle_state_match[] __initconst = {
 static int __init arm_idle_init(void)
 {
 	int cpu, ret;
+	struct cpuidle_device *dev;
 	struct cpuidle_driver *drv = &arm_idle_driver;
 
 	/*

@@ -105,18 +109,46 @@ static int __init arm_idle_init(void)
 	if (ret <= 0)
 		return ret ? : -ENODEV;
 
+
+	ret = cpuidle_register_driver(drv);
+	if (ret) {
+		pr_err("Failed to register cpuidle driver\n");
+		return ret;
+	}
+
 	/*
 	 * Call arch CPU operations in order to initialize
 	 * idle states suspend back-end specific data
 	 */
 	for_each_possible_cpu(cpu) {
+
 		ret = arm_cpuidle_init(cpu);
+		/*
+		 * This cpu does not support any idle states

drivers/cpuidle/cpuidle-arm.c +13 -1

+		if (!dev)
+			return -ENOMEM;
+
+		dev->cpu = cpu;
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("Failed to register cpuidle device for CPU %d\n",
+			       cpu);
+			return ret;
+		}
+
+		per_cpu(cpuidle_arm_dev, cpu) = dev;
 	}
 
-	return cpuidle_register(drv, NULL);
+	return 0;
 }
 device_initcall(arm_idle_init);

Signature · 6 lines

2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 65 lines · Lina Iyer

From: Daniel Lezcano <[email protected]>

Some architectures have some cpus which does not support idle states.

Let the underlying low level code to return -ENOSYS when it is not possible to set an idle state.

Signed-off-by Daniel Lezcano <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

[Minor clean ups and fixes of the per-cpu variable]

---
 drivers/cpuidle/cpuidle-arm.c | 34 +++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

drivers/cpuidle/cpuidle-arm.c +18 -0

index 1c94b88..f7cdb73 100644
@@ -17,11 +17,14 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/slab.h>
 
 #include <asm/cpuidle.h>
 
 #include "dt_idle_states.h"
 
+static DEFINE_PER_CPU(struct cpuidle_device, *cpuidle_arm_dev);
+
 /*
  * arm_enter_idle_state - Programs CPU to enter the specified state
  *

@@ -93,6 +96,7 @@ static const struct of_device_id arm_idle_state_match[] __initconst = {
 static int __init arm_idle_init(void)
 {
 	int cpu, ret;
+	struct cpuidle_device *dev;
 	struct cpuidle_driver *drv = &arm_idle_driver;
 
 	/*

@@ -105,18 +109,46 @@ static int __init arm_idle_init(void)
 	if (ret <= 0)
 		return ret ? : -ENODEV;
 
+
+	ret = cpuidle_register_driver(drv);
+	if (ret) {
+		pr_err("Failed to register cpuidle driver\n");
+		return ret;
+	}
+
 	/*
 	 * Call arch CPU operations in order to initialize
 	 * idle states suspend back-end specific data
 	 */
 	for_each_possible_cpu(cpu) {
+
 		ret = arm_cpuidle_init(cpu);
+		/*
+		 * This cpu does not support any idle states
+		 */
+		if (ret == -ENOSYS)
+			continue;
+

drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 137

Quoted 6 lines

drivers/cpuidle/cpuidle-arm.c +2 -0

 		if (ret) {
 			pr_err("CPU %d failed to init idle CPU ops\n", cpu);
 			return ret;
 		}
+
+		dev = kzalloc(sizeof(*dev), GFP_KERNEL);

devm_kzalloc? Otherwise, failures could lead to lost memory.

Quoted 25 lines

drivers/cpuidle/cpuidle-arm.c +13 -2

+		if (!dev)
+			return -ENOMEM;
+
+		dev->cpu = cpu;
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("Failed to register cpuidle device for CPU %d\n",
+			       cpu);
+			return ret;
+		}
+
+		per_cpu(cpuidle_arm_dev, cpu) = dev;
 	}
 
-	return cpuidle_register(drv, NULL);
+	return 0;
 }
 device_initcall(arm_idle_init);
--

2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Quoted 9 lines

Quoted 9 lines

	for_each_possible_cpu(cpu) {
+
		ret = arm_cpuidle_init(cpu);

drivers/cpuidle/cpuidle-arm.c +6 -0

+		/*
+		 * This cpu does not support any idle states
+		 */
+		if (ret == -ENOSYS)
+			continue;
+

Quoted 9 lines

Review

Quoted 6 lines

		if (ret) {
			pr_err("CPU %d failed to init idle CPU ops\n", cpu);
			return ret;
		}

(quoted diff) +2 -0

+
+		dev = kzalloc(sizeof(*dev), GFP_KERNEL);

devm_kzalloc? Otherwise, failures could lead to lost memory.

I dont have a local device to work with for allocation. May be I can get the cpu device node and the device therefore for allocation. Thoughts?

Quoted 8 lines

Quoted 8 lines

drivers/cpuidle/cpuidle-arm.c +8 -0

+		if (!dev)
+			return -ENOMEM;
+
+		dev->cpu = cpu;
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("Failed to register cpuidle device for CPU %d\n",
+			       cpu);

drivers/cpuidle/cpuidle-arm.c +4 -0

+		if (ret == -ENOSYS)
+			continue;
+
 		if (ret) {
 			pr_err("CPU %d failed to init idle CPU ops\n", cpu);
 			return ret;
 		}
+

drivers/cpuidle/cpuidle-arm.c +4 -1

+		}
+
+		per_cpu(cpuidle_arm_dev, cpu) = dev;
 	}
 
-	return cpuidle_register(drv, NULL);
+	return 0;
 }
 device_initcall(arm_idle_init);

drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 112

Quoted 5 lines · Lina Iyer

drivers/cpuidle/cpuidle-arm.c +1 -0

@@ -105,18 +109,46 @@ static int __init arm_idle_init(void)
 	if (ret <= 0)
 		return ret ? : -ENODEV;
 
+

A bit better formatting would be nice - you don't need the extra blank line here.

drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 124

Quoted 12 lines

drivers/cpuidle/cpuidle-arm.c +7 -0

+	ret = cpuidle_register_driver(drv);
+	if (ret) {
+		pr_err("Failed to register cpuidle driver\n");
+		return ret;
+	}
+
 	/*
 	 * Call arch CPU operations in order to initialize
 	 * idle states suspend back-end specific data
 	 */
 	for_each_possible_cpu(cpu) {
+

This blank line is not necessary either.

drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 125

Quoted 1 line

drivers/cpuidle/cpuidle-arm.c +0 -0

 		ret = arm_cpuidle_init(cpu);

However, a blank line here would be a good thing.

drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 128

Quoted 3 lines

drivers/cpuidle/cpuidle-arm.c +3 -0

+		/*
+		 * This cpu does not support any idle states
+		 */

Also, formatting this as /* This cpu does not support any idle states */ is acceptable too, and doesn't waste as many lines.

Quoted 12 lines

drivers/cpuidle/cpuidle-arm.c +8 -0

+		if (ret == -ENOSYS)
+			continue;
+
 		if (ret) {
 			pr_err("CPU %d failed to init idle CPU ops\n", cpu);
 			return ret;
 		}
+
+		dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+		if (!dev)
+			return -ENOMEM;
+

drivers/cpuidle/cpuidle-arm.c static int __init arm_idle_init(void) · line 146

Quoted 6 lines

drivers/cpuidle/cpuidle-arm.c +6 -0

+		dev->cpu = cpu;
+		ret = cpuidle_register_device(dev);
+		if (ret) {
+			pr_err("Failed to register cpuidle device for CPU %d\n",
+			       cpu);
+			return ret;

It looks like we leak the 'dev' allocation here.

Also, other error paths, it looks like we leave the previously registered cpuidle devices in place. That may be acceptable if the intention is to initialise as many CPUs as possible - but we then miss the cpuidle_register() call below, which seems to make the registered devices useless. It's a little inconsistent.

Also, it's useful to report why something fails - printing the error code can help debugging if it isn't already printed elsewhere.

Quoted 9 lines

drivers/cpuidle/cpuidle-arm.c +4 -1

+		}
+
+		per_cpu(cpuidle_arm_dev, cpu) = dev;
 	}
 
-	return cpuidle_register(drv, NULL);
+	return 0;
 }
 device_initcall(arm_idle_init);

Signature · 2 lines

FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

SPM is a hardware block that controls the peripheral logic surrounding the application cores (cpu/l$). When the core executes WFI instruction, the SPM takes over the putting the core in low power state as configured. The wake up for the SPM is an interrupt at the GIC, which then completes the rest of low power mode sequence and brings the core out of low power mode.

The SPM has a set of control registers that configure the SPMs individually based on the type of the core and the runtime conditions. SPM is a finite state machine block to which a sequence is provided and it interprets the bytes and executes them in sequence. Each low power mode that the core can enter into is provided to the SPM as a sequence.

Configure the SPM to set the core (cpu or L2) into its low power mode, the index of the first command in the sequence is set in the SPM_CTL register. When the core executes ARM wfi instruction, it triggers the SPM state machine to start executing from that index. The SPM state machine waits until the interrupt occurs and starts executing the rest of the sequence until it hits the end of the sequence. The end of the sequence jumps the core out of its low power mode.

Add support for an idle driver to set up the SPM to place the core in Standby or Standalone power collapse mode when the core is idle.

Based on work by: Mahesh Sivasubramanian <[email protected]>, Ai Li <[email protected]>, Praveen Chidambaram <[email protected]> Original tree available at -
git://codeaurora.org/quic/la/kernel/msm-3.10.git

Cc Stephen Boyd <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Daniel Lezcano <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---

.../devicetree/bindings/arm/msm/qcom,saw2.txt      |  31 +-
drivers/soc/qcom/Kconfig                           |   7 +
drivers/soc/qcom/Makefile                          |   1 +
drivers/soc/qcom/spm.c                             | 422 +++++++++++++++++++++
4 files changed, 455 insertions(+), 6 deletions(-)

create mode 100644 drivers/soc/qcom/spm.c

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +0 -0

index 1505fb8..690c3c0 100644
@@ -2,11 +2,20 @@ SPM AVS Wrapper 2 (SAW2)
 

The SAW2 is a wrapper around the Subsystem Power Manager (SPM) and the Adaptive Voltage Scaling (AVS) hardware. The SPM is a programmable

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +1 -1

-micro-controller that transitions a piece of hardware (like a processor or
+power-controller that transitions a piece of hardware (like a processor or

subsystem) into and out of low power modes via a direct connection to the PMIC. It can also be wired up to interact with other processors in the system, notifying them when a low power state is entered or exited.

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +9 -0

+Multiple revisions of the SAW hardware are supported using these Device Nodes.
+SAW2 revisions differ in the register offset and configuration data. Also, the
+same revision of the SAW in different SoCs may have different configuration
+data due the the differences in hardware capabilities. Hence the SoC name, the
+version of the SAW hardware in that SoC and the distinction between cpu (big
+or Little) or cache, may be needed to uniquely identify the SAW register
+configuration and initialization data. The compatible string is used to
+indicate this parameter.
+

PROPERTIES

- compatible:

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +0 -2

@@ -14,10 +23,13 @@ PROPERTIES
 	Value type: <string>
 	Definition: shall contain "qcom,saw2". A more specific value should be
 		    one of:
-			 "qcom,saw2-v1"
-			 "qcom,saw2-v1.1"

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +6 -1

- reg:
 	Usage: required

@@ -26,10 +38,17 @@ PROPERTIES
 		    the register region. An optional second element specifies
 		    the base address and size of the alias register region.
 
+- regulator:
+	Usage: optional
+	Value type: boolean
+	Definition: Indicates that this SPM device acts as a regulator device
+			device for the core (CPU or Cache) the SPM is attached
+			to.
 

Example:

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +1 -1

-	regulator at 2099000 {
+	power-controller at 2099000 {

		compatible = "qcom,saw2";
		reg = <0x02099000 0x1000>, <0x02009000 0x1000>;

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +1 -0

+		regulator;

	};

Quoted 78 lines · Lina Iyer

SPM is a hardware block that controls the peripheral logic surrounding the application cores (cpu/l$). When the core executes WFI instruction, the SPM takes over the putting the core in low power state as configured. The wake up for the SPM is an interrupt at the GIC, which then completes the rest of low power mode sequence and brings the core out of low power mode.

The SPM has a set of control registers that configure the SPMs individually based on the type of the core and the runtime conditions. SPM is a finite state machine block to which a sequence is provided and it interprets the bytes and executes them in sequence. Each low power mode that the core can enter into is provided to the SPM as a sequence.

Configure the SPM to set the core (cpu or L2) into its low power mode, the index of the first command in the sequence is set in the SPM_CTL register. When the core executes ARM wfi instruction, it triggers the SPM state machine to start executing from that index. The SPM state machine waits until the interrupt occurs and starts executing the rest of the sequence until it hits the end of the sequence. The end of the sequence jumps the core out of its low power mode.

Add support for an idle driver to set up the SPM to place the core in Standby or Standalone power collapse mode when the core is idle.

Based on work by: Mahesh Sivasubramanian <[email protected]>, Ai Li <[email protected]>, Praveen Chidambaram <[email protected]> Original tree available at -
git://codeaurora.org/quic/la/kernel/msm-3.10.git

Cc Stephen Boyd <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Daniel Lezcano <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---

.../devicetree/bindings/arm/msm/qcom,saw2.txt      |  31 +-
drivers/soc/qcom/Kconfig                           |   7 +
drivers/soc/qcom/Makefile                          |   1 +
drivers/soc/qcom/spm.c                             | 422 +++++++++++++++++++++
4 files changed, 455 insertions(+), 6 deletions(-)

create mode 100644 drivers/soc/qcom/spm.c

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +0 -0

index 1505fb8..690c3c0 100644
@@ -2,11 +2,20 @@ SPM AVS Wrapper 2 (SAW2)
 

The SAW2 is a wrapper around the Subsystem Power Manager (SPM) and the Adaptive Voltage Scaling (AVS) hardware. The SPM is a programmable

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +1 -1

-micro-controller that transitions a piece of hardware (like a processor or
+power-controller that transitions a piece of hardware (like a processor or

subsystem) into and out of low power modes via a direct connection to the PMIC. It can also be wired up to interact with other processors in the system, notifying them when a low power state is entered or exited.

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +9 -0

+Multiple revisions of the SAW hardware are supported using these Device Nodes.
+SAW2 revisions differ in the register offset and configuration data. Also, the
+same revision of the SAW in different SoCs may have different configuration
+data due the the differences in hardware capabilities. Hence the SoC name, the
+version of the SAW hardware in that SoC and the distinction between cpu (big
+or Little) or cache, may be needed to uniquely identify the SAW register
+configuration and initialization data. The compatible string is used to
+indicate this parameter.
+

PROPERTIES

- compatible:

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +2 -4

@@ -14,10 +23,13 @@ PROPERTIES
 	Value type: <string>
 	Definition: shall contain "qcom,saw2". A more specific value should be
 		    one of:
-			 "qcom,saw2-v1"
-			 "qcom,saw2-v1.1"
-			 "qcom,saw2-v2"
-			 "qcom,saw2-v2.1"
+			"qcom,saw2-v1"
+			"qcom,saw2-v1.1"

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt PROPERTIES · line 33

Quoted 6 lines

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +5 -0

+			"qcom,saw2-v2"
+			"qcom,saw2-v2.1"
+			"qcom,apq8064-saw2-v1.1-cpu"
+			"qcom,msm8974-saw2-v2.1-cpu"
+			"qcom,apq8084-saw2-v2.1-cpu?
 

We don?t seem to use the "qcom,saw2-v1*? variants so should we just drop them?

Quoted 22 lines

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +6 -1

- reg:
 	Usage: required

@@ -26,10 +38,17 @@ PROPERTIES
 		    the register region. An optional second element specifies
 		    the base address and size of the alias register region.
 
+- regulator:
+	Usage: optional
+	Value type: boolean
+	Definition: Indicates that this SPM device acts as a regulator device
+			device for the core (CPU or Cache) the SPM is attached
+			to.
 

Example:

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +1 -1

-	regulator at 2099000 {
+	power-controller at 2099000 {

		compatible = "qcom,saw2";
		reg = <0x02099000 0x1000>, <0x02009000 0x1000>;

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt +1 -0

+		regulator;

Quoted 1 line

	};

- k

Signature · 3 lines

Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

drivers/soc/qcom/spm.c new file +12 -0

+
+	/*
+	 * Returns here only if there was a pending interrupt and we did not
+	 * power down as a result.
+	 */
+	return -1;
+}
+
+static int qcom_cpu_standby(int cpuarg)
+{
+	spm_set_low_power_mode(PM_SLEEP_MODE_STBY);
+	cpu_do_idle();

drivers/soc/qcom/spm.c new file +35 -0

+	{ .compatible = "qcom,idle-state-stby", .data = qcom_cpu_standby },
+	{ .compatible = "qcom,idle-state-spc", .data = qcom_cpu_spc },
+	{ },
+};
+
+static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
+{
+	const struct of_device_id *match_id;
+	struct device_node *state_node;
+	int i;
+	int state_count = 0;
+	idle_fn idle_fns[CPUIDLE_STATE_MAX];
+	idle_fn *fns;
+	cpumask_t mask;
+	bool use_scm_power_down = false;
+
+	for (i = 0; ; i++) {
+		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+		if (!state_node)
+			break;
+
+		if (!of_device_is_available(state_node))
+			continue;
+
+		if (i == CPUIDLE_STATE_MAX) {
+			pr_warn("%s: cpuidle states reached max possible\n",
+					__func__);
+			break;
+		}
+
+		match_id = of_match_node(qcom_idle_state_match, state_node);
+		if (!match_id)
+			return -ENODEV;
+
+		idle_fns[state_count] = match_id->data;

drivers/soc/qcom/spm.c new file +21 -0

+		cpumask_clear(&mask);
+		cpumask_set_cpu(cpu, &mask);
+		qcom_scm_set_warm_boot_addr(cpu_resume, &mask);
+	}
+
+	per_cpu(qcom_idle_ops, cpu) = fns;
+
+	/*
+	 * Condition: cpuidle_driver_register() needs to happen before
+	 * cpuidle_register_device().
+	 * Check if the SPM probe has happened -
+	 * - If SPM probed successfully before arm_idle_init(), then defer
+	 *   the registration of cpuidle_device back to arm_idle_init()
+	 * - If the SPM probe happens in the future, then let the SPM probe
+	 *   register the cpuidle device, return -ENOSYS.
+	 */
+	return per_cpu(cpu_spm_drv, cpu) ? 0 : -ENOSYS;
+}
+
+struct cpuidle_ops qcom_kpss_v1_cpuidle_ops __initdata = {
+	.name = "qcom,kpss-acc-v1",

drivers/soc/qcom/spm.c line 216

Quoted 5 lines · Lina Iyer

drivers/soc/qcom/spm.c new file +5 -0

+
+static int qcom_idle_enter(int cpu, unsigned long index)
+{
+	if (!per_cpu(qcom_idle_ops, cpu)[index])
+		return -EOPNOTSUPP;

Is this case still happening?

drivers/soc/qcom/spm.c line 221

Quoted 5 lines

drivers/soc/qcom/spm.c new file +5 -0

+
+	return per_cpu(qcom_idle_ops, cpu)[index](cpu);
+}
+
+const struct of_device_id qcom_idle_state_match[] __initconst = {

static?

Quoted 39 lines

drivers/soc/qcom/spm.c new file +39 -0

+	{ .compatible = "qcom,idle-state-stby", .data = qcom_cpu_standby },
+	{ .compatible = "qcom,idle-state-spc", .data = qcom_cpu_spc },
+	{ },
+};
+
+static int __init qcom_cpuidle_init(struct device_node *cpu_node, int cpu)
+{
+	const struct of_device_id *match_id;
+	struct device_node *state_node;
+	int i;
+	int state_count = 0;
+	idle_fn idle_fns[CPUIDLE_STATE_MAX];
+	idle_fn *fns;
+	cpumask_t mask;
+	bool use_scm_power_down = false;
+
+	for (i = 0; ; i++) {
+		state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+		if (!state_node)
+			break;
+
+		if (!of_device_is_available(state_node))
+			continue;
+
+		if (i == CPUIDLE_STATE_MAX) {
+			pr_warn("%s: cpuidle states reached max possible\n",
+					__func__);
+			break;
+		}
+
+		match_id = of_match_node(qcom_idle_state_match, state_node);
+		if (!match_id)
+			return -ENODEV;
+
+		idle_fns[state_count] = match_id->data;
+
+		/* Check if any of the states allow power down */
+		if (match_id->data == qcom_cpu_spc)
+			use_scm_power_down = true;

drivers/soc/qcom/spm.c line 266

Quoted 6 lines

drivers/soc/qcom/spm.c new file +6 -0

+
+		state_count++;
+	}
+
+	if (!state_count) {
+		pr_warn("No idle ops founds for cpu %d\n", cpu);

Maybe pr_debug? It's not the end of the world that we don't have cpuidle.

Quoted 6 lines

drivers/soc/qcom/spm.c new file +6 -0

+		return -ENODEV;
+	}
+
+	fns = kcalloc(state_count, sizeof(*fns), GFP_KERNEL);
+	if (!fns)
+		return -ENOMEM;

drivers/soc/qcom/spm.c line 278

Quoted 6 lines

drivers/soc/qcom/spm.c new file +6 -0

+
+	for (i = 0; i < state_count; i++)
+		fns[i] = idle_fns[i];
+
+	if (use_scm_power_down) {
+		/* We have atlease one power down mode */

s/atlease/at least/

Quoted 28 lines

drivers/soc/qcom/spm.c new file +28 -0

+		cpumask_clear(&mask);
+		cpumask_set_cpu(cpu, &mask);
+		qcom_scm_set_warm_boot_addr(cpu_resume, &mask);
+	}
+
+	per_cpu(qcom_idle_ops, cpu) = fns;
+
+	/*
+	 * Condition: cpuidle_driver_register() needs to happen before
+	 * cpuidle_register_device().
+	 * Check if the SPM probe has happened -
+	 * - If SPM probed successfully before arm_idle_init(), then defer
+	 *   the registration of cpuidle_device back to arm_idle_init()
+	 * - If the SPM probe happens in the future, then let the SPM probe
+	 *   register the cpuidle device, return -ENOSYS.
+	 */
+	return per_cpu(cpu_spm_drv, cpu) ? 0 : -ENOSYS;
+}
+
+struct cpuidle_ops qcom_kpss_v1_cpuidle_ops __initdata = {
+	.name = "qcom,kpss-acc-v1",
+	.suspend = qcom_idle_enter,
+	.init = qcom_cpuidle_init,
+};
+
+struct cpuidle_ops qcom_kpss_v2_cpuidle_ops __initdata = {
+	.name = "qcom,kpss-acc-v2",
+	.suspend = qcom_idle_enter,

drivers/soc/qcom/spm.c line 303

Quoted 4 lines

drivers/soc/qcom/spm.c new file +3 -0

+	.init = qcom_cpuidle_init,
+};
+

This just looks weird because of the macro magic in Daniel's series. Any reason we can't use the linker instead of doing preprocessor magic so that it looks like these structures are actually used?

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Register cpuidle ops for Krait Processor Sub-System. Supports v1 and v2 versions of KPSS.

Cc Stephen Boyd <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Daniel Lezcano <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
 arch/arm/include/asm/cpuidle_ops.h | 5 +++++
 1 file changed, 5 insertions(+)

arch/arm/include/asm/cpuidle_ops.h +0 -0

index be0a612..40cb262 100644

arch/arm/include/asm/cpuidle_ops.h +3 -0

+CPUIDLE_OPS(qcom_kpss_v1)
+CPUIDLE_OPS(qcom_kpss_v2)
+#endif

Quoted 12 lines · Lina Iyer

Register cpuidle ops for Krait Processor Sub-System. Supports v1 and v2 versions of KPSS.

Cc Stephen Boyd <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Kevin Hilman <[email protected]>

Cc Daniel Lezcano <[email protected]>

Signed-off-by Lina Iyer <[email protected]>

---
  arch/arm/include/asm/cpuidle_ops.h | 5 +++++
  1 file changed, 5 insertions(+)

arch/arm/include/asm/cpuidle_ops.h line 5

Quoted 10 lines

arch/arm/include/asm/cpuidle_ops.h +2 -0

index be0a612..40cb262 100644
@@ -1,3 +1,8 @@
  /*
   * List of cpuidle operations
   */
+
+#if IS_ENABLED(CONFIG_QCOM_PM)

Why not #ifdef CONFIG_QCOM_PM? I thought IS_ENABLED was for C code where you want to put it in an if () statement.

Quoted 3 lines

arch/arm/include/asm/cpuidle_ops.h +3 -0

+CPUIDLE_OPS(qcom_kpss_v1)
+CPUIDLE_OPS(qcom_kpss_v2)
+#endif

Quoted 1 line · Stephen Boyd

On 03/09/15 08:16, Lina Iyer wrote:

Quoted 4 lines

Quoted 1 line

+#if IS_ENABLED(CONFIG_QCOM_PM)

Why not #ifdef CONFIG_QCOM_PM? I thought IS_ENABLED was for C code where you want to put it in an if () statement.

This is right. But the IS_ENABLED is also valid.

It seems the current trend is to replace #ifdef with #if IS_ENABLED. It is supposed to prevent to create or'ed conditional with the module support.

eg:

#if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)

is replaced by:

#if IS_ENABLED(CONFIG_FOO)

Lina: By the way, it would be preferable to use the CONFIG_ARM_QCOM_CPUIDLE where IS_ENABLED makes more sense even the

drivers are not modules (who knows if they are converted as module again hence #ifdef will be invalid).

Quoted 3 lines

Quoted 3 lines

arch/arm/include/asm/cpuidle_ops.h +3 -0

+CPUIDLE_OPS(qcom_kpss_v1)
+CPUIDLE_OPS(qcom_kpss_v2)
+#endif

Quoted 2 lines · Daniel Lezcano

Quoted 1 line · Stephen Boyd

On 03/09/15 08:16, Lina Iyer wrote:

[...]

Quoted 13 lines

Quoted 4 lines

Quoted 1 line

+#if IS_ENABLED(CONFIG_QCOM_PM)

Why not #ifdef CONFIG_QCOM_PM? I thought IS_ENABLED was for C code where you want to put it in an if () statement.

This is right. But the IS_ENABLED is also valid.

It seems the current trend is to replace #ifdef with #if IS_ENABLED. It is supposed to prevent to create or'ed conditional with the module support.

eg:

Quoted 8 lines

#if defined(CONFIG_FOO) || defined(CONFIG_FOO_MODULE)

is replaced by:

#if IS_ENABLED(CONFIG_FOO)

Lina: By the way, it would be preferable to use the CONFIG_ARM_QCOM_CPUIDLE where IS_ENABLED makes more sense even the

Daniel: I dont have CONFIG_ARM_QCOM_CPUIDLE defined anymore. By moving to generic ARM cpuidle, i removed all changes to drivers/cpuidle/. So I used the config item that defineds these items.

Also, having two indpendent CONFIG_ items provides a chance that the declaration is not matched with the definition or vice versa.

Quoted 8 lines

drivers are not modules (who knows if they are converted as module again hence #ifdef will be invalid).

Quoted 3 lines

Quoted 3 lines

arch/arm/include/asm/cpuidle_ops.h +3 -0

+CPUIDLE_OPS(qcom_kpss_v1)
+CPUIDLE_OPS(qcom_kpss_v2)
+#endif