QCOM 8974 and 8084 cpuidle driver

Based on work by many authors, available at codeaurora.org

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

+	u32 ctl_val;
+
+	if (!drv || !drv->reg_data)

+#endif /* __QCOM_SPM_H */

Based on work by many authors, available at codeaurora.org

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

Based on work by many authors, available at codeaurora.org

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.

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

[lina: simplify the driver for initial submission, clean up and update commit text]

---
 .../devicetree/bindings/arm/msm/qcom,saw2.txt      |  10 +-
 drivers/soc/qcom/Kconfig                           |   8 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/spm.c                             | 216 +++++++++++++++++++++
 include/soc/qcom/spm.h                             |  35 ++++
 5 files changed, 264 insertions(+), 6 deletions(-)
 create mode 100644 drivers/soc/qcom/spm.c
 create mode 100644 include/soc/qcom/spm.h

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

index 1505fb8..9a9cc99 100644

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

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

drivers/soc/qcom/Kconfig +0 -0

index 7dcd554..cd249c4 100644

drivers/soc/qcom/Kconfig +4 -0

+	help
+	  QCOM Platform specific power driver to manage cores and L2 low power
+	  modes. It interface with various system drivers to put the cores in
+	  low power modes.

drivers/soc/qcom/Makefile +1 -0

index 70d52ed..20b329f 100644
@@ -1,3 +1,4 @@
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
+obj-$(CONFIG_QCOM_PM)	+=	spm.o
 CFLAGS_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
 obj-$(CONFIG_QCOM_SCM) += scm.o scm-boot.o

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

new file mode 100644
index 0000000..0ba7949
@@ -0,0 +1,216 @@
+/* Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+
+#include <soc/qcom/spm.h>
+
+enum {
+	SPM_REG_CFG,
+	SPM_REG_SPM_CTL,
+	SPM_REG_DLY,
+	SPM_REG_PMIC_DATA_0,
+	SPM_REG_PMIC_DATA_1,
+	SPM_REG_PMIC_DATA_2,
+	SPM_REG_PMIC_DATA_3,
+	SPM_REG_VCTL,
+	SPM_REG_SEQ_ENTRY_0,
+	SPM_REG_SEQ_ENTRY_1,
+	SPM_REG_SEQ_ENTRY_2,
+	SPM_REG_SEQ_ENTRY_3,
+	SPM_REG_SEQ_ENTRY_4,
+	SPM_REG_SEQ_ENTRY_5,
+	SPM_REG_SEQ_ENTRY_6,
+	SPM_REG_SEQ_ENTRY_7,
+	SPM_REG_SEQ_ENTRY_LAST = SPM_REG_SEQ_ENTRY_7,
+	SPM_REG_SPM_STS,
+	SPM_REG_PMIC_STS,
+	SPM_REG_NR,
+};
+
+struct spm_reg_data {
+	/* Register position and initialization value */
+	struct register_info {
+		u8 offset;
+		u32 value;
+	} reg[SPM_REG_NR];
+
+	/* Start address offset for the supported idle states*/
+	u8 start_addr[SPM_MODE_NR];
+};
+
+struct spm_driver_data {
+	void __iomem *reg_base_addr;
+	const struct spm_reg_data *reg_data;
+};
+
+/* SPM register data for 8974, 8084 */
+static const struct spm_reg_data spm_reg_8974_8084_cpu  = {
+	.reg[SPM_REG_CFG]		= {0x08, 0x1},
+	.reg[SPM_REG_SPM_STS]		= {0x0C, 0x0},
+	.reg[SPM_REG_PMIC_STS]		= {0x14, 0x0},
+	.reg[SPM_REG_VCTL]		= {0x1C, 0x0},

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

+	.reg[SPM_REG_SEQ_ENTRY_5]	= {0x94, 0x0},
+	.reg[SPM_REG_SEQ_ENTRY_6]	= {0x98, 0x0},
+	.reg[SPM_REG_SEQ_ENTRY_7]	= {0x9C, 0x0},
+
+	.start_addr[SPM_MODE_CLOCK_GATING]	= 0,
+	.start_addr[SPM_MODE_POWER_COLLAPSE]	= 3,
+};
+

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

+	ctl_val = readl_relaxed(drv->reg_base_addr +
+				drv->reg_data->reg[SPM_REG_SPM_CTL].offset);
+	start_addr &= 0x7F;
+	start_addr <<= 4;
+	ctl_val &= 0xFFFFF80F;
+	ctl_val |= start_addr;
+	writel_relaxed(ctl_val, drv->reg_base_addr +
+				drv->reg_data->reg[SPM_REG_SPM_CTL].offset);
+	/* Ensure we have written the start address */
+	wmb();
+
+	return 0;
+}
+
+static struct spm_driver_data *spm_get_drv(struct platform_device *pdev)
+{
+	struct spm_driver_data *drv = NULL;
+	struct device_node *cpu_node, *saw_node;
+	u32 cpu;
+
+	for_each_possible_cpu(cpu) {

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

+	}
+
+	return drv;
+}
+
+static const struct of_device_id spm_match_table[] __initconst = {
+	{ .compatible = "qcom,msm8974-saw2-v2.1-cpu",
+	  .data = &spm_reg_8974_8084_cpu },
+	{ .compatible = "qcom,apq8084-saw2-v2.1-cpu",
+	  .data = &spm_reg_8974_8084_cpu },
+	{ },
+};
+
+static int spm_dev_probe(struct platform_device *pdev)
+{
+	struct spm_driver_data *drv;
+	struct resource *res;
+	const struct of_device_id *match_id;
+	int i;
+
+	 /* Get the right SPM device */
+	drv = spm_get_drv(pdev);
+	if (!drv)
+		return -EINVAL;
+
+	/* Get the SPM start address */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	drv->reg_base_addr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(drv->reg_base_addr))
+		return PTR_ERR(drv->reg_base_addr);
+
+	match_id = of_match_node(spm_match_table, pdev->dev.of_node);
+	if (!match_id)
+		return -ENODEV;
+
+	/* Get the SPM register data for this instance */
+	drv->reg_data = match_id->data;
+	if (!drv->reg_data)
+		return -EINVAL;
+
+	/* Write the SPM sequences first */
+	for (i = SPM_REG_SEQ_ENTRY_0; i <= SPM_REG_SEQ_ENTRY_LAST; i++)
+		writel_relaxed(drv->reg_data->reg[i].value,
+			drv->reg_base_addr + drv->reg_data->reg[i].offset);
+
+	/* Write the SPM control registers */
+	writel_relaxed(drv->reg_data->reg[SPM_REG_DLY].value,
+		drv->reg_base_addr + drv->reg_data->reg[SPM_REG_DLY].offset);
+
+	writel_relaxed(drv->reg_data->reg[SPM_REG_CFG].value,
+		drv->reg_base_addr + drv->reg_data->reg[SPM_REG_CFG].offset);
+
+	writel_relaxed(drv->reg_data->reg[SPM_REG_SPM_CTL].value,
+			drv->reg_base_addr +
+			drv->reg_data->reg[SPM_REG_SPM_CTL].offset);
+
+	/**
+	 * Ensure all observers see the above register writes before the
+	 * cpuidle driver is allowed to use the SPM.

include/soc/qcom/spm.h new file +25 -0

new file mode 100644
index 0000000..997abfc
@@ -0,0 +1,35 @@
+/* Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __QCOM_SPM_H
+#define __QCOM_SPM_H
+
+enum spm_mode {
+	SPM_MODE_CLOCK_GATING,
+	SPM_MODE_RETENTION,
+	SPM_MODE_GDHS,
+	SPM_MODE_POWER_COLLAPSE,
+	SPM_MODE_NR
+};
+
+#if defined(CONFIG_QCOM_PM)
+

On 09/26/14 17:58, Lina Iyer wrote:

Definition: shall contain "qcom,saw2". A more specific value should be

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

Quoted 8 lines

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

 		    one of:
-			 "qcom,saw2-v1"
-			 "qcom,saw2-v1.1"
-			 "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"

It's probably not good to remove the old compatibles. Just add more to the list. Please Cc dt reviewers on dt bindings.

Quoted 10 lines

- reg:

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

 	Usage: required

@@ -26,10 +25,9 @@ PROPERTIES
 		    the register region. An optional second element specifies
 		    the base address and size of the alias register region.
 
-
 Example:
 

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

Quoted 2 lines

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

-	regulator at 2099000 {
+	saw at 2099000 {

saw is not a standard thing. Hence the usage of regulator here. I agree when it doesn't directly control a regulator then it should be called something else, power-controller perhaps? I don't really see a need to change this example though.

drivers/soc/qcom/Kconfig

Quoted 1 line

drivers/soc/qcom/Kconfig +0 -0

+config QCOM_PM

drivers/soc/qcom/Kconfig config QCOM_GSBI · line 17

Quoted 2 lines

drivers/soc/qcom/Kconfig +2 -0

+	bool "Qualcomm Power Management"
+	depends on PM && ARCH_QCOM

Drop the PM dependency. There isn't any right? Honestly we don't want this type of option at all. We want an option for each driver introduced.

+

Quoted 10 lines

drivers/soc/qcom/Kconfig +10 -0

+/* SPM register data for 8974, 8084 */
+static const struct spm_reg_data spm_reg_8974_8084_cpu  = {
+	.reg[SPM_REG_CFG]		= {0x08, 0x1},
+	.reg[SPM_REG_SPM_STS]		= {0x0C, 0x0},
+	.reg[SPM_REG_PMIC_STS]		= {0x14, 0x0},
+	.reg[SPM_REG_VCTL]		= {0x1C, 0x0},
+	.reg[SPM_REG_SPM_CTL]		= {0x30, 0x1},
+	.reg[SPM_REG_DLY]		= {0x34, 0x3C102800},
+	.reg[SPM_REG_PMIC_DATA_0]	= {0x40, 0x0},
+	.reg[SPM_REG_PMIC_DATA_1]	= {0x44, 0x0},

drivers/soc/qcom/Kconfig

Quoted 6 lines

drivers/soc/qcom/Kconfig +6 -0

+	.reg[SPM_REG_PMIC_DATA_2]	= {0x48, 0x0},
+	.reg[SPM_REG_SEQ_ENTRY_0]	= {0x80, 0x000F0B03},
+	.reg[SPM_REG_SEQ_ENTRY_1]	= {0x84, 0xE8108020},
+	.reg[SPM_REG_SEQ_ENTRY_2]	= {0x88, 0xE83B035B},
+	.reg[SPM_REG_SEQ_ENTRY_3]	= {0x8C, 0x300B1082},
+	.reg[SPM_REG_SEQ_ENTRY_4]	= {0x90, 0x0F302606},

Is this endian agnostic?

We don't need this initial value table. The
only thing that really is different is delay and seq entries.

The seq
entries can be a byte array that gets written to the device in an endian agnostic fashion and the delay can be a different struct member.

The
register map can be per version of the spm (i.e. not per-soc) and that can be pointed to by the SoC data.

I really don't like setting the SPM_CTL register's enable bit to 1 with this table either. That should be done explicitly because it isn't "configuration" like the delays or the sequences are. It's a bit that will have some effect. It probably even needs to be cleared if we're reprogramming the SPM sequence in a scenario like kexec where the bit may already be set.

Quoted 12 lines

drivers/soc/qcom/Kconfig +12 -0

+	.reg[SPM_REG_SEQ_ENTRY_5]	= {0x94, 0x0},
+	.reg[SPM_REG_SEQ_ENTRY_6]	= {0x98, 0x0},
+	.reg[SPM_REG_SEQ_ENTRY_7]	= {0x9C, 0x0},
+
+	.start_addr[SPM_MODE_CLOCK_GATING]	= 0,
+	.start_addr[SPM_MODE_POWER_COLLAPSE]	= 3,
+};
+
+static DEFINE_PER_CPU_SHARED_ALIGNED(struct spm_driver_data, cpu_spm_drv);
+
+/**
+ * spm_set_low_power_mode() - Configure SPM start address for low power mode

drivers/soc/qcom/Kconfig

Quoted 6 lines

+ * @mode: SPM LPM mode to enter
+ */

drivers/soc/qcom/Kconfig +4 -0

+int qcom_spm_set_low_power_mode(enum spm_mode mode)
+{
+	struct spm_driver_data *drv = &__get_cpu_var(cpu_spm_drv);
+	u32 start_addr = 0;

Unnecessary assignment.

drivers/soc/qcom/Kconfig

Quoted 4 lines

drivers/soc/qcom/Kconfig +4 -0

+	u32 ctl_val;
+
+	if (!drv || !drv->reg_data)
+		return -ENXIO;

Does this ever happen? Please remove.

drivers/soc/qcom/Kconfig

Quoted 4 lines

drivers/soc/qcom/Kconfig +4 -0

+
+	start_addr = drv->reg_data->start_addr[mode];
+
+	/* Update bits 10:4 in the SPM CTL register */

This comment provides nothing that isn't evident from the code. Remove.

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

+	for_each_possible_cpu(cpu) {

Quoted 4 lines

drivers/soc/qcom/Kconfig +4 -0

+		cpu_node = of_get_cpu_node(cpu, NULL);
+		if (!cpu_node)
+			continue;
+		saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0);

drivers/soc/qcom/Kconfig

Quoted 6 lines

drivers/soc/qcom/Kconfig +6 -0

+		if (!saw_node)
+			continue;
+		if (saw_node == pdev->dev.of_node) {
+			drv = &per_cpu(cpu_spm_drv, cpu);
+			break;
+		}

Missing a couple of_node_put()s.

+

drivers/soc/qcom/Kconfig

Quoted 4 lines

drivers/soc/qcom/Kconfig +4 -0

+static struct platform_driver spm_driver = {
+	.probe = spm_dev_probe,
+	.driver = {
+		.name = "spm",

qcom-spm?

drivers/soc/qcom/Kconfig

Quoted 5 lines

drivers/soc/qcom/Kconfig +5 -0

+static int __init spm_driver_init(void)
+{
+	return platform_driver_register(&spm_driver);
+}
+device_initcall(spm_driver_init);

Why can't we support modules?

include/soc/qcom/spm.h

Quoted 5 lines

include/soc/qcom/spm.h new file +0 -0

new file mode 100644
index 0000000..997abfc

+#endif /* __QCOM_SPM_H */

It would be nice to not have this file.

Based on work by many authors, available at codeaurora.org

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.

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

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

index 1505fb8..9a9cc99 100644

Quoted 24 lines · Lina Iyer

Based on work by many authors, available at codeaurora.org

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.

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

[...]

Quoted 10 lines

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

index 1505fb8..9a9cc99 100644
@@ -14,10 +14,9 @@ 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 PROPERTIES · line 19

Quoted 5 lines

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

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

This looks odd (to me.) Why are the SoC name and the SAW2 version both needed?

Add SPM device bindings for QCOM 8084 based cpus. SPM is the sub-system power manager and controls the logic around the cores (cpu and L2).

Each core has an instance of SPM and controls only that core. Each cpu SPM is configured to support WFI and SPC (standalone-power collapse).

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

Based on work by many authors, available at codeaurora.org

Add interface layer to abstract and handle hardware specific functionality for executing various cpu low power modes in QCOM chipsets.

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

+#define SCM_L2_OFF		(0x1)
+
+

(quoted diff) +2 -0

+
+#endif  /* __QCOM_PM_H */

Based on work by many authors, available at codeaurora.org

Add interface layer to abstract and handle hardware specific functionality for executing various cpu low power modes in QCOM chipsets.

QCOM cpus support multiple low power modes. The C-States are defined as -

* WFI (clock gating)
* Retention (clock gating at lower power)
* Standalone Power Collapse (Standalone PC or SPC) - The power to the cpu is lost and the cpu warmboots.
* Power Collapse (PC) - Same as SPC, but is a cognizant of the fact that the SoC may do deeper sleep modes.

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

[lina: simplify the driver for an initial submission, add commit text description of idle states]

---
 drivers/soc/qcom/Makefile |   2 +-
 drivers/soc/qcom/pm.c     | 109 ++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/qcom/pm.h     |  26 +++++++++++
 3 files changed, 136 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/qcom/pm.c
 create mode 100644 include/soc/qcom/pm.h

drivers/soc/qcom/Makefile +1 -1

index 20b329f..19900ed 100644
@@ -1,4 +1,4 @@
 obj-$(CONFIG_QCOM_GSBI)	+=	qcom_gsbi.o
-obj-$(CONFIG_QCOM_PM)	+=	spm.o
+obj-$(CONFIG_QCOM_PM)	+=	spm.o pm.o
 CFLAGS_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
 obj-$(CONFIG_QCOM_SCM) += scm.o scm-boot.o

drivers/soc/qcom/pm.c new file +44 -0

new file mode 100644
index 0000000..a2f7d72
@@ -0,0 +1,109 @@
+/* Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+
+#include <asm/proc-fns.h>
+#include <asm/suspend.h>
+
+#include <soc/qcom/pm.h>
+#include <soc/qcom/scm.h>
+#include <soc/qcom/scm-boot.h>
+#include <soc/qcom/spm.h>
+
+#define SCM_CMD_TERMINATE_PC	(0x2)
+#define SCM_FLUSH_FLAG_MASK	(0x3)
+#define SCM_L2_ON 		(0x0)
+#define SCM_L2_OFF		(0x1)
+
+
+static int set_up_boot_address(void *entry, int cpu)
+{
+	static int flags[NR_CPUS] = {
+		SCM_FLAG_WARMBOOT_CPU0,
+		SCM_FLAG_WARMBOOT_CPU1,
+		SCM_FLAG_WARMBOOT_CPU2,
+		SCM_FLAG_WARMBOOT_CPU3,
+	};
+	static DEFINE_PER_CPU(void *, last_known_entry);
+
+	if (entry == per_cpu(last_known_entry, cpu))
+		return 0;
+

drivers/soc/qcom/pm.c new file +15 -0

+		return ret;
+	}
+
+	flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
+	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
+
+	return 0;
+}
+
+/**
+ * qcom_cpu_pm_enter_sleep(): Enter a low power mode on current cpu
+ *
+ * @mode - sleep mode to enter
+ *
+ * The code should be called with interrupts disabled and on the core on

drivers/soc/qcom/pm.c new file +9 -0

+		ret = cpu_suspend(0, qcom_pm_collapse);
+		break;
+	default:
+	case PM_SLEEP_MODE_WFI:
+		qcom_spm_set_low_power_mode(SPM_MODE_CLOCK_GATING);
+		ret = cpu_do_idle();
+		break;
+	}
+

include/soc/qcom/pm.h new file +16 -0

new file mode 100644
index 0000000..563b9c3
@@ -0,0 +1,26 @@
+/*
+ * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __QCOM_PM_H
+#define __QCOM_PM_H

Quoted 6 lines · Lina Iyer

Based on work by many authors, available at codeaurora.org

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

[lina: simplify the driver for an initial submission, add commit text description of idle states]

Maintainer tags don't really make sense unless there is another author.

+

drivers/soc/qcom/pm.c line 54

Quoted 6 lines

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

+static int qcom_pm_collapse(unsigned long int unused)
+{
+	int ret;
+	u32 flag;
+
+	ret = set_up_boot_address(cpu_resume, raw_smp_processor_id());

Preemption better be off here, so why are we using raw_smp_processor_id()?

drivers/soc/qcom/pm.c line 57

Quoted 3 lines

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

+	if (ret) {
+		pr_err("Failed to set warm boot address for cpu %d\n",
+				raw_smp_processor_id());

Stuff cpu into a local variable?

Quoted 20 lines

drivers/soc/qcom/pm.c new file +20 -0

+		return ret;
+	}
+
+	flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
+	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
+
+	return 0;
+}
+
+/**
+ * qcom_cpu_pm_enter_sleep(): Enter a low power mode on current cpu
+ *
+ * @mode - sleep mode to enter
+ *
+ * The code should be called with interrupts disabled and on the core on
+ * which the low power mode is to be executed.
+ *
+ */
+static int qcom_cpu_pm_enter_sleep(enum pm_sleep_mode mode)
+{

drivers/soc/qcom/pm.c line 82

Quoted 5 lines

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

+	int ret;
+
+	switch (mode) {
+	case PM_SLEEP_MODE_SPC:
+		qcom_spm_set_low_power_mode(SPM_MODE_POWER_COLLAPSE);

Isn't it a one-to-one between PM_SLEEP_MODE_SPC and SPM_MODE_POWER_COLLAPSE? The enum to enum map seems overly complicated.

Quoted 13 lines

drivers/soc/qcom/pm.c new file +13 -0

+		ret = cpu_suspend(0, qcom_pm_collapse);
+		break;
+	default:
+	case PM_SLEEP_MODE_WFI:
+		qcom_spm_set_low_power_mode(SPM_MODE_CLOCK_GATING);
+		ret = cpu_do_idle();
+		break;
+	}
+
+	local_irq_enable();
+
+	return ret;
+}

drivers/soc/qcom/pm.c line 101

Quoted 6 lines

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

+
+static struct platform_device qcom_cpuidle_device = {
+	.name              = "qcom_cpuidle",
+	.id                = -1,
+	.dev.platform_data = qcom_cpu_pm_enter_sleep,
+};

This doesn't need to be static. Use platform_device_register_full() instead.

drivers/soc/qcom/pm.c line 109

Quoted 8 lines

drivers/soc/qcom/pm.c new file +8 -0

+
+static int __init qcom_pm_device_init(void)
+{
+	platform_device_register(&qcom_cpuidle_device);
+
+	return 0;
+}
+device_initcall(qcom_pm_device_init);

modules?

Quoted 22 lines

include/soc/qcom/pm.h new file +16 -0

new file mode 100644
index 0000000..563b9c3
@@ -0,0 +1,26 @@
+/*
+ * Copyright (c) 2009-2014, The Linux Foundation. All rights reserved.
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __QCOM_PM_H
+#define __QCOM_PM_H

include/soc/qcom/pm.h line 26

Quoted 10 lines

include/soc/qcom/pm.h new file +10 -0

+
+enum pm_sleep_mode {
+	PM_SLEEP_MODE_WFI,
+	PM_SLEEP_MODE_RET,
+	PM_SLEEP_MODE_SPC,
+	PM_SLEEP_MODE_PC,
+	PM_SLEEP_MODE_NR,
+};
+
+#endif  /* __QCOM_PM_H */

Hopefully this header is not necessary.

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Quoted 8 lines · Stephen Boyd

Quoted 6 lines · Lina Iyer

Based on work by many authors, available at codeaurora.org

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

[lina: simplify the driver for an initial submission, add commit text description of idle states]

Quoted 2 lines

Maintainer tags don't really make sense unless there is another author.

Hmm.. Since this patch is a derivative work, I wanted to clarify, what changed seems important. The work was done by many authors. Adding signed-off from everybody who could have contributed to the patch downstream is confusing.
I would be okay removing it.

Quoted 8 lines · Stephen Boyd

Quoted 6 lines · Lina Iyer

Based on work by many authors, available at codeaurora.org

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

[lina: simplify the driver for an initial submission, add commit text description of idle states]

Quoted 2 lines

Maintainer tags don't really make sense unless there is another author.

Hmm.. Since this patch is a derivative work, I wanted to clarify, what changed seems important. The work was done by many authors. Adding signed-off from everybody who could have contributed to the patch downstream is confusing.
I would be okay removing it.

Quoted 1 line

+

[...]

Quoted 10 lines

drivers/soc/qcom/pm.c line 54

Quoted 6 lines

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

+static int qcom_pm_collapse(unsigned long int unused)
+{
+    int ret;
+    u32 flag;
+
+    ret = set_up_boot_address(cpu_resume, raw_smp_processor_id());

Preemption better be off here, so why are we using raw_smp_processor_id()?

True, so raw_ returns without premeption disable, no?

Quoted 21 lines

Quoted 21 lines

drivers/soc/qcom/pm.c new file +15 -0

+        return ret;
+    }
+
+    flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
+    scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
+
+    return 0;
+}
+
+/**
+ * qcom_cpu_pm_enter_sleep(): Enter a low power mode on current cpu
+ *
+ * @mode - sleep mode to enter
+ *
+ * The code should be called with interrupts disabled and on the

core on
+ * which the low power mode is to be executed.
+ *
+ */

(quoted diff) +2 -0

+static int qcom_cpu_pm_enter_sleep(enum pm_sleep_mode mode)
+{

Quoted 9 lines

drivers/soc/qcom/pm.c line 82

Quoted 5 lines

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

+    int ret;
+
+    switch (mode) {
+    case PM_SLEEP_MODE_SPC:
+        qcom_spm_set_low_power_mode(SPM_MODE_POWER_COLLAPSE);

Isn't it a one-to-one between PM_SLEEP_MODE_SPC and SPM_MODE_POWER_COLLAPSE? The enum to enum map seems overly complicated.

Not really. SPM modes, differ when the idle state has to notify RPM. It does not have 2 enums for those modes, but uses an overloaded enum with an additional flag.

Quoted 11 lines

drivers/soc/qcom/pm.c line 109

Quoted 8 lines

drivers/soc/qcom/pm.c new file +8 -0

+
+static int __init qcom_pm_device_init(void)
+{
+    platform_device_register(&qcom_cpuidle_device);
+
+    return 0;
+}
+device_initcall(qcom_pm_device_init);

modules?

Why? An earlier initialization helps with power saving

drivers/soc/qcom/pm.c new file +52 -0

new file mode 100644
index 0000000..a2f7d72
@@ -0,0 +1,109 @@
+/* Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+
+#include <asm/proc-fns.h>
+#include <asm/suspend.h>
+
+#include <soc/qcom/pm.h>
+#include <soc/qcom/scm.h>
+#include <soc/qcom/scm-boot.h>
+#include <soc/qcom/spm.h>
+
+#define SCM_CMD_TERMINATE_PC	(0x2)
+#define SCM_FLUSH_FLAG_MASK	(0x3)
+#define SCM_L2_ON 		(0x0)
+#define SCM_L2_OFF		(0x1)
+
+
+static int set_up_boot_address(void *entry, int cpu)
+{
+	static int flags[NR_CPUS] = {
+		SCM_FLAG_WARMBOOT_CPU0,
+		SCM_FLAG_WARMBOOT_CPU1,
+		SCM_FLAG_WARMBOOT_CPU2,
+		SCM_FLAG_WARMBOOT_CPU3,
+	};
+	static DEFINE_PER_CPU(void *, last_known_entry);
+
+	if (entry == per_cpu(last_known_entry, cpu))
+		return 0;
+
+	per_cpu(last_known_entry, cpu) = entry;
+	return scm_set_boot_addr(virt_to_phys(entry), flags[cpu]);
+}
+
+static int qcom_pm_collapse(unsigned long int unused)
+{
+	int ret;
+	u32 flag;

drivers/soc/qcom/pm.c new file +16 -0

+
+	return 0;
+}
+
+/**
+ * qcom_cpu_pm_enter_sleep(): Enter a low power mode on current cpu
+ *
+ * @mode - sleep mode to enter
+ *
+ * The code should be called with interrupts disabled and on the core on
+ * which the low power mode is to be executed.
+ *
+ */
+static int qcom_cpu_pm_enter_sleep(enum pm_sleep_mode mode)
+{
+	int ret;

On Sat, Sep 27, 2014 at 01:58:12AM +0100, Lina Iyer wrote:

[...]

Quoted 15 lines

drivers/soc/qcom/pm.c new file +9 -0

new file mode 100644
index 0000000..a2f7d72
@@ -0,0 +1,109 @@
+static int qcom_pm_collapse(unsigned long int unused)
+{
+	int ret;
+	u32 flag;
+
+	ret = set_up_boot_address(cpu_resume, raw_smp_processor_id());
+	if (ret) {
+		pr_err("Failed to set warm boot address for cpu %d\n",
+				raw_smp_processor_id());

drivers/soc/qcom/pm.c line 62

Quoted 5 lines

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

+		return ret;
+	}
+
+	flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
+	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);

Function call above does not return IIUC (ie it returns through cpu_resume), add a comment to clarify please.

Quoted 16 lines

drivers/soc/qcom/pm.c new file +16 -0

+
+	return 0;
+}
+
+/**
+ * qcom_cpu_pm_enter_sleep(): Enter a low power mode on current cpu
+ *
+ * @mode - sleep mode to enter
+ *
+ * The code should be called with interrupts disabled and on the core on
+ * which the low power mode is to be executed.
+ *
+ */
+static int qcom_cpu_pm_enter_sleep(enum pm_sleep_mode mode)
+{
+	int ret;

drivers/soc/qcom/pm.c line 88

Quoted 10 lines

drivers/soc/qcom/pm.c new file +10 -0

+
+	switch (mode) {
+	case PM_SLEEP_MODE_SPC:
+		qcom_spm_set_low_power_mode(SPM_MODE_POWER_COLLAPSE);
+		ret = cpu_suspend(0, qcom_pm_collapse);
+		break;
+	default:
+	case PM_SLEEP_MODE_WFI:
+		qcom_spm_set_low_power_mode(SPM_MODE_CLOCK_GATING);
+		ret = cpu_do_idle();

Hmmm...are we sure the ret value you get from cpu_do_idle() is correct ?

Maybe I am missing something but I do not think that the cpu_do_idle implementation (in assembly files mm/proc-* called through cpu_do_idle() macro) returns a proper value, it seems to me that it "returns" whatever is present in r0 at the time of the call, which might not be what you want.

Am I missing something here ? I do not think you should rely on the cpu_do_idle() return value, at least for the moment.

drivers/soc/qcom/pm.c line 92

Quoted 4 lines

drivers/soc/qcom/pm.c new file +4 -0

+		break;
+	}
+
+	local_irq_enable();

This looks wrong to me. You end up running the CPU PM notifiers with irqs enabled, among other issues.

drivers/cpuidle/cpuidle-qcom.c new file +4 -0

+	},
+};
+
+module_platform_driver(qcom_cpuidle_plat_driver);

Add cpuidle driver interface to allow cpus to go into C-States. Use the cpuidle DT interface common across ARM architectures to provide the C-State information to the cpuidle framework.

Hi Lina

One sections mismatch warning here as well:

WARNING: drivers/cpuidle/built-in.o(.text+0x2740): Section mismatch in reference from the function qcom_cpuidle_probe() to the (unknown reference) .init.rodata:(unknown)
The function qcom_cpuidle_probe() references
the (unknown reference) __initconst (unknown).
This is often because qcom_cpuidle_probe lacks a __initconst annotation or the annotation of (unknown) is wrong.

Quoted 5 lines

Add cpuidle driver interface to allow cpus to go into C-States. Use the cpuidle DT interface common across ARM architectures to provide the C-State information to the cpuidle framework.

..

drivers/cpuidle/cpuidle-qcom.c line 50

Quoted 3 lines

drivers/cpuidle/cpuidle-qcom.c new file +3 -0

+};
+
+static const struct of_device_id qcom_idle_state_match[] __initconst = {

This is causing it.

drivers/cpuidle/cpuidle-qcom.c line 55

Quoted 5 lines

drivers/cpuidle/cpuidle-qcom.c new file +5 -0

+	{ .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi },
+	{ .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc },
+	{ },
+};
+

Thanks
Pramod

Add cpuidle driver interface to allow cpus to go into C-States. Use the cpuidle DT interface common across ARM architectures to provide the C-State information to the cpuidle framework.

Supported modes at this time are clock gating (wfi) and cpu power down (Standalone PC or spc).

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

---
 .../bindings/arm/msm/qcom,idle-state.txt           | 72 +++++++++++++++++
 drivers/cpuidle/Kconfig.arm                        |  7 ++
 drivers/cpuidle/Makefile                           |  1 +
 drivers/cpuidle/cpuidle-qcom.c                     | 89 ++++++++++++++++++++++
 4 files changed, 169 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
 create mode 100644 drivers/cpuidle/cpuidle-qcom.c

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +3 -0

new file mode 100644
index 0000000..47095b9
@@ -0,0 +1,72 @@
+QCOM Idle States for cpuidle driver
+
+ARM provides idle-state node to define the cpuidle states, as defined in [1].

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +14 -0

+cpuidle. The devicetree representation of the idle state should be -
+
+Required properties:
+
+- compatible: Must be "arm,idle-state"
+		and one of -
+			"qcom,idle-state-wfi",
+			"qcom,idle-state-ret",
+			"qcom,idle-state-spc",
+			"qcom,idle-state-pc",
+
+Other required and optional properties are specified in [1].
+
+[1]. Documentation/devicetree/bindings/arm/idle-states.txt

drivers/cpuidle/Kconfig.arm +7 -0

index 38cff69..6a9ee12 100644
@@ -62,3 +62,10 @@ config ARM_MVEBU_V7_CPUIDLE
 	depends on ARCH_MVEBU
 	help
 	  Select this to enable cpuidle on Armada 370, 38x and XP processors.
+
+config ARM_QCOM_CPUIDLE
+	bool "CPU Idle drivers for Qualcomm processors"
+	depends on QCOM_PM
+	select DT_IDLE_STATES
+	help
+	  Select this to enable cpuidle for QCOM processors

drivers/cpuidle/Makefile +1 -0

index 4d177b9..6c222d5 100644
@@ -17,6 +17,7 @@ obj-$(CONFIG_ARM_ZYNQ_CPUIDLE)		+= cpuidle-zynq.o
 obj-$(CONFIG_ARM_U8500_CPUIDLE)         += cpuidle-ux500.o
 obj-$(CONFIG_ARM_AT91_CPUIDLE)          += cpuidle-at91.o
 obj-$(CONFIG_ARM_EXYNOS_CPUIDLE)        += cpuidle-exynos.o
+obj-$(CONFIG_ARM_QCOM_CPUIDLE)		+= cpuidle-qcom.o
 
 ###############################################################################
 # MIPS drivers

drivers/cpuidle/cpuidle-qcom.c new file +40 -0

new file mode 100644
index 0000000..2fcf79a
@@ -0,0 +1,89 @@
+/*
+ * Copyright (c) 2014, Linaro Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/cpu_pm.h>
+#include <linux/cpuidle.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include <soc/qcom/pm.h>
+#include "dt_idle_states.h"
+
+static void (*qcom_idle_enter)(enum pm_sleep_mode);
+
+static int qcom_lpm_enter_wfi(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int index)
+{
+	qcom_idle_enter(PM_SLEEP_MODE_WFI);
+
+	return index;
+}
+
+static int qcom_lpm_enter_spc(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int index)
+{
+	cpu_pm_enter();
+	qcom_idle_enter(PM_SLEEP_MODE_SPC);
+	cpu_pm_exit();

drivers/cpuidle/cpuidle-qcom.c new file +28 -0

+	if (!qcom_idle_enter)
+		return -EFAULT;
+
+	 /* Probe for other states including platform WFI */
+	ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0);
+	if (ret <= 0) {
+		pr_err("%s: No cpuidle state found.\n", __func__);
+		return ret;
+	}
+
+	ret = cpuidle_register(drv, NULL);
+	if (ret) {
+		pr_err("%s: failed to register cpuidle driver\n", __func__);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver qcom_cpuidle_plat_driver = {
+	.probe	= qcom_cpuidle_probe,
+	.driver = {
+		.name = "qcom_cpuidle",
+		.owner = THIS_MODULE,
+	},
+};
+
+module_platform_driver(qcom_cpuidle_plat_driver);

Signature · 1 line

1.9.1

Hi Lina,

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 13

Quoted 8 lines · Lina Iyer

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +8 -0

+The idle states supported by the QCOM SoC are defined as -
+
+    * WFI
+    * Retention
+    * Standalone Power Collapse (Standalone PC or SPC)
+    * Power Collapse (PC)
+
+WFI: WFI does a little more in addition to architectural clock gating.  ARM

This may be misleading. Call it PlatformWFI or something like that, not WFI if that's not what it is.

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 17

Quoted 4 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +4 -0

+processors when execute the wfi instruction will gate their internal clocks.
+QCOM cpus use this instruction as a trigger for the SPM state machine. Usually
+with a cpu entering WFI, the SPM is configured to do clock-gating as well. The
+SPM state machine waits for the interrrupt to trigger the core back in to

s/interrrupt/interrupt/

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 21

Quoted 4 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +4 -0

+active. When all CPUs in the SoC, clock gate using the ARM wfi instruction, the
+second level cache usually can also clock gate sensing no cpu activity. When a
+cpu is ready to run, it needs the cache to be active before starting execution.
+Allowing the SPM to execute the clock gating statemachine and waiting for

s/statemachine/state machine/

You are defining a generic binding for Qualcomm idle states, so it should not be tied to a specific cache level (ie L2 gating), otherwise if the same state shows up in a future system with L3 you are back to square one.

"Platform WFI" or something like that ? You got what I mean.

Quoted 4 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +4 -0

+interrupt on behalf of the processor has a benefit of guaranteeing that the
+system state is conducive for the core to resume execution.
+
+Retention: Retention is a low power state where the core is clockgated and the

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 28

Quoted 3 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +3 -0

+memory and the registers associated with the core are retained.  The voltage
+may be reduced to the minimum value needed to keep the processor registers
+active. Retention is triggered when the core executes wfi instruction. The SPM

I think that saying "..is triggered when the core executes wfi instruction" is not necessary. I am not aware of any other _proper_ way of entering an ARM idle state other than executing wfi and I hope I will never have to to become aware of one.

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 34

Quoted 6 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +6 -0

+should be configured to execute the retention sequence and would wait for
+interrupt, before restoring the cpu to execution state. Retention may have a
+slightly higher latency than WFI.
+
+Standalone PC: A cpu can power down and warmboot if there is a sufficient time
+between now and the next know wake up. SPC mode is used to indicate a core

s/know/known/

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 39

Quoted 5 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +5 -0

+entering a power down state without consulting any other cpu or the system
+resources. This helps save power only on that core. Like WFI and Retention, the
+core executes wfi and the SPM programmed to do SPC would use the cpu control
+logic to power down the core's supply and restore it back when woken up by an
+interrupt.  Applying power and reseting the core causes the core to warmboot

s/reseting/resetting/

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 46

Quoted 6 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +6 -0

+back into secure mode which trampolines the control back to the kernel. To
+enter a power down state the kernel needs to call into the secure layer which
+would then execute the ARM wfi instruction. Failing to do so, would result in a
+crash enforced by the warm boot code in the secure layer. On a SoC with
+write-back L1 cache, the cache would need to be flushed.
+Power Collapse: This state is similiar to the SPC mode, but distinguishes

s/similiar/similar/

Quoted 1 line

+itself in the fact that the cpu acknowledges and permits the SoC to enter

s/in the fact that/in that/

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 51

Quoted 4 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +4 -0

+deeper sleep modes. In a hierarchical power domain SoC, this means L2 and other
+caches can be flushed, system bus, clocks - lowered, and SoC main XO turned off
+and voltages reduced, provided all cpus enter this state. In other words, it is
+a coupled idle state.  Since the span of low power modes possible at this state

Careful with the wording here. "Coupled" idle states are defined in the kernel for systems where the CPUs must enter power down with a specific ordering. I do not think "Power Collapse" is a "coupled" state from this perspective, it seems to me more like a "cluster" state, a state that is entered when all (not necessarily coordinated) CPUs in the cluster enter that state. Feel free to call it a hierarchical state, if it makes sense to you.

Quoted 6 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +6 -0

+is vast, the exit latency and the residency of this low power mode would be
+considered high even though at a cpu level, this essentially is cpu power down.
+The SPM in this state also may handshake with the Resource power manager
+processor in the SoC to indicate a complete subsystem shut down.
+
+The idle-state for QCOM SoCs are distinguished by the compatible property of

Quoted 1 line

+the node. They indicate to the cpuidle driver the entry point to use for

What node ? Please be specific. Moreover, the compatible string has a standard DT meaning and it does not indicate anything. This is a DT idle states binding and that's where it should stop, anything else is a kernel implementation detail, or put it differently, it must not define how the kernel translates that compatible property into data structures/control code.

Quoted 12 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +12 -0

+cpuidle. The devicetree representation of the idle state should be -
+
+Required properties:
+
+- compatible: Must be "arm,idle-state"
+		and one of -
+			"qcom,idle-state-wfi",
+			"qcom,idle-state-ret",
+			"qcom,idle-state-spc",
+			"qcom,idle-state-pc",
+
+static struct cpuidle_driver qcom_cpuidle_driver = {

drivers/cpuidle/cpuidle-qcom.c line 50

Quoted 5 lines

drivers/cpuidle/cpuidle-qcom.c new file +5 -0

+	.name	= "qcom_cpuidle",
+	.owner	= THIS_MODULE,
+};
+
+static const struct of_device_id qcom_idle_state_match[] __initconst = {

If it can be built as a module, __initconst should be removed (and that's true for my Exynos patch too).

Quoted 7 lines

drivers/cpuidle/cpuidle-qcom.c new file +7 -0

+	{ .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi },
+	{ .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc },
+	{ },
+};
+
+static int qcom_cpuidle_probe(struct platform_device *pdev)
+{

drivers/cpuidle/cpuidle-qcom.c line 61

Quoted 4 lines

drivers/cpuidle/cpuidle-qcom.c new file +4 -0

+	struct cpuidle_driver *drv = &qcom_cpuidle_driver;
+	int ret;
+
+	qcom_idle_enter = (void *)(pdev->dev.platform_data);

Casting a void* to a void* does not seem that useful to me, and that's valid for other CPUidle drivers too, the cast can be removed unless you explain to me what it is for.

Quoted 19 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 17

Quoted 4 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +4 -0

+processors when execute the wfi instruction will gate their internal clocks.
+QCOM cpus use this instruction as a trigger for the SPM state machine. Usually
+with a cpu entering WFI, the SPM is configured to do clock-gating as well. The
+SPM state machine waits for the interrrupt to trigger the core back in to

s/interrrupt/interrupt/

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 21

Quoted 4 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +4 -0

+active. When all CPUs in the SoC, clock gate using the ARM wfi instruction, the
+second level cache usually can also clock gate sensing no cpu activity. When a
+cpu is ready to run, it needs the cache to be active before starting execution.
+Allowing the SPM to execute the clock gating statemachine and waiting for

s/statemachine/state machine/

You are defining a generic binding for Qualcomm idle states, so it should not be tied to a specific cache level (ie L2 gating), otherwise if the same state shows up in a future system with L3 you are back to square one.

Quoted 2 lines

"Platform WFI" or something like that ? You got what I mean.

I am not, I am just explaining the difference between Architectural and Platform WFI and how the WFI on the core, can help L2 enter shallower idle states, which is true for L3 as well (provided there is enough time and we are not allowed to do power down states).

Quoted 11 lines

drivers/cpuidle/cpuidle-qcom.c line 61

Quoted 6 lines

drivers/cpuidle/cpuidle-qcom.c new file +6 -0

+static int qcom_cpuidle_probe(struct platform_device *pdev)
+{
+	struct cpuidle_driver *drv = &qcom_cpuidle_driver;
+	int ret;
+
+	qcom_idle_enter = (void *)(pdev->dev.platform_data);

Casting a void* to a void* does not seem that useful to me, and that's valid for other CPUidle drivers too, the cast can be removed unless you explain to me what it is for.

Sure, I dont need it.

Thanks for your time, Lorenzo.

Hi Lina,

Quoted 31 lines · Lina Iyer

Add cpuidle driver interface to allow cpus to go into C-States. Use the cpuidle DT interface common across ARM architectures to provide the C-State information to the cpuidle framework.

Supported modes at this time are clock gating (wfi) and cpu power down (Standalone PC or spc).

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

---
 .../bindings/arm/msm/qcom,idle-state.txt           | 72 +++++++++++++++++
 drivers/cpuidle/Kconfig.arm                        |  7 ++
 drivers/cpuidle/Makefile                           |  1 +
 drivers/cpuidle/cpuidle-qcom.c                     | 89 ++++++++++++++++++++++
 4 files changed, 169 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
 create mode 100644 drivers/cpuidle/cpuidle-qcom.c

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +8 -0

new file mode 100644
index 0000000..47095b9
@@ -0,0 +1,72 @@
+QCOM Idle States for cpuidle driver
+
+ARM provides idle-state node to define the cpuidle states, as defined in [1].
+cpuidle-qcom is the cpuidle driver for Qualcomm SoCs and uses these idle
+states. Idle states have different enter/exit latency and residency values.
+The idle states supported by the QCOM SoC are defined as -
+
+    * WFI

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 13

Quoted 5 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +5 -0

+    * Retention
+    * Standalone Power Collapse (Standalone PC or SPC)
+    * Power Collapse (PC)
+
+WFI: WFI does a little more in addition to architectural clock gating.  ARM

This may be misleading. Call it PlatformWFI or something like that, not WFI if that's not what it is.

Quoted 34 lines · Lorenzo Pieralisi

Hi Lina,

Quoted 31 lines · Lina Iyer

Add cpuidle driver interface to allow cpus to go into C-States. Use the cpuidle DT interface common across ARM architectures to provide the C-State information to the cpuidle framework.

Supported modes at this time are clock gating (wfi) and cpu power down (Standalone PC or spc).

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

---
 .../bindings/arm/msm/qcom,idle-state.txt           | 72 +++++++++++++++++
 drivers/cpuidle/Kconfig.arm                        |  7 ++
 drivers/cpuidle/Makefile                           |  1 +
 drivers/cpuidle/cpuidle-qcom.c                     | 89 ++++++++++++++++++++++
 4 files changed, 169 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
 create mode 100644 drivers/cpuidle/cpuidle-qcom.c

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +8 -0

new file mode 100644
index 0000000..47095b9
@@ -0,0 +1,72 @@
+QCOM Idle States for cpuidle driver
+
+ARM provides idle-state node to define the cpuidle states, as defined in [1].
+cpuidle-qcom is the cpuidle driver for Qualcomm SoCs and uses these idle
+states. Idle states have different enter/exit latency and residency values.
+The idle states supported by the QCOM SoC are defined as -
+
+    * WFI

Quoted 8 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 13

Quoted 5 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +5 -0

+    * Retention
+    * Standalone Power Collapse (Standalone PC or SPC)
+    * Power Collapse (PC)
+
+WFI: WFI does a little more in addition to architectural clock gating.  ARM

This may be misleading. Call it PlatformWFI or something like that, not WFI if that's not what it is.

This gets at a little pet peeve of mine:

IMO, naming any state with "WFI" is a bit confusing, because typically *every* idle state is entered by one (or more) CPU executing WFI, no?

Quoted 47 lines · Kevin Hilman

Quoted 34 lines · Lorenzo Pieralisi

Hi Lina,

Quoted 31 lines · Lina Iyer

Add cpuidle driver interface to allow cpus to go into C-States. Use the cpuidle DT interface common across ARM architectures to provide the C-State information to the cpuidle framework.

Supported modes at this time are clock gating (wfi) and cpu power down (Standalone PC or spc).

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

---
 .../bindings/arm/msm/qcom,idle-state.txt           | 72 +++++++++++++++++
 drivers/cpuidle/Kconfig.arm                        |  7 ++
 drivers/cpuidle/Makefile                           |  1 +
 drivers/cpuidle/cpuidle-qcom.c                     | 89 ++++++++++++++++++++++
 4 files changed, 169 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
 create mode 100644 drivers/cpuidle/cpuidle-qcom.c

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +8 -0

new file mode 100644
index 0000000..47095b9
@@ -0,0 +1,72 @@
+QCOM Idle States for cpuidle driver
+
+ARM provides idle-state node to define the cpuidle states, as defined in [1].
+cpuidle-qcom is the cpuidle driver for Qualcomm SoCs and uses these idle
+states. Idle states have different enter/exit latency and residency values.
+The idle states supported by the QCOM SoC are defined as -
+
+    * WFI

Quoted 8 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 13

Quoted 5 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +5 -0

+    * Retention
+    * Standalone Power Collapse (Standalone PC or SPC)
+    * Power Collapse (PC)
+
+WFI: WFI does a little more in addition to architectural clock gating.  ARM

This may be misleading. Call it PlatformWFI or something like that, not WFI if that's not what it is.

This gets at a little pet peeve of mine:

Quoted 2 lines

IMO, naming any state with "WFI" is a bit confusing, because typically *every* idle state is entered by one (or more) CPU executing WFI, no?

Agreed.

The only state called "WFI" should be the one that only executes the WFI instruction without any other hardware setup around it.

Quoted 52 lines · Nicolas Pitre

Quoted 49 lines · Kevin Hilman

Quoted 34 lines · Lorenzo Pieralisi

Hi Lina,

Quoted 31 lines · Lina Iyer

Add cpuidle driver interface to allow cpus to go into C-States. Use the cpuidle DT interface common across ARM architectures to provide the C-State information to the cpuidle framework.

Supported modes at this time are clock gating (wfi) and cpu power down (Standalone PC or spc).

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

---
 .../bindings/arm/msm/qcom,idle-state.txt           | 72 +++++++++++++++++
 drivers/cpuidle/Kconfig.arm                        |  7 ++
 drivers/cpuidle/Makefile                           |  1 +
 drivers/cpuidle/cpuidle-qcom.c                     | 89 ++++++++++++++++++++++
 4 files changed, 169 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
 create mode 100644 drivers/cpuidle/cpuidle-qcom.c

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +8 -0

new file mode 100644
index 0000000..47095b9
@@ -0,0 +1,72 @@
+QCOM Idle States for cpuidle driver
+
+ARM provides idle-state node to define the cpuidle states, as defined in [1].
+cpuidle-qcom is the cpuidle driver for Qualcomm SoCs and uses these idle
+states. Idle states have different enter/exit latency and residency values.
+The idle states supported by the QCOM SoC are defined as -
+
+    * WFI

Quoted 8 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt line 13

Quoted 5 lines

Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt new file +5 -0

+    * Retention
+    * Standalone Power Collapse (Standalone PC or SPC)
+    * Power Collapse (PC)
+
+WFI: WFI does a little more in addition to architectural clock gating.  ARM

This may be misleading. Call it PlatformWFI or something like that, not WFI if that's not what it is.

This gets at a little pet peeve of mine:

IMO, naming any state with "WFI" is a bit confusing, because typically *every* idle state is entered by one (or more) CPU executing WFI, no?

Quoted 4 lines

Agreed.

The only state called "WFI" should be the one that only executes the WFI instruction without any other hardware setup around it.

Well, I would go even further in that none of the states should be called WFI, because WFI is used to enter all of them.

Quoted 5 lines · Kevin Hilman

Quoted 3 lines · Nicolas Pitre

Quoted 1 line · Kevin Hilman

Lorenzo Pieralisi <[email protected]> writes:

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

This may be misleading. Call it PlatformWFI or something like that, not WFI if that's not what it is.

This gets at a little pet peeve of mine:

IMO, naming any state with "WFI" is a bit confusing, because typically *every* idle state is entered by one (or more) CPU executing WFI, no?

Quoted 7 lines

Quoted 4 lines

Agreed.

The only state called "WFI" should be the one that only executes the WFI instruction without any other hardware setup around it.

Well, I would go even further in that none of the states should be called WFI, because WFI is used to enter all of them.

Fair enough.

So let's fix this by finding a name for that state that consists of only executing WFI and that every SOC has.

Suggestions?

Quoted 7 lines · Nicolas Pitre

Quoted 5 lines · Kevin Hilman

Quoted 3 lines · Nicolas Pitre

Quoted 1 line · Kevin Hilman

Lorenzo Pieralisi <[email protected]> writes:

[...]

Quoted 18 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

This may be misleading. Call it PlatformWFI or something like that, not WFI if that's not what it is.

This gets at a little pet peeve of mine:

IMO, naming any state with "WFI" is a bit confusing, because typically *every* idle state is entered by one (or more) CPU executing WFI, no?

Quoted 7 lines

Quoted 4 lines

Agreed.

The only state called "WFI" should be the one that only executes the WFI instruction without any other hardware setup around it.

Well, I would go even further in that none of the states should be called WFI, because WFI is used to enter all of them.

Fair enough.

Quoted 4 lines

So let's fix this by finding a name for that state that consists of only executing WFI and that every SOC has.

Suggestions?

The DT idle-states binding doc (though seemingly written more with arm64 and SBSA in mind) uses "standby" for the shallowest idle.

drivers/cpuidle/cpuidle-qcom.c new file +20 -0

new file mode 100644
index 0000000..2fcf79a
@@ -0,0 +1,89 @@
+/*
+ * Copyright (c) 2014, Linaro Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/cpu_pm.h>
+#include <linux/cpuidle.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>

drivers/cpuidle/cpuidle-qcom.c new file +23 -0

+
+	return index;
+}
+
+static int qcom_lpm_enter_spc(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int index)
+{
+	cpu_pm_enter();
+	qcom_idle_enter(PM_SLEEP_MODE_SPC);
+	cpu_pm_exit();
+
+	return index;
+}
+
+static struct cpuidle_driver qcom_cpuidle_driver = {
+	.name	= "qcom_cpuidle",
+	.owner	= THIS_MODULE,
+};
+
+static const struct of_device_id qcom_idle_state_match[] __initconst = {
+	{ .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi },
+	{ .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc },
+	{ },

Quoted 31 lines · Lina Iyer

drivers/cpuidle/cpuidle-qcom.c new file +25 -0

new file mode 100644
index 0000000..2fcf79a
@@ -0,0 +1,89 @@
+/*
+ * Copyright (c) 2014, Linaro Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/cpu_pm.h>
+#include <linux/cpuidle.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include <soc/qcom/pm.h>
+#include "dt_idle_states.h"
+
+static void (*qcom_idle_enter)(enum pm_sleep_mode);

drivers/cpuidle/cpuidle-qcom.c line 30

Quoted 5 lines

drivers/cpuidle/cpuidle-qcom.c new file +5 -0

+
+static int qcom_lpm_enter_wfi(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int index)
+{
+	qcom_idle_enter(PM_SLEEP_MODE_WFI);

Why can't we just pass index here? It would be nice to not need to include soc/qcom/pm.h in this file.

Quoted 22 lines

drivers/cpuidle/cpuidle-qcom.c new file +22 -0

+	return index;
+}
+
+static int qcom_lpm_enter_spc(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int index)
+{
+	cpu_pm_enter();
+	qcom_idle_enter(PM_SLEEP_MODE_SPC);
+	cpu_pm_exit();
+
+	return index;
+}
+
+static struct cpuidle_driver qcom_cpuidle_driver = {
+	.name	= "qcom_cpuidle",
+	.owner	= THIS_MODULE,
+};
+
+static const struct of_device_id qcom_idle_state_match[] __initconst = {
+	{ .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi },
+	{ .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc },
+	{ },

drivers/cpuidle/cpuidle-qcom.c line 63

Quoted 10 lines

drivers/cpuidle/cpuidle-qcom.c new file +10 -0

+};
+
+static int qcom_cpuidle_probe(struct platform_device *pdev)
+{
+	struct cpuidle_driver *drv = &qcom_cpuidle_driver;
+	int ret;
+
+	qcom_idle_enter = (void *)(pdev->dev.platform_data);
+	if (!qcom_idle_enter)
+		return -EFAULT;

Error code looks a little wrong. -ENODEV?

drivers/cpuidle/cpuidle-qcom.c line 68

Quoted 5 lines

drivers/cpuidle/cpuidle-qcom.c new file +5 -0

+
+	 /* Probe for other states including platform WFI */
+	ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0);
+	if (ret <= 0) {
+		pr_err("%s: No cpuidle state found.\n", __func__);

This would be true if ret == 0, but if it's < 0 that isn't true. Drop the print?

drivers/cpuidle/cpuidle-qcom.c line 74

Quoted 6 lines

drivers/cpuidle/cpuidle-qcom.c new file +6 -0

+		return ret;
+	}
+
+	ret = cpuidle_register(drv, NULL);
+	if (ret) {
+		pr_err("%s: failed to register cpuidle driver\n", __func__);

This seems redundant given that cpuidle_register() already prints an error when it fails.

Quoted 2 lines · Stephen Boyd

Quoted 1 line · Lina Iyer

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

Quoted 7 lines

+static int qcom_lpm_enter_wfi(struct cpuidle_device *dev,

drivers/cpuidle/cpuidle-qcom.c line 30

Quoted 3 lines

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

+				struct cpuidle_driver *drv, int index)
+{
+	qcom_idle_enter(PM_SLEEP_MODE_WFI);

Why can't we just pass index here? It would be nice to not need to include soc/qcom/pm.h in this file.

I think this is definitely worth exploring.

Quoted 15 lines

Quoted 15 lines

drivers/cpuidle/cpuidle-qcom.c +15 -0

+	return index;
+}
+
+static int qcom_lpm_enter_spc(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int index)
+{
+	cpu_pm_enter();
+	qcom_idle_enter(PM_SLEEP_MODE_SPC);
+	cpu_pm_exit();
+
+	return index;
+}
+
+static struct cpuidle_driver qcom_cpuidle_driver = {
+	.name	= "qcom_cpuidle",

Quoted 8 lines

drivers/cpuidle/cpuidle-qcom.c line 54

Quoted 8 lines

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

+	.owner	= THIS_MODULE,
+};
+
+static const struct of_device_id qcom_idle_state_match[] __initconst = {
+	{ .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi },
+	{ .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc },
+	{ },
+};

Quoted 12 lines

drivers/cpuidle/cpuidle-qcom.c line 63

Quoted 9 lines

drivers/cpuidle/cpuidle-qcom.c +9 -0

+
+static int qcom_cpuidle_probe(struct platform_device *pdev)
+{
+	struct cpuidle_driver *drv = &qcom_cpuidle_driver;
+	int ret;
+
+	qcom_idle_enter = (void *)(pdev->dev.platform_data);
+	if (!qcom_idle_enter)
+		return -EFAULT;

Error code looks a little wrong. -ENODEV?

Quoted 8 lines

drivers/cpuidle/cpuidle-qcom.c line 68

Quoted 5 lines

drivers/cpuidle/cpuidle-qcom.c +5 -0

+
+	 /* Probe for other states including platform WFI */
+	ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0);
+	if (ret <= 0) {
+		pr_err("%s: No cpuidle state found.\n", __func__);

This would be true if ret == 0, but if it's < 0 that isn't true. Drop the print?

I think we can safely drop the print, DT parsing code already barfs on error. Maybe you want to keep the (ret == 0) case to signal that driver was probed but no idle states were found.

Quoted 9 lines

drivers/cpuidle/cpuidle-qcom.c line 74

Quoted 6 lines

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

+		return ret;
+	}
+
+	ret = cpuidle_register(drv, NULL);
+	if (ret) {
+		pr_err("%s: failed to register cpuidle driver\n", __func__);

This seems redundant given that cpuidle_register() already prints an error when it fails.

Yes, I will drop it from arm64 driver too as part of a minor clean-up when the merge window closes (also other drivers do that, and as you say it is redundant).

Quoted 31 lines · Lina Iyer

drivers/cpuidle/cpuidle-qcom.c new file +25 -0

new file mode 100644
index 0000000..2fcf79a
@@ -0,0 +1,89 @@
+/*
+ * Copyright (c) 2014, Linaro Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/cpu_pm.h>
+#include <linux/cpuidle.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+
+#include <soc/qcom/pm.h>
+#include "dt_idle_states.h"
+
+static void (*qcom_idle_enter)(enum pm_sleep_mode);

drivers/cpuidle/cpuidle-qcom.c line 30

Quoted 5 lines

drivers/cpuidle/cpuidle-qcom.c new file +5 -0

+
+static int qcom_lpm_enter_wfi(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int index)
+{
+	qcom_idle_enter(PM_SLEEP_MODE_WFI);

Why can't we just pass index here? It would be nice to not need to include soc/qcom/pm.h in this file.

Quoted 27 lines

drivers/cpuidle/cpuidle-qcom.c new file +27 -0

+
+	return index;
+}
+
+static int qcom_lpm_enter_spc(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int index)
+{
+	cpu_pm_enter();
+	qcom_idle_enter(PM_SLEEP_MODE_SPC);
+	cpu_pm_exit();
+
+	return index;
+}
+
+static struct cpuidle_driver qcom_cpuidle_driver = {
+	.name	= "qcom_cpuidle",
+	.owner	= THIS_MODULE,
+};
+
+static const struct of_device_id qcom_idle_state_match[] __initconst = {
+	{ .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi },
+	{ .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc },
+	{ },
+};
+
+static int qcom_cpuidle_probe(struct platform_device *pdev)
+{

drivers/cpuidle/cpuidle-qcom.c line 63

Quoted 6 lines

drivers/cpuidle/cpuidle-qcom.c new file +6 -0

+	struct cpuidle_driver *drv = &qcom_cpuidle_driver;
+	int ret;
+
+	qcom_idle_enter = (void *)(pdev->dev.platform_data);
+	if (!qcom_idle_enter)
+		return -EFAULT;

Error code looks a little wrong. -ENODEV?

drivers/cpuidle/cpuidle-qcom.c line 68

Quoted 5 lines

drivers/cpuidle/cpuidle-qcom.c new file +5 -0

+
+	 /* Probe for other states including platform WFI */
+	ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0);
+	if (ret <= 0) {
+		pr_err("%s: No cpuidle state found.\n", __func__);

This would be true if ret == 0, but if it's < 0 that isn't true. Drop the print?

drivers/cpuidle/cpuidle-qcom.c line 74

Quoted 6 lines

drivers/cpuidle/cpuidle-qcom.c new file +6 -0

+		return ret;
+	}
+
+	ret = cpuidle_register(drv, NULL);
+	if (ret) {
+		pr_err("%s: failed to register cpuidle driver\n", __func__);

This seems redundant given that cpuidle_register() already prints an error when it fails.

drivers/cpuidle/cpuidle-qcom.c line 78

Quoted 4 lines

drivers/cpuidle/cpuidle-qcom.c new file +4 -0

+		return ret;
+	}
+
+	return 0;

Could just be return cpuidle_register(drv, NULL);

Signature · 2 lines

Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Changes since v6:
[ https://www.mail-archive.com/[email protected]/msg11012.html ]
- SPM device nodes merged with existing SAW DT nodes
- SPM register information is handled within the driver
- Clean up from using 'msm' to 'qcom'
- Shorten some enumerations as well
- Review comments from v6 addressed
- New: Support for 8084 SoC
- Not tested. I do not have a board with this SoC, but the SPM configuration should be identical for WFI and SPC

Changes since v5:
[ https://www.mail-archive.com/[email protected]/msg10559.html ]
- Merge spm-devices.c and spm.c into one file and one patch
- Simplify implementation of the driver.
- Update documentation mapping the DT properties with corresponding SPM register information.
- Removed scm-boot changes for quad core warmboot, its has been pulled in.

Changes since v4:
[ https://www.mail-archive.com/[email protected]/msg10327.html ]
- Update to the v8 of ARM generic idle states patches
- Use platform device model for cpuidle-qcom
- Clean up msm-pm.c to remove unnecessary include files and functions
- Update commit text and documentation for all idle states
- Remove scm-boot relocate patch from this series, submitted earlier [ https://www.mail-archive.com/[email protected]/msg10518.html ]

Changes since v3:
[ https://www.mail-archive.com/[email protected]/msg10288.html ]
- Fix CONFIG_QCOM_PM Kconfig as bool
- More clean ups in spm.c and spm-devices.c
- Removed and re-organized data structures to make initialization simple
- Remove export of sequence flush functions
- Updated commit text
- Comments for use of barriers.
- Rebase on top of 3.17-rc1

Changes since v2:
[ https://www.mail-archive.com/[email protected]/msg10148.html ]
- Prune all the drivers to support basic WFI and power down cpuidle functionality. Remove debug code.
- Integrate KConfig changes into the drivers' patches.
- Use Lorenzo's ARM idle-states patches as the basis for reading cpuidle c-states from DT.
[ http://marc.info/?l=linux-pm&m=140794514812383&w=2 ]
- Incorporate review comments
- Rebase on top of 3.16

Changes since v1/RFC:
[ https://www.mail-archive.com/[email protected]/msg10065.html ]
- Remove hotplug from the patch series. Will submit it separately.
- Fix SPM drivers per the review comments
- Modify patch sequence to compile SPM drivers independent of msm-pm, so as to allow wfi() calls to use SPM even without SoC interface driver.

8074/8084 like any ARM SoC can do architectural clock gating, that helps save on power, but not enough of leakage power. Leakage power of the SoC can be further reduced by turning off power to the core. To aid this, every core (cpu and L2) is accompanied by a Sub-system Power Manager (SPM), that can be configured to indicate the low power mode, the core would be put into and the SPM programs the peripheral h/w accordingly to enter low power and turn off the power rail to the core.

The idle invocation hierarchy -

	CPUIDLE
	|
	cpuidle-qcom.c [CPUIdle driver]
	|
	------>	pm.c [SoC Interface layer for QCOM chipsets]
		|
		------> spm.c [SPM driver]
		|
		------> scm-boot.c [SCM interface layer]		
			|
------------------------|--------------------------
(EL)			Secure Monitor Code
			|
			|
			wfi(); 
------------------------|--------------------------
(HW)			[CPU] {clock gate}
			|
			-----> [SPM] {statemachine}

The patchset does the following -

- Introduce the SPM driver to control power to the core

- Add device bindings for 8974 CPU SPM devices

- Add device bindings for 8084 CPU SPM devices

- Introduce the SoC interface layer to configure SPM per the core's idle state.

- Add CPUIDLE driver for QCOM cpus, using ARM generic idle state definitions.

- Add device bindings for 8974 idle-states - WFI and SPC

- Add device bindings for 8084 idle-states - WFI and SPC

Thanks,
Lina

Lina Iyer (7):
qcom: spm: Add Subsystem Power Manager driver
arm: dts: qcom: Add SPM device bindings for 8974
arm: dts: qcom: Add SPM device bindings for 8084
qcom: pm: Add cpu low power mode functions
qcom: cpuidle: Add cpuidle driver for QCOM cpus
arm: dts: qcom: Add idle states device nodes for 8974
arm: dts: qcom: Add idle states device nodes for 8084

 .../bindings/arm/msm/qcom,idle-state.txt           |  72 +++++++
 .../devicetree/bindings/arm/msm/qcom,saw2.txt      |  10 +-
 arch/arm/boot/dts/qcom-apq8084.dtsi                |  46 ++++-
 arch/arm/boot/dts/qcom-msm8974.dtsi                |  46 ++++-
 drivers/cpuidle/Kconfig.arm                        |   7 +
 drivers/cpuidle/Makefile                           |   1 +
 drivers/cpuidle/cpuidle-qcom.c                     |  89 +++++++++
 drivers/soc/qcom/Kconfig                           |   8 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/pm.c                              | 109 +++++++++++
 drivers/soc/qcom/spm.c                             | 216 +++++++++++++++++++++
 include/soc/qcom/pm.h                              |  26 +++
 include/soc/qcom/spm.h                             |  35 ++++
 13 files changed, 658 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
 create mode 100644 drivers/cpuidle/cpuidle-qcom.c
 create mode 100644 drivers/soc/qcom/pm.c
 create mode 100644 drivers/soc/qcom/spm.c
 create mode 100644 include/soc/qcom/pm.h
 create mode 100644 include/soc/qcom/spm.h

Hi Lina,

Thanks for the patches. Tested these patches on my Dragonboard APQ8074 and with cpuidle tests from Linaro. They all pass.

And the proper names of the cpuidle state (wfi and spc) are also reflecting in sysfs.

Tested-by pramod.gurav at smartplayin.com.

Thanks
Pramod

Quoted 138 lines · Lina Iyer

Changes since v6:
[ https://www.mail-archive.com/linux-arm-msm at vger.kernel.org/msg11012.html ]
- SPM device nodes merged with existing SAW DT nodes
- SPM register information is handled within the driver
- Clean up from using 'msm' to 'qcom'
- Shorten some enumerations as well
- Review comments from v6 addressed
- New: Support for 8084 SoC
- Not tested. I do not have a board with this SoC, but the SPM configuration should be identical for WFI and SPC

Changes since v5:
[ https://www.mail-archive.com/linux-arm-msm at vger.kernel.org/msg10559.html ]
- Merge spm-devices.c and spm.c into one file and one patch
- Simplify implementation of the driver.
- Update documentation mapping the DT properties with corresponding SPM register information.
- Removed scm-boot changes for quad core warmboot, its has been pulled in.

Changes since v4:
[ https://www.mail-archive.com/linux-arm-msm at vger.kernel.org/msg10327.html ]
- Update to the v8 of ARM generic idle states patches
- Use platform device model for cpuidle-qcom
- Clean up msm-pm.c to remove unnecessary include files and functions
- Update commit text and documentation for all idle states
- Remove scm-boot relocate patch from this series, submitted earlier [ https://www.mail-archive.com/linux-arm-msm at vger.kernel.org/msg10518.html ]

Changes since v3:
[ https://www.mail-archive.com/linux-arm-msm at vger.kernel.org/msg10288.html ]
- Fix CONFIG_QCOM_PM Kconfig as bool
- More clean ups in spm.c and spm-devices.c
- Removed and re-organized data structures to make initialization simple
- Remove export of sequence flush functions
- Updated commit text
- Comments for use of barriers.
- Rebase on top of 3.17-rc1

Changes since v2:
[ https://www.mail-archive.com/linux-arm-msm at vger.kernel.org/msg10148.html ]
- Prune all the drivers to support basic WFI and power down cpuidle functionality. Remove debug code.
- Integrate KConfig changes into the drivers' patches.
- Use Lorenzo's ARM idle-states patches as the basis for reading cpuidle c-states from DT.
[ http://marc.info/?l=linux-pm&m=140794514812383&w=2 ]
- Incorporate review comments
- Rebase on top of 3.16

Changes since v1/RFC:
[ https://www.mail-archive.com/linux-arm-msm at vger.kernel.org/msg10065.html ]
- Remove hotplug from the patch series. Will submit it separately.
- Fix SPM drivers per the review comments
- Modify patch sequence to compile SPM drivers independent of msm-pm, so as to allow wfi() calls to use SPM even without SoC interface driver.

8074/8084 like any ARM SoC can do architectural clock gating, that helps save on power, but not enough of leakage power. Leakage power of the SoC can be further reduced by turning off power to the core. To aid this, every core (cpu and L2) is accompanied by a Sub-system Power Manager (SPM), that can be configured to indicate the low power mode, the core would be put into and the SPM programs the peripheral h/w accordingly to enter low power and turn off the power rail to the core.

The idle invocation hierarchy -

	CPUIDLE
	|
	cpuidle-qcom.c [CPUIdle driver]
	|
	------>	pm.c [SoC Interface layer for QCOM chipsets]
		|
		------> spm.c [SPM driver]
		|
		------> scm-boot.c [SCM interface layer]		
			|
------------------------|--------------------------
(EL)			Secure Monitor Code
			|
			|
			wfi();
------------------------|--------------------------
(HW)			[CPU] {clock gate}
			|
			-----> [SPM] {statemachine}

The patchset does the following -

- Introduce the SPM driver to control power to the core

- Add device bindings for 8974 CPU SPM devices

- Add device bindings for 8084 CPU SPM devices

- Introduce the SoC interface layer to configure SPM per the core's idle state.

- Add CPUIDLE driver for QCOM cpus, using ARM generic idle state definitions.

- Add device bindings for 8974 idle-states - WFI and SPC

- Add device bindings for 8084 idle-states - WFI and SPC

Thanks,
Lina

Lina Iyer (7):
qcom: spm: Add Subsystem Power Manager driver
arm: dts: qcom: Add SPM device bindings for 8974
arm: dts: qcom: Add SPM device bindings for 8084
qcom: pm: Add cpu low power mode functions
qcom: cpuidle: Add cpuidle driver for QCOM cpus
arm: dts: qcom: Add idle states device nodes for 8974
arm: dts: qcom: Add idle states device nodes for 8084

 .../bindings/arm/msm/qcom,idle-state.txt           |  72 +++++++
 .../devicetree/bindings/arm/msm/qcom,saw2.txt      |  10 +-
 arch/arm/boot/dts/qcom-apq8084.dtsi                |  46 ++++-
 arch/arm/boot/dts/qcom-msm8974.dtsi                |  46 ++++-
 drivers/cpuidle/Kconfig.arm                        |   7 +
 drivers/cpuidle/Makefile                           |   1 +
 drivers/cpuidle/cpuidle-qcom.c                     |  89 +++++++++
 drivers/soc/qcom/Kconfig                           |   8 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/pm.c                              | 109 +++++++++++
 drivers/soc/qcom/spm.c                             | 216 +++++++++++++++++++++
 include/soc/qcom/pm.h                              |  26 +++
 include/soc/qcom/spm.h                             |  35 ++++
 13 files changed, 658 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
 create mode 100644 drivers/cpuidle/cpuidle-qcom.c
 create mode 100644 drivers/soc/qcom/pm.c
 create mode 100644 drivers/soc/qcom/spm.c
 create mode 100644 include/soc/qcom/pm.h
 create mode 100644 include/soc/qcom/spm.h