[PATCH v7 0/7] QCOM 8974 and 8084 cpuidle driver

[PATCH v7 0/7] QCOM 8974 and 8084 cpuidle driver

[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

-- 
1.9.1

[PATCH v7 1/7] qcom: spm: Add Subsystem Power Manager driver

[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 <lina.iyer@linaro.org>
[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

diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
index 1505fb8..9a9cc99 100644
--- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
@@ -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"
-			 "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"
 
 - reg:
 	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:
 
-	regulator at 2099000 {
+	saw at 2099000 {
 		compatible = "qcom,saw2";
 		reg = <0x02099000 0x1000>, <0x02009000 0x1000>;
 	};
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 7dcd554..cd249c4 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -11,3 +11,11 @@ config QCOM_GSBI
 
 config QCOM_SCM
 	bool
+
+config QCOM_PM
+	bool "Qualcomm Power Management"
+	depends on PM && ARCH_QCOM
+	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.
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 70d52ed..20b329f 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -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
diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
new file mode 100644
index 0000000..0ba7949
--- /dev/null
+++ b/drivers/soc/qcom/spm.c
@@ -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},
+	.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},
+	.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},
+	.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
+ * @mode: SPM LPM mode to enter
+ */
+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;
+	u32 ctl_val;
+
+	if (!drv || !drv->reg_data)
+		return -ENXIO;
+
+	start_addr = drv->reg_data->start_addr[mode];
+
+	/* Update bits 10:4 in the SPM CTL register */
+	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) {
+		cpu_node = of_get_cpu_node(cpu, NULL);
+		if (!cpu_node)
+			continue;
+		saw_node = of_parse_phandle(cpu_node, "qcom,saw", 0);
+		if (!saw_node)
+			continue;
+		if (saw_node == pdev->dev.of_node) {
+			drv = &per_cpu(cpu_spm_drv, cpu);
+			break;
+		}
+	}
+
+	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.
+	 */
+	wmb();
+
+	return 0;
+}
+
+static struct platform_driver spm_driver = {
+	.probe = spm_dev_probe,
+	.driver = {
+		.name = "spm",
+		.of_match_table = spm_match_table,
+	},
+};
+
+static int __init spm_driver_init(void)
+{
+	return platform_driver_register(&spm_driver);
+}
+device_initcall(spm_driver_init);
diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
new file mode 100644
index 0000000..997abfc
--- /dev/null
+++ b/include/soc/qcom/spm.h
@@ -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)
+
+int qcom_spm_set_low_power_mode(enum spm_mode mode);
+
+#else
+
+static inline int qcom_spm_set_low_power_mode(enum spm_mode mode)
+{ return -ENOSYS; }
+
+#endif  /* CONFIG_QCOM_PM */
+
+#endif  /* __QCOM_SPM_H */
-- 
1.9.1

[PATCH v7 2/7] arm: dts: qcom: Add SPM device bindings for 8974

[Lina Iyer]

Add SPM device bindings for QCOM 8974 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 <lina.iyer@linaro.org>
---
 arch/arm/boot/dts/qcom-msm8974.dtsi | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index 69dca2a..828f5bb 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -21,6 +21,7 @@
 			reg = <0>;
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc0>;
+			qcom,saw = <&saw0>;
 		};
 
 		cpu at 1 {
@@ -30,6 +31,7 @@
 			reg = <1>;
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc1>;
+			qcom,saw = <&saw1>;
 		};
 
 		cpu at 2 {
@@ -39,6 +41,7 @@
 			reg = <2>;
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc2>;
+			qcom,saw = <&saw2>;
 		};
 
 		cpu at 3 {
@@ -48,6 +51,7 @@
 			reg = <3>;
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc3>;
+			qcom,saw = <&saw3>;
 		};
 
 		L2: l2-cache {
@@ -144,7 +148,27 @@
 			};
 		};
 
-		saw_l2: regulator at f9012000 {
+		saw0: saw at f9089000 {
+			compatible = "qcom,msm8974-saw2-v2.1-cpu";
+			reg = <0xf9089000 0x1000>;
+		};
+
+		saw1: saw at f9099000 {
+			compatible = "qcom,msm8974-saw2-v2.1-cpu";
+			reg = <0xf9099000 0x1000>;
+		};
+
+		saw2: saw at f90a9000 {
+			compatible = "qcom,msm8974-saw2-v2.1-cpu";
+			reg = <0xf90a9000 0x1000>;
+		};
+
+		saw3: saw at f90b9000 {
+			compatible = "qcom,msm8974-saw2-v2.1-cpu";
+			reg = <0xf90b9000 0x1000>;
+		};
+
+		saw_l2: saw at f9012000 {
 			compatible = "qcom,saw2";
 			reg = <0xf9012000 0x1000>;
 			regulator;
-- 
1.9.1

[PATCH v7 3/7] arm: dts: qcom: Add SPM device bindings for 8084

[Lina Iyer]

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 <lina.iyer@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8084.dtsi | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi
index e3e009a..3dda230 100644
--- a/arch/arm/boot/dts/qcom-apq8084.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8084.dtsi
@@ -18,6 +18,7 @@
 			enable-method = "qcom,kpss-acc-v2";
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc0>;
+			qcom,saw = <&saw0>;
 		};
 
 		cpu at 1 {
@@ -27,6 +28,7 @@
 			enable-method = "qcom,kpss-acc-v2";
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc1>;
+			qcom,saw = <&saw1>;
 		};
 
 		cpu at 2 {
@@ -36,6 +38,7 @@
 			enable-method = "qcom,kpss-acc-v2";
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc2>;
+			qcom,saw = <&saw2>;
 		};
 
 		cpu at 3 {
@@ -45,6 +48,7 @@
 			enable-method = "qcom,kpss-acc-v2";
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc3>;
+			qcom,saw = <&saw3>;
 		};
 
 		L2: l2-cache {
@@ -141,7 +145,27 @@
 			};
 		};
 
-		saw_l2: regulator at f9012000 {
+		saw0: saw at f9089000 {
+			compatible = "qcom,apq8084-saw2-v2.1-cpu";
+			reg = <0xf9089000 0x1000>;
+		};
+
+		saw1: saw at f9099000 {
+			compatible = "qcom,apq8084-saw2-v2.1-cpu";
+			reg = <0xf9099000 0x1000>;
+		};
+
+		saw2: saw at f90a9000 {
+			compatible = "qcom,apq8084-saw2-v2.1-cpu";
+			reg = <0xf90a9000 0x1000>;
+		};
+
+		saw3: saw at f90b9000 {
+			compatible = "qcom,apq8084-saw2-v2.1-cpu";
+			reg = <0xf90b9000 0x1000>;
+		};
+
+		saw_l2: saw at f9012000 {
 			compatible = "qcom,saw2";
 			reg = <0xf9012000 0x1000>;
 			regulator;
-- 
1.9.1

[PATCH v7 4/7] qcom: pm: Add cpu low power mode functions

[Lina Iyer]

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 <lina.iyer@linaro.org>
[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

diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 20b329f..19900ed 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -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
diff --git a/drivers/soc/qcom/pm.c b/drivers/soc/qcom/pm.c
new file mode 100644
index 0000000..a2f7d72
--- /dev/null
+++ b/drivers/soc/qcom/pm.c
@@ -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;
+
+	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());
+		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)
+{
+	int ret;
+
+	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();
+		break;
+	}
+
+	local_irq_enable();
+
+	return ret;
+}
+
+static struct platform_device qcom_cpuidle_device = {
+	.name              = "qcom_cpuidle",
+	.id                = -1,
+	.dev.platform_data = qcom_cpu_pm_enter_sleep,
+};
+
+static int __init qcom_pm_device_init(void)
+{
+	platform_device_register(&qcom_cpuidle_device);
+
+	return 0;
+}
+device_initcall(qcom_pm_device_init);
diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
new file mode 100644
index 0000000..563b9c3
--- /dev/null
+++ b/include/soc/qcom/pm.h
@@ -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
+
+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 */
-- 
1.9.1

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[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 <lina.iyer@linaro.org>
---
 .../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

diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
new file mode 100644
index 0000000..47095b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
@@ -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
+    * Retention
+    * Standalone Power Collapse (Standalone PC or SPC)
+    * Power Collapse (PC)
+
+WFI: WFI does a little more in addition to architectural clock gating.  ARM
+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
+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
+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
+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
+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
+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
+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
+itself in the fact that the cpu acknowledges and permits the SoC to enter
+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
+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
+the node. They indicate to the cpuidle driver the entry point to use for
+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
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index 38cff69..6a9ee12 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -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
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 4d177b9..6c222d5 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -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
diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
new file mode 100644
index 0000000..2fcf79a
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-qcom.c
@@ -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();
+
+	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)
+{
+	struct cpuidle_driver *drv = &qcom_cpuidle_driver;
+	int ret;
+
+	qcom_idle_enter = (void *)(pdev->dev.platform_data);
+	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);
-- 
1.9.1

[PATCH v7 6/7] arm: dts: qcom: Add idle states device nodes for 8974

[Lina Iyer]

Add allowable C-States for each cpu using the cpu-idle-states node.
ARM spec dictates WFI as the default idle state at 0. Support standalone
power collapse (power down that does not affect any SoC idle states) for
each cpu.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/boot/dts/qcom-msm8974.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index 828f5bb..cb710ce 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -22,6 +22,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc0>;
 			qcom,saw = <&saw0>;
+			cpu-idle-states = <&CPU_WFI &CPU_SPC>;
 		};
 
 		cpu at 1 {
@@ -32,6 +33,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc1>;
 			qcom,saw = <&saw1>;
+			cpu-idle-states = <&CPU_WFI &CPU_SPC>;
 		};
 
 		cpu at 2 {
@@ -42,6 +44,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc2>;
 			qcom,saw = <&saw2>;
+			cpu-idle-states = <&CPU_WFI &CPU_SPC>;
 		};
 
 		cpu at 3 {
@@ -52,6 +55,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc3>;
 			qcom,saw = <&saw3>;
+			cpu-idle-states = <&CPU_WFI &CPU_SPC>;
 		};
 
 		L2: l2-cache {
@@ -59,6 +63,22 @@
 			cache-level = <2>;
 			qcom,saw = <&saw_l2>;
 		};
+
+		idle-states {
+			CPU_WFI: wfi {
+				compatible = "qcom,idle-state-wfi", "arm,idle-state";
+				entry-latency-us = <1>;
+				exit-latency-us = <1>;
+				min-residency-us = <2>;
+			};
+
+			CPU_SPC: spc {
+				compatible = "qcom,idle-state-spc", "arm,idle-state";
+				entry-latency-us = <150>;
+				exit-latency-us = <200>;
+				min-residency-us = <2000>;
+			};
+		};
 	};
 
 	cpu-pmu {
-- 
1.9.1

[PATCH v7 7/7] arm: dts: qcom: Add idle states device nodes for 8084

[Lina Iyer]

Add allowable C-States for each cpu using the cpu-idle-states node.
ARM spec dictates WFI as the default idle state at 0. Support standalone
power collapse (power down that does not affect any SoC idle states) for
each cpu.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 arch/arm/boot/dts/qcom-apq8084.dtsi | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/arch/arm/boot/dts/qcom-apq8084.dtsi b/arch/arm/boot/dts/qcom-apq8084.dtsi
index 3dda230..a1ea1c0 100644
--- a/arch/arm/boot/dts/qcom-apq8084.dtsi
+++ b/arch/arm/boot/dts/qcom-apq8084.dtsi
@@ -19,6 +19,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc0>;
 			qcom,saw = <&saw0>;
+			cpu-idle-states = <&CPU_WFI &CPU_SPC>;
 		};
 
 		cpu at 1 {
@@ -29,6 +30,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc1>;
 			qcom,saw = <&saw1>;
+			cpu-idle-states = <&CPU_WFI &CPU_SPC>;
 		};
 
 		cpu at 2 {
@@ -39,6 +41,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc2>;
 			qcom,saw = <&saw2>;
+			cpu-idle-states = <&CPU_WFI &CPU_SPC>;
 		};
 
 		cpu at 3 {
@@ -49,6 +52,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc3>;
 			qcom,saw = <&saw3>;
+			cpu-idle-states = <&CPU_WFI &CPU_SPC>;
 		};
 
 		L2: l2-cache {
@@ -56,6 +60,22 @@
 			cache-level = <2>;
 			qcom,saw = <&saw_l2>;
 		};
+
+		idle-states {
+			CPU_WFI: wfi {
+				compatible = "qcom,idle-state-wfi", "arm,idle-state";
+				entry-latency-us = <1>;
+				exit-latency-us = <1>;
+				min-residency-us = <2>;
+			};
+
+			CPU_SPC: spc {
+				compatible = "qcom,idle-state-spc", "arm,idle-state";
+				entry-latency-us = <150>;
+				exit-latency-us = <200>;
+				min-residency-us = <2000>;
+			};
+		};
 	};
 
 	cpu-pmu {
-- 
1.9.1

[PATCH v7 1/7] qcom: spm: Add Subsystem Power Manager driver

[Pramod Gurav]

On 27-09-2014 06:28 AM, Lina Iyer wrote:
> Based on work by many authors, available at codeaurora.org
> 

..

> +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;
initialization not necessary.

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

..

> +#endif  /* __QCOM_SPM_H */
> 

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Pramod Gurav]

On 27-09-2014 06:28 AM, Lina Iyer wrote:
..

> +}
> +
> +static struct platform_driver qcom_cpuidle_plat_driver = {
> +	.probe	= qcom_cpuidle_probe,
> +	.driver = {
> +		.name = "qcom_cpuidle",
> +		.owner = THIS_MODULE,
.owner field can be removed here, as for drivers which
use the module_platform_driver API, this is overriden in
platform_driver_register anyway.
> +	},
> +};
> +
> +module_platform_driver(qcom_cpuidle_plat_driver);
> 

[PATCH v7 4/7] qcom: pm: Add cpu low power mode functions

[Pramod Gurav]

On 27-09-2014 06:28 AM, Lina Iyer wrote:
> 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.
> 

..

> +#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)
There is checkpatch warning here.

WARNING: please, no space before tabs
#75: FILE: drivers/soc/qcom/pm.c:28:
+#define SCM_L2_ON ^I^I(0x0)$

> +#define SCM_L2_OFF		(0x1)
> +
> +
..

> +
> +#endif  /* __QCOM_PM_H */
> 

[PATCH v7 1/7] qcom: spm: Add Subsystem Power Manager driver

[Pramod Gurav]

Hi Lina,

After enabling CONFIG_DEBUG_SECTION_MISMATCH I see few section mismatch
warnings like:

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

On Saturday 27 September 2014 06:28 AM, Lina Iyer wrote:
> 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

..

> +
> +static const struct of_device_id spm_match_table[] __initconst = {
Removing __initconst fixes the warning.

> +	{ .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)
> +{

Thanks
Pramod

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Pramod Gurav]

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.

On Saturday 27 September 2014 06:28 AM, Lina Iyer wrote:
> 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.
> 
>

..

> +};
> +
> +static const struct of_device_id qcom_idle_state_match[] __initconst = {
This is causing it.

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

[PATCH v7 0/7] QCOM 8974 and 8084 cpuidle driver

[Pramod Gurav]

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
On Saturday 27 September 2014 06:28 AM, Lina Iyer wrote:
> 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
> 

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lina Iyer]

On Mon, Sep 29 2014 at 04:27 -0600, Pramod Gurav wrote:
>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.
>

Thanks, will fix it.

>On Saturday 27 September 2014 06:28 AM, Lina Iyer wrote:
>> 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.
>>
>>
>
>..
>
>> +};
>> +
>> +static const struct of_device_id qcom_idle_state_match[] __initconst = {
>This is causing it.
>
>> +	{ .compatible = "qcom,idle-state-wfi", .data = qcom_lpm_enter_wfi },
>> +	{ .compatible = "qcom,idle-state-spc", .data = qcom_lpm_enter_spc },
>> +	{ },
>> +};
>> +
>Thanks
>Pramod
>

[PATCH v7 0/7] QCOM 8974 and 8084 cpuidle driver

[Lina Iyer]

On Mon, Sep 29 2014 at 06:18 -0600, Pramod Gurav wrote:
>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.

>Thanks
>Pramod
>On Saturday 27 September 2014 06:28 AM, Lina Iyer wrote:
>> 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
>>

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lorenzo Pieralisi]

Hi Lina,

On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote:
> 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 <lina.iyer@linaro.org>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
> new file mode 100644
> index 0000000..47095b9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
> @@ -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
> +    * 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.

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

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

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

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

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

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

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

s/in the fact that/in that/

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

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

> +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
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index 38cff69..6a9ee12 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -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
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 4d177b9..6c222d5 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -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
> diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
> new file mode 100644
> index 0000000..2fcf79a
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-qcom.c
> @@ -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();
> +
> +	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 = {

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

> +	{ .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)
> +{
> +	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.

Lorenzo

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

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lina Iyer]

On Mon, Sep 29 2014 at 09:31 -0600, Lorenzo Pieralisi wrote:
>Hi Lina,
>
>On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote:
>> +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.
>
Hmm.. Not elegant. Let me see what I can do.

>> +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/
>
>> +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.
>
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).

>> +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
>> +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.
>
Hopefully so :)

>> +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/
>
>> +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/
>
>> +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/
>
>> +itself in the fact that the cpu acknowledges and permits the SoC to enter
>
>s/in the fact that/in that/
>
>> +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.
>
Okay. I thought I changed it from coupled to cluster.. I will change it
to cluster.

>> +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
>> +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.
>
Hmm. I'll take it out.

>> +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 = {
>> +	.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).
>
Yes, fixing it.
>> +	{ .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)
>> +{
>> +	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.

Lina

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lorenzo Pieralisi]

On Mon, Sep 29, 2014 at 05:16:25PM +0100, Lina Iyer wrote:

[...]

> >> +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/
> >
> >> +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.
> >
> 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).

I wanted to say that instead of referring to L2 you can refer to the
computing system as a whole, it is a computing subsystem clock gating.

Instead of referring to L2, refer to cache hierarchy, generically.

It is just a nitpick to make your life easier in the future.

[...]

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

You are welcome, thanks for you patience in converting the driver to the
new DT API.

Lorenzo

[PATCH v7 2/7] arm: dts: qcom: Add SPM device bindings for 8974

[Stephen Boyd]

On 09/26/14 17:58, Lina Iyer wrote:
> @@ -144,7 +148,27 @@
>  			};
>  		};
>  
> -		saw_l2: regulator at f9012000 {
> +		saw0: saw at f9089000 {
> +			compatible = "qcom,msm8974-saw2-v2.1-cpu";
> +			reg = <0xf9089000 0x1000>;

There is another reg property as part of the binding that should be
specified here.

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

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Stephen Boyd]

On 09/26/14 17:58, Lina Iyer wrote:
> diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
> new file mode 100644
> index 0000000..2fcf79a
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-qcom.c
> @@ -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);

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

> +
> +	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)
> +{
> +	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?

> +
> +	 /* 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?

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

> +		return ret;
> +	}
> +
> +	return 0;

Could just be return cpuidle_register(drv, NULL);

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

[PATCH v7 1/7] qcom: spm: Add Subsystem Power Manager driver

[Stephen Boyd]

On 09/26/14 17:58, Lina Iyer wrote:
> 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 <lina.iyer@linaro.org>
> [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
>
> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> index 1505fb8..9a9cc99 100644
> --- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> @@ -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"
> -			 "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.

>  
>  - reg:
>  	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:
>  
> -	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.

>  		compatible = "qcom,saw2";
>  		reg = <0x02099000 0x1000>, <0x02009000 0x1000>;
>  	};
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 7dcd554..cd249c4 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -11,3 +11,11 @@ config QCOM_GSBI
>  
>  config QCOM_SCM
>  	bool
> +
> +config QCOM_PM
> +	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.

> +	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.
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 70d52ed..20b329f 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -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
> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
> new file mode 100644
> index 0000000..0ba7949
> --- /dev/null
> +++ b/drivers/soc/qcom/spm.c
> @@ -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},
> +	.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},
> +	.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.

> +	.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
> + * @mode: SPM LPM mode to enter
> + */
> +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.
 
> +	u32 ctl_val;
> +
> +	if (!drv || !drv->reg_data)
> +		return -ENXIO;

Does this ever happen? Please remove.

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

> +	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) {
> +		cpu_node = of_get_cpu_node(cpu, NULL);
> +		if (!cpu_node)
> +			continue;
> +		saw_node = of_parse_phandle(cpu_node, "qcom,saw", 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.

> +	}
> +
> +	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.
> +	 */
> +	wmb();
> +
> +	return 0;
> +}
> +
> +static struct platform_driver spm_driver = {
> +	.probe = spm_dev_probe,
> +	.driver = {
> +		.name = "spm",

qcom-spm?

> +		.of_match_table = spm_match_table,
> +	},
> +};
> +
> +static int __init spm_driver_init(void)
> +{
> +	return platform_driver_register(&spm_driver);
> +}
> +device_initcall(spm_driver_init);

Why can't we support modules?

> diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
> new file mode 100644
> index 0000000..997abfc
> --- /dev/null
> +++ b/include/soc/qcom/spm.h
> @@ -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)
> +
> +int qcom_spm_set_low_power_mode(enum spm_mode mode);
> +
> +#else
> +
> +static inline int qcom_spm_set_low_power_mode(enum spm_mode mode)
> +{ return -ENOSYS; }
> +
> +#endif  /* CONFIG_QCOM_PM */
> +
> +#endif  /* __QCOM_SPM_H */

It would be nice to not have this file.

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

[PATCH v7 4/7] qcom: pm: Add cpu low power mode functions

[Stephen Boyd]

On 09/26/14 17:58, Lina Iyer wrote:
> 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 <lina.iyer@linaro.org>
> [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/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
>
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index 20b329f..19900ed 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -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
> diff --git a/drivers/soc/qcom/pm.c b/drivers/soc/qcom/pm.c
> new file mode 100644
> index 0000000..a2f7d72
> --- /dev/null
> +++ b/drivers/soc/qcom/pm.c
> @@ -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;
> +
> +	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()?

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

Stuff cpu into a local variable?

> +		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)
> +{
> +	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.

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

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

modules?

> diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
> new file mode 100644
> index 0000000..563b9c3
> --- /dev/null
> +++ b/include/soc/qcom/pm.h
> @@ -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
> +
> +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.

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

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lorenzo Pieralisi]

On Tue, Sep 30, 2014 at 12:18:46AM +0100, Stephen Boyd wrote:
> On 09/26/14 17:58, Lina Iyer wrote:
> > diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
> > new file mode 100644
> > index 0000000..2fcf79a
> > --- /dev/null
> > +++ b/drivers/cpuidle/cpuidle-qcom.c
> > @@ -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);
> 
> 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.

> > +	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)
> > +{
> > +	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?
> 
> > +
> > +	 /* 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.

> > +		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).

Lorenzo

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lina Iyer]

On Mon, Sep 29 2014 at 17:18 -0600, Stephen Boyd wrote:
>On 09/26/14 17:58, Lina Iyer wrote:
>> diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
>> new file mode 100644
>> index 0000000..2fcf79a
>> --- /dev/null
>> +++ b/drivers/cpuidle/cpuidle-qcom.c
>> @@ -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);
>
>Why can't we just pass index here? It would be nice to not need to
>include soc/qcom/pm.h in this file.
>

This is the right place, the translation of QCOM's idle states is done
while registering and needs to be translated back to QCOM's idle states.
The interpretation of idle state bindings to the QCOM is hence
contained in one place.

>> +
>> +	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)
>> +{
>> +	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?
>
The dev is there, not just the expected platform data.
I can change.

>> +
>> +	 /* 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?
>
Okay
>> +		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.
>

>> +		return ret;
>> +	}
>> +
>> +	return 0;
>
>Could just be return cpuidle_register(drv, NULL);
>
Sure.

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

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lina Iyer]

On Tue, Sep 30 2014 at 02:58 -0600, Lorenzo Pieralisi wrote:
>On Tue, Sep 30, 2014 at 12:18:46AM +0100, Stephen Boyd wrote:
>> On 09/26/14 17:58, Lina Iyer wrote:
>> > diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
[...]
> > +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.
>
>I think this is definitely worth exploring.
>
Sorry, I explained in the other thread. I feel that we are dispersing
the translation information all over the place in doing so. The reason
being, the compatible flag is not passed over to pm.c and I believe this
is where it should be.

>> > +	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 },
>> > +	{ },
>> > +};
This is the onward translation from QCOM's idle states to cpuidle's
states and passing cpuidle index value to SoC layer, opens up
possibility for errors.

>> > +
>> > +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?
>>
>> > +
>> > +	 /* 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.
>
OK
>> > +		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).
>
OK
>

[PATCH v7 4/7] qcom: pm: Add cpu low power mode functions

[Lina Iyer]

On Mon, Sep 29 2014 at 17:37 -0600, Stephen Boyd wrote:
>On 09/26/14 17:58, Lina Iyer wrote:
>> Based on work by many authors, available at codeaurora.org
>>
>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> [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.
>
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.

> +
[...]
>> +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?

>> +	if (ret) {
>> +		pr_err("Failed to set warm boot address for cpu %d\n",
>> +				raw_smp_processor_id());
>
>Stuff cpu into a local variable?
>
Yeah
>> +		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)
>> +{
>> +	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.

>> +		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;
>> +}
>> +
>> +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.
>
Okay
>> +
>> +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

>> diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
>> new file mode 100644
>> index 0000000..563b9c3
>> --- /dev/null
>> +++ b/include/soc/qcom/pm.h
>> @@ -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
>> +
>> +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.
>
>-- 
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>hosted by The Linux Foundation
>

[PATCH v7 1/7] qcom: spm: Add Subsystem Power Manager driver

[Lina Iyer]

On Mon, Sep 29 2014 at 17:19 -0600, Stephen Boyd wrote:
>On 09/26/14 17:58, Lina Iyer wrote:

>  	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,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.
>
You are right, I should not have removed them.
>>
>>  - reg:
>>  	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:
>>
>> -	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.
>
I am okay with the name power controller.

>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> +config QCOM_PM
>> +	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.
>
OK.

We want? Why? Thats just many config items that we have to configure to
build the kernel. If you know of a reason, please let me know.

[...]
> +
>> +/* 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},
>> +	.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? 
I dont think I considered that.

>We don't need this initial value table. The
>only thing that really is different is delay and seq entries. 
I need the PMIC_DATA for supporting 8064. And the voltage control and
the status registers for verifying the changes. I looked at the common
functionality with SoC that I plan to support and used them here.

>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.
Endianness is something I may need to think about. So for that purpose,
I may need to fashion this into sequences. I just removed a bunch of
code that did that. Made the driver a lot easy on the eyes.

>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 thought about it, its just unnecessary bunch of data structures. This
is clearly a name, value pair and is much more readable.


>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.
>
Fair enough.

>> +	.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
>> + * @mode: SPM LPM mode to enter
>> + */
>> +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.
>
Done.
>> +	u32 ctl_val;
>> +
>> +	if (!drv || !drv->reg_data)
>> +		return -ENXIO;
>
>Does this ever happen? Please remove.
>
Possible, if the idle driver makes a call, before the SPM is ready.
>> +
>> +	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.
>
okay

> +	for_each_possible_cpu(cpu) {
>> +		cpu_node = of_get_cpu_node(cpu, NULL);
>> +		if (!cpu_node)
>> +			continue;
>> +		saw_node = of_parse_phandle(cpu_node, "qcom,saw", 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.
>
Argh. I saw them after I sent the patches. Thanks for pointing it out.

> +
>> +static struct platform_driver spm_driver = {
>> +	.probe = spm_dev_probe,
>> +	.driver = {
>> +		.name = "spm",
>
>qcom-spm?
>
ok

>> +static int __init spm_driver_init(void)
>> +{
>> +	return platform_driver_register(&spm_driver);
>> +}
>> +device_initcall(spm_driver_init);
>
>Why can't we support modules?
>
It just happens later than we would like.

>> diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
>> new file mode 100644
>> index 0000000..997abfc
>> --- /dev/null
>> +++ b/include/soc/qcom/spm.h

> +#endif  /* __QCOM_SPM_H */
>
>It would be nice to not have this file.
>
Why?

Thanks for your time Stephen.

Lina

[PATCH v7 1/7] qcom: spm: Add Subsystem Power Manager driver

[Kevin Hilman]

Lina Iyer <lina.iyer@linaro.org> writes:

> 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 <lina.iyer@linaro.org>

[...]

> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> index 1505fb8..9a9cc99 100644
> --- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> @@ -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"
> -			 "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?

Kevin

[PATCH v7 3/7] arm: dts: qcom: Add SPM device bindings for 8084

[Kevin Hilman]

Lina Iyer <lina.iyer@linaro.org> writes:

> 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 <lina.iyer@linaro.org>

nit: This will need some more description, IMO.  The changelog talks
about SPM bindings, but all the stuff added refers to SAW.

Kevin

[PATCH v7 4/7] qcom: pm: Add cpu low power mode functions

[Kevin Hilman]

Lina Iyer <lina.iyer@linaro.org> writes:

> On Mon, Sep 29 2014 at 17:37 -0600, Stephen Boyd wrote:
>>On 09/26/14 17:58, Lina Iyer wrote:
>>> Based on work by many authors, available at codeaurora.org
>>>
>>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> [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.
>>
> 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.

Yes, you should remove it, but move the "Based on work by many authors"
part to just before your signoff.  Also, it's considered good manners to
name any significant contributors if there are a few standouts.  Also,
the "available at codeaurora.org" is wanting a link, ideally to a
specific commit, or a branch so original code can easily be found.

Kevin
 

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Kevin Hilman]

Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:

> Hi Lina,
>
> On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote:
>> 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 <lina.iyer@linaro.org>
>> ---
>>  .../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
>> 
>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
>> new file mode 100644
>> index 0000000..47095b9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
>> @@ -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
>> +    * 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?

Kevin

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Nicolas Pitre]

On Tue, 30 Sep 2014, Kevin Hilman wrote:

> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
> 
> > Hi Lina,
> >
> > On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote:
> >> 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 <lina.iyer@linaro.org>
> >> ---
> >>  .../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
> >> 
> >> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
> >> new file mode 100644
> >> index 0000000..47095b9
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
> >> @@ -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
> >> +    * 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?

Agreed.

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


Nicolas

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Kevin Hilman]

On Tue, Sep 30, 2014 at 10:51 AM, Nicolas Pitre
<nicolas.pitre@linaro.org> wrote:
> On Tue, 30 Sep 2014, Kevin Hilman wrote:
>
>> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
>>
>> > Hi Lina,
>> >
>> > On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote:
>> >> 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 <lina.iyer@linaro.org>
>> >> ---
>> >>  .../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
>> >>
>> >> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
>> >> new file mode 100644
>> >> index 0000000..47095b9
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
>> >> @@ -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
>> >> +    * 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?
>
> 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.

Kevin

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Nicolas Pitre]

On Tue, 30 Sep 2014, Kevin Hilman wrote:

> On Tue, Sep 30, 2014 at 10:51 AM, Nicolas Pitre
> <nicolas.pitre@linaro.org> wrote:
> > On Tue, 30 Sep 2014, Kevin Hilman wrote:
> >
> >> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
> >>
> >> > Hi Lina,
> >> >
> >> > On Sat, Sep 27, 2014 at 01:58:13AM +0100, Lina Iyer wrote:
> >> >> 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 <lina.iyer@linaro.org>
> >> >> ---
> >> >>  .../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
> >> >>
> >> >> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
> >> >> new file mode 100644
> >> >> index 0000000..47095b9
> >> >> --- /dev/null
> >> >> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
> >> >> @@ -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
> >> >> +    * 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?
> >
> > 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?


Nicolas

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Kevin Hilman]

Nicolas Pitre <nicolas.pitre@linaro.org> writes:

> On Tue, 30 Sep 2014, Kevin Hilman wrote:
>
>> On Tue, Sep 30, 2014 at 10:51 AM, Nicolas Pitre
>> <nicolas.pitre@linaro.org> wrote:
>> > On Tue, 30 Sep 2014, Kevin Hilman wrote:
>> >
>> >> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:

[...]

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

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

Kevin

[PATCH v7 5/7] qcom: cpuidle: Add cpuidle driver for QCOM cpus

[Lina Iyer]

On Tue, Sep 30 2014 at 12:41 -0600, Kevin Hilman wrote:
>Nicolas Pitre <nicolas.pitre@linaro.org> writes:
>
>> On Tue, 30 Sep 2014, Kevin Hilman wrote:
>>
>>> On Tue, Sep 30, 2014 at 10:51 AM, Nicolas Pitre
>>> <nicolas.pitre@linaro.org> wrote:
>>> > On Tue, 30 Sep 2014, Kevin Hilman wrote:
>>> >
>>> >> Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> writes:
>
>[...]
>
>>> >> > 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?
>>> >
>>> > 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?
>
>The DT idle-states binding doc (though seemingly written more with
>arm64 and SBSA in mind) uses "standby" for the shallowest idle.
>
Standby - looks good to me.

[PATCH v7 1/7] qcom: spm: Add Subsystem Power Manager driver

[Lina Iyer]

On Tue, Sep 30 2014 at 11:26 -0600, Kevin Hilman wrote:
>Lina Iyer <lina.iyer@linaro.org> writes:
>
>> 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 <lina.iyer@linaro.org>
>
>[...]
>
>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
>> index 1505fb8..9a9cc99 100644
>> --- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
>> @@ -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"
>> -			 "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?
Even with the same version of SAW2, each SoC would have different
values for the register information like the delays, sequences etc.
The capabilities of the SoC dictate many of these values.
So in mapping the .compatible with a corresponding .data in the match
table, the cpu and the SAW version combination is necessary to identify
the set of register values.

[PATCH v7 4/7] qcom: pm: Add cpu low power mode functions

[Stephen Boyd]

On 09/30/14 09:03, Lina Iyer wrote:
> On Mon, Sep 29 2014 at 17:37 -0600, Stephen Boyd wrote:
>> On 09/26/14 17:58, Lina Iyer wrote:
>>> Based on work by many authors, available at codeaurora.org
>>>
>>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> [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.
>>
> 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.
>

Kevin's suggestion is good.

>> +
> [...]
>>> +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?
>
>

No. raw_ just means "I know what I'm doing, go away preemption checks"
which usually is not the right thing if you actually care about the CPU
this code is running on staying the same after you call the function.

>>> +        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)
>>> +{
>>> +    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.
>

I imagine that the RPM notification will go into power domains? From the
cpuidle perspective we're putting the CPU into wfi, retention, gdhs,
power collapse and those correspond exactly to a flat mapping of the cpu
idle indices. Those idle states will tell the power domain "I have this
much time to sleep" and the power domain path will determine if we can
go notify the RPM or not based on the time and the state of the other
CPUs. At least this is my understanding of what Daniel was saying about
how power domains could be used to overcome the "hierarchical" idle states.

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

If that was true the bootloader should do it. The goal is to move away
from initcall ordering and use probe defer for these things.

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

[PATCH v7 4/7] qcom: pm: Add cpu low power mode functions

[Lorenzo Pieralisi]

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

[...]

> diff --git a/drivers/soc/qcom/pm.c b/drivers/soc/qcom/pm.c
> new file mode 100644
> index 0000000..a2f7d72
> --- /dev/null
> +++ b/drivers/soc/qcom/pm.c
> @@ -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;
> +
> +	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());
> +		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.

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

> +		break;
> +	}
> +
> +	local_irq_enable();

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

Lorenzo

[PATCH v7 4/7] qcom: pm: Add cpu low power mode functions

[Lina Iyer]

On Thu, Oct 02 2014 at 03:50 -0600, Lorenzo Pieralisi wrote:
>On Sat, Sep 27, 2014 at 01:58:12AM +0100, Lina Iyer wrote:
>
>[...]
>
>> diff --git a/drivers/soc/qcom/pm.c b/drivers/soc/qcom/pm.c
>> new file mode 100644
>> index 0000000..a2f7d72
>> --- /dev/null
>> +++ b/drivers/soc/qcom/pm.c
>> @@ -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());
>> +		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.
>
Sure.
>> +
>> +	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;
>> +
>> +	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.
>
Okay. I dont use it beyond this function. I shall remove it from this
function.

>> +		break;
>> +	}
>> +
>> +	local_irq_enable();
>
>This looks wrong to me. You end up running the CPU PM notifiers with irqs
>enabled, among other issues.
>
Good catch. Thanks!

Lina