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

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

[Lina Iyer]

Hi,

This v8 revision of the cpuidle driver is available at
git.linaro.org:/people/lina.iyer/linux-next cpuidle-v8


Changes since v7:
[ https://www.mail-archive.com/linux-arm-msm at vger.kernel.org/msg11199.html ]
- Address review comments
- Tested on 8974 but not 8084
- WFI renamed to Standby
- Update commit text with original author and link to the downstream tree

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 power-controller device node for 8974 Krait CPUs
  arm: dts: qcom: Add power-controller device node for 8084 Krait CPUs
  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           |  81 ++++++++
 .../devicetree/bindings/arm/msm/qcom,saw2.txt      |  31 ++-
 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                     |  79 ++++++++
 drivers/soc/qcom/Kconfig                           |   8 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/pm.c                              | 115 +++++++++++
 drivers/soc/qcom/spm.c                             | 223 +++++++++++++++++++++
 include/soc/qcom/pm.h                              |  28 +++
 12 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

-- 
1.9.1

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

[Lina Iyer]

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

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

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

Based on work by: Mahesh Sivasubramanian <msivasub@codeaurora.org>,
Ai Li <ali@codeaurora.org>, Praveen Chidambaram <pchidamb@codeaurora.org>
Original tree available at -
git://codeaurora.org/quic/la/kernel/msm-3.10.git

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 .../devicetree/bindings/arm/msm/qcom,saw2.txt      |  31 ++-
 drivers/soc/qcom/Kconfig                           |   8 +
 drivers/soc/qcom/Makefile                          |   1 +
 drivers/soc/qcom/spm.c                             | 223 +++++++++++++++++++++
 4 files changed, 257 insertions(+), 6 deletions(-)
 create mode 100644 drivers/soc/qcom/spm.c

diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
index 1505fb8..a18e8fc 100644
--- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
@@ -2,11 +2,20 @@ SPM AVS Wrapper 2 (SAW2)
 
 The SAW2 is a wrapper around the Subsystem Power Manager (SPM) and the
 Adaptive Voltage Scaling (AVS) hardware. The SPM is a programmable
-micro-controller that transitions a piece of hardware (like a processor or
+power-controller that transitions a piece of hardware (like a processor or
 subsystem) into and out of low power modes via a direct connection to
 the PMIC. It can also be wired up to interact with other processors in the
 system, notifying them when a low power state is entered or exited.
 
+Multiple revisions of the SAW hardware is supported using these Device Nodes.
+SAW2 revisions differ in the register offset and configuration data. Also,
+same revision of the SAW in different SoCs may have different configuration
+data due the the differences in hardware capabilities. Hence the SoC name, the
+version of the SAW hardware in that SoC and the distinction between cpu (big
+or Little) or cache, may be needed to uniquely identify the SAW register
+configuration and initialization data. The compatible string is used to
+indicate this parameter.
+
 PROPERTIES
 
 - compatible:
@@ -14,10 +23,13 @@ PROPERTIES
 	Value type: <string>
 	Definition: shall contain "qcom,saw2". A more specific value should be
 		    one of:
-			 "qcom,saw2-v1"
-			 "qcom,saw2-v1.1"
-			 "qcom,saw2-v2"
-			 "qcom,saw2-v2.1"
+			"qcom,saw2-v1"
+			"qcom,saw2-v1.1"
+			"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 +38,17 @@ PROPERTIES
 		    the register region. An optional second element specifies
 		    the base address and size of the alias register region.
 
+- regulator:
+	Usage: optional
+	Value type: boolean
+	Definition: Indicates that this SPM device acts as a regulator device
+			device for the core (CPU or Cache) the SPM is attached
+			to.
 
 Example:
 
-	regulator at 2099000 {
+	power-controller at 2099000 {
 		compatible = "qcom,saw2";
 		reg = <0x02099000 0x1000>, <0x02009000 0x1000>;
+		regulator;
 	};
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 7dcd554..012fb37 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 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..c1dd04b
--- /dev/null
+++ b/drivers/soc/qcom/spm.c
@@ -0,0 +1,223 @@
+/* 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/pm.h>
+
+#define MAX_PMIC_DATA 3
+#define MAX_SEQ_DATA 64
+
+enum {
+	SPM_REG_CFG,
+	SPM_REG_SPM_CTL,
+	SPM_REG_DLY,
+	SPM_REG_PMIC_DLY,
+	SPM_REG_PMIC_DATA_0,
+	SPM_REG_VCTL,
+	SPM_REG_SEQ_ENTRY,
+	SPM_REG_SPM_STS,
+	SPM_REG_PMIC_STS,
+	SPM_REG_NR,
+};
+
+struct spm_reg_data {
+	/* Register position */
+	const u8 *reg_offset;
+
+	/* Register initialization values */
+	u32 spm_cfg;
+	u32 spm_dly;
+	u32 pmic_dly;
+	u32 pmic_data[MAX_PMIC_DATA];
+
+	/* Sequences and start indices */
+	u8 seq[MAX_SEQ_DATA];
+	u8 start_index[PM_SLEEP_MODE_NR];
+
+};
+
+struct spm_driver_data {
+	void __iomem *reg_base_addr;
+	const struct spm_reg_data *reg_data;
+};
+
+static const u8 spm_reg_offset_v2_1[SPM_REG_NR] = {
+	[SPM_REG_CFG]		= 0x08,
+	[SPM_REG_SPM_CTL]	= 0x30,
+	[SPM_REG_DLY]		= 0x34,
+	[SPM_REG_SEQ_ENTRY]	= 0x80,
+};
+
+/* SPM register data for 8974, 8084 */
+static const struct spm_reg_data spm_reg_8974_8084_cpu  = {
+	.reg_offset = spm_reg_offset_v2_1,
+	.spm_cfg = 0x1,
+	.spm_dly = 0x3C102800,
+	.seq = { 0x03, 0x0B, 0x0F, 0x00, 0x20, 0x80, 0x10, 0xE8, 0x5B, 0x03,
+		0x3B, 0xE8, 0x5B, 0x82, 0x10, 0x0B, 0x30, 0x06, 0x26, 0x30,
+		0x0F },
+	.start_index[PM_SLEEP_MODE_STBY] = 0,
+	.start_index[PM_SLEEP_MODE_SPC] = 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 pm_sleep_mode mode)
+{
+	struct spm_driver_data *drv = &__get_cpu_var(cpu_spm_drv);
+	u32 start_index;
+	u32 ctl_val;
+
+	if (!drv->reg_base_addr)
+		return -ENXIO;
+
+	start_index = drv->reg_data->start_index[mode];
+
+	ctl_val = readl_relaxed(drv->reg_base_addr +
+				drv->reg_data->reg_offset[SPM_REG_SPM_CTL]);
+	start_index &= 0x7F;
+	start_index <<= 4;
+	ctl_val &= 0xFFFFF80F;
+	ctl_val |= start_index;
+	ctl_val |= 0x1; /* Enable the SPM CTL register */
+	writel_relaxed(ctl_val, drv->reg_base_addr +
+				drv->reg_data->reg_offset[SPM_REG_SPM_CTL]);
+	/* 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) {
+		if (drv)
+			break;
+		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) {
+			if (saw_node == pdev->dev.of_node)
+				drv = &per_cpu(cpu_spm_drv, cpu);
+			of_node_put(saw_node);
+		}
+		of_node_put(cpu_node);
+	}
+
+	return drv;
+}
+
+static const struct of_device_id spm_match_table[] = {
+	{ .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;
+	void __iomem *addr, *reg_base;
+	int i;
+	const u32 *seq_regs;
+
+	 /* 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);
+	reg_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(reg_base))
+		return PTR_ERR(reg_base);
+
+	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 */
+	addr = reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY];
+	seq_regs = (const u32 *)drv->reg_data->seq;
+	for (i = 0; i < ARRAY_SIZE(drv->reg_data->seq)/4; i++)
+		writel_relaxed(seq_regs[i], 4 * i + addr);
+
+	/**
+	 *  Write the SPM registers.
+	 *  An offset of 0, indicates that the SPM version does not support
+	 *  this register, otherwise it should be supported.
+	 */
+	writel_relaxed(drv->reg_data->spm_cfg,
+			reg_base + drv->reg_data->reg_offset[SPM_REG_CFG]);
+
+	if (drv->reg_data->reg_offset[SPM_REG_DLY])
+		writel_relaxed(drv->reg_data->spm_dly, reg_base +
+				drv->reg_data->reg_offset[SPM_REG_DLY]);
+
+	if (drv->reg_data->reg_offset[SPM_REG_PMIC_DLY])
+		writel_relaxed(drv->reg_data->pmic_dly, reg_base +
+				drv->reg_data->reg_offset[SPM_REG_PMIC_DLY]);
+
+	/* Write the PMIC data */
+	if (drv->reg_data->reg_offset[SPM_REG_PMIC_DATA_0])
+		for (i = 0; i < MAX_PMIC_DATA; i++)
+			writel_relaxed(drv->reg_data->pmic_data[i], reg_base +
+				drv->reg_data->reg_offset[SPM_REG_PMIC_DATA_0] +
+				4 * i);
+
+	/**
+	 * Ensure all observers see the above register writes before the
+	 * cpuidle driver is allowed to use the SPM.
+	 */
+	wmb();
+	drv->reg_base_addr = reg_base;
+
+	return 0;
+}
+
+static struct platform_driver spm_driver = {
+	.probe = spm_dev_probe,
+	.driver = {
+		.name = "qcom,spm",
+		.of_match_table = spm_match_table,
+	},
+};
+
+module_platform_driver(spm_driver);
-- 
1.9.1

[PATCH v8 2/7] arm: dts: qcom: Add power-controller device node for 8974 Krait CPUs

[Lina Iyer]

Each Krait CPU in the QCOM 8974 SoC has an SAW power controller to
regulate the power to the cpu and aide the core in entering idle states.
Reference the SAW instance and associate the instance with the CPU core.

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..70c4329 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: power-controller at f9089000 {
+			compatible = "qcom,msm8974-saw2-v2.1-cpu";
+			reg = <0xf9089000 0x1000>;
+		};
+
+		saw1: power-controller at f9099000 {
+			compatible = "qcom,msm8974-saw2-v2.1-cpu";
+			reg = <0xf9099000 0x1000>;
+		};
+
+		saw2: power-controller at f90a9000 {
+			compatible = "qcom,msm8974-saw2-v2.1-cpu";
+			reg = <0xf90a9000 0x1000>;
+		};
+
+		saw3: power-controller at f90b9000 {
+			compatible = "qcom,msm8974-saw2-v2.1-cpu";
+			reg = <0xf90b9000 0x1000>;
+		};
+
+		saw_l2: power-controller at f9012000 {
 			compatible = "qcom,saw2";
 			reg = <0xf9012000 0x1000>;
 			regulator;
-- 
1.9.1

[PATCH v8 3/7] arm: dts: qcom: Add power-controller device node for 8084 Krait CPUs

[Lina Iyer]

Each Krait CPU in the QCOM 8084 SoC has an SAW power controller to
regulate the power to the cpu and aide the core in entering idle states.
Reference the SAW instance and associate the instance with the CPU core.

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..1581b12 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: power-controller at f9089000 {
+			compatible = "qcom,apq8084-saw2-v2.1-cpu";
+			reg = <0xf9089000 0x1000>;
+		};
+
+		saw1: power-controller at f9099000 {
+			compatible = "qcom,apq8084-saw2-v2.1-cpu";
+			reg = <0xf9099000 0x1000>;
+		};
+
+		saw2: power-controller at f90a9000 {
+			compatible = "qcom,apq8084-saw2-v2.1-cpu";
+			reg = <0xf90a9000 0x1000>;
+		};
+
+		saw3: power-controller at f90b9000 {
+			compatible = "qcom,apq8084-saw2-v2.1-cpu";
+			reg = <0xf90b9000 0x1000>;
+		};
+
+		saw_l2: power-controller at f9012000 {
 			compatible = "qcom,saw2";
 			reg = <0xf9012000 0x1000>;
 			regulator;
-- 
1.9.1

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

[Lina Iyer]

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 -

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

Support Standby and SPC for the currently available QCOM SoCs.

Based on work by: Mahesh Sivasubramanian <msivasub@codeaurora.org>,
Praveen Chidambaram <pchidamb@codeaurora.org>, Murali Nalajala
<mnalajal@codeaurora.org>
Original tree available at -
git://codeaurora.org/quic/la/kernel/msm-3.10.git

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 drivers/soc/qcom/Makefile |   2 +-
 drivers/soc/qcom/pm.c     | 115 ++++++++++++++++++++++++++++++++++++++++++++++
 include/soc/qcom/pm.h     |  28 +++++++++++
 3 files changed, 144 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..1cb622e
--- /dev/null
+++ b/drivers/soc/qcom/pm.c
@@ -0,0 +1,115 @@
+/* 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>
+
+#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);
+	int ret;
+
+	if (entry == per_cpu(last_known_entry, cpu))
+		return 0;
+
+	ret = scm_set_boot_addr(virt_to_phys(entry), flags[cpu]);
+	if (!ret)
+		per_cpu(last_known_entry, cpu) = entry;
+
+	return ret;
+}
+
+static int qcom_pm_collapse(unsigned long int unused)
+{
+	int ret;
+	u32 flag;
+	int cpu = smp_processor_id();
+
+	ret = set_up_boot_address(cpu_resume, cpu);
+	if (ret) {
+		pr_err("Failed to set warm boot address for cpu %d\n", cpu);
+		return ret;
+	}
+
+	flag = SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
+	scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
+
+	/**
+	 *  Returns here only if there was a pending interrupt and we did not
+	 *  power down as a result.
+	 */
+	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;
+
+	ret = qcom_spm_set_low_power_mode(mode);
+	if (ret)
+		return ret;
+
+	switch (mode) {
+	case PM_SLEEP_MODE_SPC:
+		cpu_suspend(0, qcom_pm_collapse);
+		break;
+	default:
+	case PM_SLEEP_MODE_STBY:
+		cpu_do_idle();
+		break;
+	}
+
+	return 0;
+}
+
+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;
+}
+module_init(qcom_pm_device_init);
diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
new file mode 100644
index 0000000..e63dc1c
--- /dev/null
+++ b/include/soc/qcom/pm.h
@@ -0,0 +1,28 @@
+/*
+ * 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_STBY,
+	PM_SLEEP_MODE_RET,
+	PM_SLEEP_MODE_SPC,
+	PM_SLEEP_MODE_PC,
+	PM_SLEEP_MODE_NR,
+};
+
+int qcom_spm_set_low_power_mode(enum pm_sleep_mode mode);
+
+#endif  /* __QCOM_PM_H */
-- 
1.9.1

[PATCH v8 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 Standby and Standalone Power Collapse.

Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
 .../bindings/arm/msm/qcom,idle-state.txt           | 81 ++++++++++++++++++++++
 drivers/cpuidle/Kconfig.arm                        |  7 ++
 drivers/cpuidle/Makefile                           |  1 +
 drivers/cpuidle/cpuidle-qcom.c                     | 79 +++++++++++++++++++++
 4 files changed, 168 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..87f1742
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
@@ -0,0 +1,81 @@
+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 -
+
+    * Standby
+    * Retention
+    * Standalone Power Collapse (Standalone PC or SPC)
+    * Power Collapse (PC)
+
+Standby: Standby does a little more in addition to architectural clock gating.
+When the WFI instruction is executed the ARM core would gate its internal
+clocks. In addition to gating the clocks, QCOM cpus use this instruction as a
+trigger to execute the SPM state machine.  The SPM state machine waits for the
+interrupt to trigger the core back in to active. This triggers the cache
+heirarchy to enter standby states, when all cpus are idle. An interrupt brings
+the SPM state machine out of its wait, the next step is to ensure that the
+cache heirarchy is also out of standby, and then the cpu is allowed to resume
+execution.
+
+Retention: Retention is a low power state where the core is clock gated 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. 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 Standby.
+
+Standalone PC: A cpu can power down and warmboot if there is a sufficient time
+between the time it enters idle and the next known 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.  The SPM
+sequence for this idle state is programmed to power down the supply to the
+core, wait for the interrupt,  restore power to the core, and ensure the
+system state including cache hierarchy is ready before allowing core to
+resume.  Applying power and resetting the core causes the core to warmboot
+back into Elevation Level (EL) which trampolines the control back to the
+kernel.  Entering a power down state for the cpu, needs to be done by trapping
+into a EL. Failing to do so, would result in a crash enforced by the warm boot
+code in the EL for the SoC. On SoCs with write-back L1 cache, the cache has to
+be flushed in s/w, before powering down the core.
+
+Power Collapse: This state is similar to the SPC mode, but distinguishes
+itself in 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 clock gated and
+voltages reduced, provided all cpus enter this 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
+application processor subsystem shut down.
+
+The idle-state for QCOM SoCs are distinguished by the compatible property of
+the idle-states device node.
+The devicetree representation of the idle state should be -
+
+Required properties:
+
+- compatible: Must be one of -
+			"qcom,idle-state-stby",
+			"qcom,idle-state-ret",
+			"qcom,idle-state-spc",
+			"qcom,idle-state-pc",
+		and "arm,idle-state".
+
+Other required and optional properties are specified in [1].
+
+Example:
+
+	idle-states {
+		CPU_SPC: spc {
+			compatible = "qcom,idle-state-spc", "arm,idle-state";
+			entry-latency-us = <150>;
+			exit-latency-us = <200>;
+			min-residency-us = <2000>;
+		};
+	};
+
+[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..0a65065
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-qcom.c
@@ -0,0 +1,79 @@
+/*
+ * 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/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_stby(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int index)
+{
+	qcom_idle_enter(PM_SLEEP_MODE_STBY);
+	local_irq_enable();
+
+	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();
+	local_irq_enable();
+
+	return index;
+}
+
+static struct cpuidle_driver qcom_cpuidle_driver = {
+	.name	= "qcom_cpuidle",
+};
+
+static const struct of_device_id qcom_idle_state_match[] = {
+	{ .compatible = "qcom,idle-state-stby", .data = qcom_lpm_enter_stby},
+	{ .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 = pdev->dev.platform_data;
+	if (!qcom_idle_enter)
+		return -EFAULT;
+
+	 /* Probe for other states, including standby */
+	ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0);
+	if (ret < 0)
+		return ret;
+
+	return cpuidle_register(drv, NULL);
+}
+
+static struct platform_driver qcom_cpuidle_plat_driver = {
+	.probe	= qcom_cpuidle_probe,
+	.driver = {
+		.name = "qcom_cpuidle",
+	},
+};
+
+module_platform_driver(qcom_cpuidle_plat_driver);
-- 
1.9.1

[PATCH v8 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.
Support Standby and 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 70c4329..a5e51fe 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_STBY &CPU_SPC>;
 		};
 
 		cpu at 1 {
@@ -32,6 +33,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc1>;
 			qcom,saw = <&saw1>;
+			cpu-idle-states = <&CPU_STBY &CPU_SPC>;
 		};
 
 		cpu at 2 {
@@ -42,6 +44,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc2>;
 			qcom,saw = <&saw2>;
+			cpu-idle-states = <&CPU_STBY &CPU_SPC>;
 		};
 
 		cpu at 3 {
@@ -52,6 +55,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc3>;
 			qcom,saw = <&saw3>;
+			cpu-idle-states = <&CPU_STBY &CPU_SPC>;
 		};
 
 		L2: l2-cache {
@@ -59,6 +63,22 @@
 			cache-level = <2>;
 			qcom,saw = <&saw_l2>;
 		};
+
+		idle-states {
+			CPU_STBY: standby {
+				compatible = "qcom,idle-state-stby", "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 v8 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.
Support standby and 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 1581b12..68eb4bb1 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_STBY &CPU_SPC>;
 		};
 
 		cpu at 1 {
@@ -29,6 +30,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc1>;
 			qcom,saw = <&saw1>;
+			cpu-idle-states = <&CPU_STBY &CPU_SPC>;
 		};
 
 		cpu at 2 {
@@ -39,6 +41,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc2>;
 			qcom,saw = <&saw2>;
+			cpu-idle-states = <&CPU_STBY &CPU_SPC>;
 		};
 
 		cpu at 3 {
@@ -49,6 +52,7 @@
 			next-level-cache = <&L2>;
 			qcom,acc = <&acc3>;
 			qcom,saw = <&saw3>;
+			cpu-idle-states = <&CPU_STBY &CPU_SPC>;
 		};
 
 		L2: l2-cache {
@@ -56,6 +60,22 @@
 			cache-level = <2>;
 			qcom,saw = <&saw_l2>;
 		};
+
+		idle-states {
+			CPU_STBY: standby {
+				compatible = "qcom,idle-state-stby", "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 v8 2/7] arm: dts: qcom: Add power-controller device node for 8974 Krait CPUs

[Stephen Boyd]

On 10/07/2014 02:41 PM, Lina Iyer wrote:
> @@ -144,7 +148,27 @@
>   			};
>   		};
>   
> -		saw_l2: regulator at f9012000 {
> +		saw0: power-controller at f9089000 {
> +			compatible = "qcom,msm8974-saw2-v2.1-cpu";
> +			reg = <0xf9089000 0x1000>;

Please add the aliases.


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

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

[Stephen Boyd]

On 10/07/2014 02:41 PM, Lina Iyer wrote:
> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> index 1505fb8..a18e8fc 100644
> --- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
> @@ -2,11 +2,20 @@ SPM AVS Wrapper 2 (SAW2)
>   
>   The SAW2 is a wrapper around the Subsystem Power Manager (SPM) and the
>   Adaptive Voltage Scaling (AVS) hardware. The SPM is a programmable
> -micro-controller that transitions a piece of hardware (like a processor or
> +power-controller that transitions a piece of hardware (like a processor or
>   subsystem) into and out of low power modes via a direct connection to
>   the PMIC. It can also be wired up to interact with other processors in the
>   system, notifying them when a low power state is entered or exited.
>   
> +Multiple revisions of the SAW hardware is supported using these Device Nodes.

s/is/are/

> +SAW2 revisions differ in the register offset and configuration data. Also,
> +same revision of the SAW in different SoCs may have different configuration

the same

> +data due the the differences in hardware capabilities. Hence the SoC name, the
> +version of the SAW hardware in that SoC and the distinction between cpu (big
> +or Little) or cache, may be needed to uniquely identify the SAW register
> +configuration and initialization data. The compatible string is used to
> +indicate this parameter.
> +
>   PROPERTIES
>   
>   - compatible:
> 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..c1dd04b
> --- /dev/null
> +++ b/drivers/soc/qcom/spm.c
> @@ -0,0 +1,223 @@
> +/* 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/pm.h>
> +
> +#define MAX_PMIC_DATA 3
> +#define MAX_SEQ_DATA 64
> +
> +enum {
> +	SPM_REG_CFG,
> +	SPM_REG_SPM_CTL,
> +	SPM_REG_DLY,
> +	SPM_REG_PMIC_DLY,
> +	SPM_REG_PMIC_DATA_0,
> +	SPM_REG_VCTL,
> +	SPM_REG_SEQ_ENTRY,
> +	SPM_REG_SPM_STS,
> +	SPM_REG_PMIC_STS,
> +	SPM_REG_NR,
> +};
> +
> +struct spm_reg_data {
> +	/* Register position */
> +	const u8 *reg_offset;
> +
> +	/* Register initialization values */
> +	u32 spm_cfg;
> +	u32 spm_dly;
> +	u32 pmic_dly;
> +	u32 pmic_data[MAX_PMIC_DATA];
> +
> +	/* Sequences and start indices */
> +	u8 seq[MAX_SEQ_DATA];
> +	u8 start_index[PM_SLEEP_MODE_NR];
> +
> +};
> +
> +struct spm_driver_data {
> +	void __iomem *reg_base_addr;

It's not really an address, more like a reg_base or just base.

> +	const struct spm_reg_data *reg_data;
> +};
> +
> +static const u8 spm_reg_offset_v2_1[SPM_REG_NR] = {
> +	[SPM_REG_CFG]		= 0x08,
> +	[SPM_REG_SPM_CTL]	= 0x30,
> +	[SPM_REG_DLY]		= 0x34,
> +	[SPM_REG_SEQ_ENTRY]	= 0x80,
> +};
> +
> +/* SPM register data for 8974, 8084 */
> +static const struct spm_reg_data spm_reg_8974_8084_cpu  = {
> +	.reg_offset = spm_reg_offset_v2_1,
> +	.spm_cfg = 0x1,
> +	.spm_dly = 0x3C102800,
> +	.seq = { 0x03, 0x0B, 0x0F, 0x00, 0x20, 0x80, 0x10, 0xE8, 0x5B, 0x03,
> +		0x3B, 0xE8, 0x5B, 0x82, 0x10, 0x0B, 0x30, 0x06, 0x26, 0x30,
> +		0x0F },
> +	.start_index[PM_SLEEP_MODE_STBY] = 0,
> +	.start_index[PM_SLEEP_MODE_SPC] = 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 pm_sleep_mode mode)
> +{
> +	struct spm_driver_data *drv = &__get_cpu_var(cpu_spm_drv);

this_cpu_ptr()

> +	u32 start_index;
> +	u32 ctl_val;
> +
> +	if (!drv->reg_base_addr)
> +		return -ENXIO;
> +
> +	start_index = drv->reg_data->start_index[mode];
> +
> +	ctl_val = readl_relaxed(drv->reg_base_addr +
> +				drv->reg_data->reg_offset[SPM_REG_SPM_CTL]);
> +	start_index &= 0x7F;

Why are we statically defining numbers larger than 0x7f? Drop this?

> +	start_index <<= 4;
> +	ctl_val &= 0xFFFFF80F;

Make a #define for this register field (or two)?

#define SPM_CTL_INDEX 0x7f
#define SPM_CTL_INDEX_SHIFT 4
#define SPM_CTL_EN BIT(0)

ctl_val &= ~(SPM_CTL_INDEX << SPM_CTL_INDEX_SHIFT);
ctl_val |= start_index << SPM_CTL_INDEX_SHIFT;
ctl_val |= SPM_CTL_EN;

> +	ctl_val |= start_index;
> +	ctl_val |= 0x1; /* Enable the SPM CTL register */
> +	writel_relaxed(ctl_val, drv->reg_base_addr +
> +				drv->reg_data->reg_offset[SPM_REG_SPM_CTL]);

Can we please have spm_read/write functions that take the drv, register 
mapping enum, and optional value?

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

int instead of u32

> +
> +	for_each_possible_cpu(cpu) {
> +		if (drv)
> +			break;

This looks weird. Why not put this at the end of the loop?

> +		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) {
> +			if (saw_node == pdev->dev.of_node)
> +				drv = &per_cpu(cpu_spm_drv, cpu);

How does this work with the logical cpu map? cpu0 in hardware may be 
cpu1 in software for example.

> +			of_node_put(saw_node);
> +		}
> +		of_node_put(cpu_node);
> +	}
> +
> +	return drv;
> +}
> +
> +static const struct of_device_id spm_match_table[] = {
> +	{ .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;
> +	void __iomem *addr, *reg_base;
> +	int i;
> +	const u32 *seq_regs;
> +
> +	 /* 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);
> +	reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(reg_base))
> +		return PTR_ERR(reg_base);
> +
> +	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 */

The above three comments seem so obvious. Why do we need them?

> +	drv->reg_data = match_id->data;
> +	if (!drv->reg_data)
> +		return -EINVAL;
> +
> +	/* Write the SPM sequences */
> +	addr = reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY];
> +	seq_regs = (const u32 *)drv->reg_data->seq;
> +	for (i = 0; i < ARRAY_SIZE(drv->reg_data->seq)/4; i++)
> +		writel_relaxed(seq_regs[i], 4 * i + addr);

Just use __iowrite32_copy()? Please run sparse, seq_regs is not an 
__iomem pointer.

> +
> +	/**
> +	 *  Write the SPM registers.
> +	 *  An offset of 0, indicates that the SPM version does not support
> +	 *  this register, otherwise it should be supported.
> +	 */
> +	writel_relaxed(drv->reg_data->spm_cfg,
> +			reg_base + drv->reg_data->reg_offset[SPM_REG_CFG]);
> +
> +	if (drv->reg_data->reg_offset[SPM_REG_DLY])

Is this ever false? I thought we always had these registers to configure.

> +		writel_relaxed(drv->reg_data->spm_dly, reg_base +
> +				drv->reg_data->reg_offset[SPM_REG_DLY]);
> +
> +	if (drv->reg_data->reg_offset[SPM_REG_PMIC_DLY])

Same comment.

> +		writel_relaxed(drv->reg_data->pmic_dly, reg_base +
> +				drv->reg_data->reg_offset[SPM_REG_PMIC_DLY]);
> +
> +	/* Write the PMIC data */
> +	if (drv->reg_data->reg_offset[SPM_REG_PMIC_DATA_0])
> +		for (i = 0; i < MAX_PMIC_DATA; i++)
> +			writel_relaxed(drv->reg_data->pmic_data[i], reg_base +
> +				drv->reg_data->reg_offset[SPM_REG_PMIC_DATA_0] +
> +				4 * i);

This looks unused. I'm not sure we even want to do it though? Would it 
be better if we wrote these registers in the SMP boot code with whatever 
value we're using to boot secondary CPUs? That way we don't have a 
dependency between the SMP code and this code to know to use the same 
values. We should also think about moving that SMP boot code into this 
file so that such dependencies are implicit.

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

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

[Stephen Boyd]

On 10/07/2014 02:41 PM, Lina Iyer wrote:

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

Same comment as last time, doesn't need to be static.

> +static int __init qcom_pm_device_init(void)
> +{
> +	platform_device_register(&qcom_cpuidle_device);
> +

This is wrong. We're going to register a platform device whenever this 
file is included in a kernel. This is then going to try and probe the 
qcom_cpuidle device which is going to fail and print an error message if 
we're not running on a qcom device. This is one reason why I've been 
arguing to get rid of this file and just put it inside the spm driver. 
That way we don't ever add the cpuidle device and:

  a) We know the SPM hardware is configured and ready to be used
  b) We don't get this annoying warning and have some weird device in 
sysfs on non-qcom devices


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

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

[Stephen Boyd]

On 10/07/2014 02:41 PM, Lina Iyer wrote:
> +
> +static int qcom_cpuidle_probe(struct platform_device *pdev)
> +{
> +	struct cpuidle_driver *drv = &qcom_cpuidle_driver;
> +	int ret;
> +
> +	qcom_idle_enter = pdev->dev.platform_data;
> +	if (!qcom_idle_enter)
> +		return -EFAULT;

Is this ever true? Let's just drop the whole check.



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

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

[Lina Iyer]

On Wed, Oct 08 2014 at 19:17 -0600, Stephen Boyd wrote:
>On 10/07/2014 02:41 PM, Lina Iyer wrote:
>
>>+
>>+static struct platform_device qcom_cpuidle_device = {
>>+	.name              = "qcom_cpuidle",
>>+	.id                = -1,
>>+	.dev.platform_data = qcom_cpu_pm_enter_sleep,
>>+};
>>+
>
>Same comment as last time, doesn't need to be static.
>
Ok, sorry I missed it.

>>+static int __init qcom_pm_device_init(void)
>>+{
>>+	platform_device_register(&qcom_cpuidle_device);
>>+
>
>This is wrong. We're going to register a platform device whenever this 
>file is included in a kernel. This is then going to try and probe the 
>qcom_cpuidle device which is going to fail and print an error message 
>if we're not running on a qcom device.
Why would this file be compiled on a non-qcom target? The file has a
dependency on ARCH_QCOM (as it should be) and would not be compiled on a
non-qcom target.

>This is one reason why I've 
>been arguing to get rid of this file and just put it inside the spm 
>driver. That way we don't ever add the cpuidle device and:
That is a poor excuse for removing this file, which has a purpose.
Putting all this SCM code inside SPM is not a good design. SPM is a
driver, doesnt know about SCM et al. Why would you want to clobber that
file with all these irrelevant code? 8660 does not have a secure mode,
but still uses an SPM. So all this code is pretty useless there, not
that we are supporting 8660 at this time, just as an example.
>
> a) We know the SPM hardware is configured and ready to be used
Its just one line check to see if the SPM hardware exists. There is no
error otherwise.
> b) We don't get this annoying warning and have some weird device in 
>sysfs on non-qcom devices
We wont compile this file on non-qcom device, so the point is moot.
Well, we may see the error, if the cpuidle framework is not compiled in.
But putthing this in SPM doesnt solve that problem either.

[PATCH v8 2/7] arm: dts: qcom: Add power-controller device node for 8974 Krait CPUs

[Lina Iyer]

On Tue, Oct 07 2014 at 17:17 -0600, Stephen Boyd wrote:
>On 10/07/2014 02:41 PM, Lina Iyer wrote:
>>@@ -144,7 +148,27 @@
>>  			};
>>  		};
>>-		saw_l2: regulator at f9012000 {
>>+		saw0: power-controller at f9089000 {
>>+			compatible = "qcom,msm8974-saw2-v2.1-cpu";
>>+			reg = <0xf9089000 0x1000>;
>
>Please add the aliases.
Will do.
>
>
>-- 
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>hosted by The Linux Foundation
>

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

[Lina Iyer]

On Wed, Oct 08 2014 at 19:12 -0600, Stephen Boyd wrote:
>On 10/07/2014 02:41 PM, Lina Iyer wrote:
>>diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
>>index 1505fb8..a18e8fc 100644
>>--- a/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
>>+++ b/Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt
>>@@ -2,11 +2,20 @@ SPM AVS Wrapper 2 (SAW2)
>>  The SAW2 is a wrapper around the Subsystem Power Manager (SPM) and the
>>  Adaptive Voltage Scaling (AVS) hardware. The SPM is a programmable
>>-micro-controller that transitions a piece of hardware (like a processor or
>>+power-controller that transitions a piece of hardware (like a processor or
>>  subsystem) into and out of low power modes via a direct connection to
>>  the PMIC. It can also be wired up to interact with other processors in the
>>  system, notifying them when a low power state is entered or exited.
>>+Multiple revisions of the SAW hardware is supported using these Device Nodes.
>
>s/is/are/
>
>>+SAW2 revisions differ in the register offset and configuration data. Also,
>>+same revision of the SAW in different SoCs may have different configuration
>
>the same
>
Will fix.

>+
>>+struct spm_driver_data {
>>+	void __iomem *reg_base_addr;
>
>It's not really an address, more like a reg_base or just base.
>
Sure.

>+ */
>>+int qcom_spm_set_low_power_mode(enum pm_sleep_mode mode)
>>+{
>>+	struct spm_driver_data *drv = &__get_cpu_var(cpu_spm_drv);
>
>this_cpu_ptr()
>
OK.
>>+	u32 start_index;
>>+	u32 ctl_val;
>>+
>>+	if (!drv->reg_base_addr)
>>+		return -ENXIO;
>>+
>>+	start_index = drv->reg_data->start_index[mode];
>>+
>>+	ctl_val = readl_relaxed(drv->reg_base_addr +
>>+				drv->reg_data->reg_offset[SPM_REG_SPM_CTL]);
>>+	start_index &= 0x7F;
>
>Why are we statically defining numbers larger than 0x7f? Drop this?
>
OK
>>+	start_index <<= 4;
>>+	ctl_val &= 0xFFFFF80F;
>
>Make a #define for this register field (or two)?
>
>#define SPM_CTL_INDEX 0x7f
>#define SPM_CTL_INDEX_SHIFT 4
>#define SPM_CTL_EN BIT(0)
>
>ctl_val &= ~(SPM_CTL_INDEX << SPM_CTL_INDEX_SHIFT);
>ctl_val |= start_index << SPM_CTL_INDEX_SHIFT;
>ctl_val |= SPM_CTL_EN;
>
Not liking all these macros, that would be used one time. But sure.

>>+	ctl_val |= start_index;
>>+	ctl_val |= 0x1; /* Enable the SPM CTL register */
>>+	writel_relaxed(ctl_val, drv->reg_base_addr +
>>+				drv->reg_data->reg_offset[SPM_REG_SPM_CTL]);
>
>Can we please have spm_read/write functions that take the drv, 
>register mapping enum, and optional value?
>
OK.

>>+	/* 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;
>
>int instead of u32
>
>>+
>>+	for_each_possible_cpu(cpu) {
>>+		if (drv)
>>+			break;
>
>This looks weird. Why not put this at the end of the loop?
>
Yeah.. Not sure what I was thinking, seemed like a good idea at that time :(
Will change.

>>+		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) {
>>+			if (saw_node == pdev->dev.of_node)
>>+				drv = &per_cpu(cpu_spm_drv, cpu);
>
>How does this work with the logical cpu map? cpu0 in hardware may be 
>cpu1 in software for example.
>
As long as the DT link to the right cpu is correct, we should be fine.
No?

>+
>>+	/* Get the SPM register data for this instance */
>
>The above three comments seem so obvious. Why do we need them?
>
>>+	drv->reg_data = match_id->data;
>>+	if (!drv->reg_data)
>>+		return -EINVAL;
>>+
>>+	/* Write the SPM sequences */
>>+	addr = reg_base + drv->reg_data->reg_offset[SPM_REG_SEQ_ENTRY];
>>+	seq_regs = (const u32 *)drv->reg_data->seq;
>>+	for (i = 0; i < ARRAY_SIZE(drv->reg_data->seq)/4; i++)
>>+		writel_relaxed(seq_regs[i], 4 * i + addr);
>
>Just use __iowrite32_copy()? Please run sparse, seq_regs is not an 
>__iomem pointer.
>
OK
>>+
>>+	/**
>>+	 *  Write the SPM registers.
>>+	 *  An offset of 0, indicates that the SPM version does not support
>>+	 *  this register, otherwise it should be supported.
>>+	 */
>>+	writel_relaxed(drv->reg_data->spm_cfg,
>>+			reg_base + drv->reg_data->reg_offset[SPM_REG_CFG]);
>>+
>>+	if (drv->reg_data->reg_offset[SPM_REG_DLY])
>
>Is this ever false? I thought we always had these registers to configure.
>
Probably not, but in the version 1.1 of SAW2 does not configure this
register, so we dont have to and let it be in its default.

>>+		writel_relaxed(drv->reg_data->spm_dly, reg_base +
>>+				drv->reg_data->reg_offset[SPM_REG_DLY]);
>>+
>>+	if (drv->reg_data->reg_offset[SPM_REG_PMIC_DLY])
>
>Same comment.
>
>>+		writel_relaxed(drv->reg_data->pmic_dly, reg_base +
>>+				drv->reg_data->reg_offset[SPM_REG_PMIC_DLY]);
>>+
>>+	/* Write the PMIC data */
>>+	if (drv->reg_data->reg_offset[SPM_REG_PMIC_DATA_0])
>>+		for (i = 0; i < MAX_PMIC_DATA; i++)
>>+			writel_relaxed(drv->reg_data->pmic_data[i], reg_base +
>>+				drv->reg_data->reg_offset[SPM_REG_PMIC_DATA_0] +
>>+				4 * i);
>
>This looks unused. I'm not sure we even want to do it though? Would it 
>be better if we wrote these registers in the SMP boot code with 
>whatever value we're using to boot secondary CPUs? That way we don't 
>have a dependency between the SMP code and this code to know to use 
>the same values. 
No, no, these are the registers that we need to bring the core out of
Standalone PC. When I add voltage control to SPM, this register will be
modified in this driver too. One of the voltage would be active votlage
and that would shadow the running voltage for the core.

>
>We should also think about moving that SMP boot code 
>into this file so that such dependencies are implicit.
Not sure, we need this register for SMP boot. But I will be open to
your ideas in this regard.

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

[Sudeep Holla]

On 07/10/14 22:41, Lina Iyer wrote:
> 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.
>
> Based on work by: Mahesh Sivasubramanian <msivasub@codeaurora.org>,
> Ai Li <ali@codeaurora.org>, Praveen Chidambaram <pchidamb@codeaurora.org>
> Original tree available at -
> git://codeaurora.org/quic/la/kernel/msm-3.10.git
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>   .../devicetree/bindings/arm/msm/qcom,saw2.txt      |  31 ++-
>   drivers/soc/qcom/Kconfig                           |   8 +
>   drivers/soc/qcom/Makefile                          |   1 +
>   drivers/soc/qcom/spm.c                             | 223 +++++++++++++++++++++
>   4 files changed, 257 insertions(+), 6 deletions(-)
>   create mode 100644 drivers/soc/qcom/spm.c
>

[...]

> +
> +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) {
> +               if (drv)
> +                       break;
> +               cpu_node = of_get_cpu_node(cpu, NULL);

I have not looked at the patch in detail, just this caught my attention
as I removed most of these unnecessary parsing in ARM code. Unless you
need this before topology_init, you need not parse DT to get cpu_node.
You can use of_cpu_device_node_get instead.

Regards,
Sudeep

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

[Lina Iyer]

On Thu, Oct 09 2014 at 10:53 -0600, Sudeep Holla wrote:
>
>
>On 07/10/14 22:41, Lina Iyer wrote:
>>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.
>>
>>Based on work by: Mahesh Sivasubramanian <msivasub@codeaurora.org>,
>>Ai Li <ali@codeaurora.org>, Praveen Chidambaram <pchidamb@codeaurora.org>
>>Original tree available at -
>>git://codeaurora.org/quic/la/kernel/msm-3.10.git
>>
>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>---
>>  .../devicetree/bindings/arm/msm/qcom,saw2.txt      |  31 ++-
>>  drivers/soc/qcom/Kconfig                           |   8 +
>>  drivers/soc/qcom/Makefile                          |   1 +
>>  drivers/soc/qcom/spm.c                             | 223 +++++++++++++++++++++
>>  4 files changed, 257 insertions(+), 6 deletions(-)
>>  create mode 100644 drivers/soc/qcom/spm.c
>>
>
>[...]
>
>>+
>>+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) {
>>+               if (drv)
>>+                       break;
>>+               cpu_node = of_get_cpu_node(cpu, NULL);
>
>I have not looked at the patch in detail, just this caught my attention
>as I removed most of these unnecessary parsing in ARM code. Unless you
>need this before topology_init, you need not parse DT to get cpu_node.
>You can use of_cpu_device_node_get instead.
Thanks. But in this usecase, I may need to iterate through all possible
cpus and do a get of the cpu and then get the SAW instance from that and
compare against the SPM instance that is being probed.
SPM does not have a reference to the CPU.
>
>Regards,
>Sudeep
>

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

[Sudeep Holla]

On 09/10/14 18:12, Lina Iyer wrote:
> On Thu, Oct 09 2014 at 10:53 -0600, Sudeep Holla wrote:
>>
>>
>> On 07/10/14 22:41, Lina Iyer wrote:
>>> 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.
>>>
>>> Based on work by: Mahesh Sivasubramanian <msivasub@codeaurora.org>,
>>> Ai Li <ali@codeaurora.org>, Praveen Chidambaram <pchidamb@codeaurora.org>
>>> Original tree available at -
>>> git://codeaurora.org/quic/la/kernel/msm-3.10.git
>>>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>> ---
>>>   .../devicetree/bindings/arm/msm/qcom,saw2.txt      |  31 ++-
>>>   drivers/soc/qcom/Kconfig                           |   8 +
>>>   drivers/soc/qcom/Makefile                          |   1 +
>>>   drivers/soc/qcom/spm.c                             | 223 +++++++++++++++++++++
>>>   4 files changed, 257 insertions(+), 6 deletions(-)
>>>   create mode 100644 drivers/soc/qcom/spm.c
>>>
>>
>> [...]
>>
>>> +
>>> +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) {
>>> +               if (drv)
>>> +                       break;
>>> +               cpu_node = of_get_cpu_node(cpu, NULL);
>>
>> I have not looked at the patch in detail, just this caught my attention
>> as I removed most of these unnecessary parsing in ARM code. Unless you
>> need this before topology_init, you need not parse DT to get cpu_node.
>> You can use of_cpu_device_node_get instead.
> Thanks. But in this usecase, I may need to iterate through all possible
> cpus and do a get of the cpu and then get the SAW instance from that and
> compare against the SPM instance that is being probed.
> SPM does not have a reference to the CPU.

No that shouldn't matter. If spm_get_drv is called after topology_init 
(which if IIRC is subsys_initcall), then what I meant is you need not
parse DT(via of_get_cpu_node) instead fetch the stashed cpu_node using
of_cpu_device_node_get(cpu_num)

Regards,
Sudeep

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

[Lina Iyer]

On Thu, Oct 09 2014 at 11:23 -0600, Sudeep Holla wrote:
>
>
>On 09/10/14 18:12, Lina Iyer wrote:
>>On Thu, Oct 09 2014 at 10:53 -0600, Sudeep Holla wrote:
>>>
>>>
>>>On 07/10/14 22:41, Lina Iyer wrote:
>>>>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.
>>>>
>>>>Based on work by: Mahesh Sivasubramanian <msivasub@codeaurora.org>,
>>>>Ai Li <ali@codeaurora.org>, Praveen Chidambaram <pchidamb@codeaurora.org>
>>>>Original tree available at -
>>>>git://codeaurora.org/quic/la/kernel/msm-3.10.git
>>>>
>>>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>>---
>>>>  .../devicetree/bindings/arm/msm/qcom,saw2.txt      |  31 ++-
>>>>  drivers/soc/qcom/Kconfig                           |   8 +
>>>>  drivers/soc/qcom/Makefile                          |   1 +
>>>>  drivers/soc/qcom/spm.c                             | 223 +++++++++++++++++++++
>>>>  4 files changed, 257 insertions(+), 6 deletions(-)
>>>>  create mode 100644 drivers/soc/qcom/spm.c
>>>>
>>>
>>>[...]
>>>
>>>>+
>>>>+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) {
>>>>+               if (drv)
>>>>+                       break;
>>>>+               cpu_node = of_get_cpu_node(cpu, NULL);
>>>
>>>I have not looked at the patch in detail, just this caught my attention
>>>as I removed most of these unnecessary parsing in ARM code. Unless you
>>>need this before topology_init, you need not parse DT to get cpu_node.
>>>You can use of_cpu_device_node_get instead.
>>Thanks. But in this usecase, I may need to iterate through all possible
>>cpus and do a get of the cpu and then get the SAW instance from that and
>>compare against the SPM instance that is being probed.
>>SPM does not have a reference to the CPU.
>
>No that shouldn't matter. If spm_get_drv is called after topology_init 
>(which if IIRC is subsys_initcall), then what I meant is you need not
>parse DT(via of_get_cpu_node) instead fetch the stashed cpu_node using
>of_cpu_device_node_get(cpu_num)
Ah, Ok. 
>
>Regards,
>Sudeep
>

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

[Stephen Boyd]

On 10/09, Lina Iyer wrote:
> On Wed, Oct 08 2014 at 19:17 -0600, Stephen Boyd wrote:
> 
> >>+static int __init qcom_pm_device_init(void)
> >>+{
> >>+	platform_device_register(&qcom_cpuidle_device);
> >>+
> >
> >This is wrong. We're going to register a platform device whenever
> >this file is included in a kernel. This is then going to try and
> >probe the qcom_cpuidle device which is going to fail and print an
> >error message if we're not running on a qcom device.
> Why would this file be compiled on a non-qcom target? The file has a
> dependency on ARCH_QCOM (as it should be) and would not be compiled on a
> non-qcom target.

We will compile this file on non-qcom devices in a multi-platform
kernel build. Actually that looks like it would be a problem
because cpuidle_register() will blow away any other registered
driver on non-qcom devices.

> 
> >This is one reason why I've been arguing to get rid of this file
> >and just put it inside the spm driver. That way we don't ever add
> >the cpuidle device and:
> That is a poor excuse for removing this file, which has a purpose.
> Putting all this SCM code inside SPM is not a good design. SPM is a
> driver, doesnt know about SCM et al. Why would you want to clobber that
> file with all these irrelevant code? 8660 does not have a secure mode,
> but still uses an SPM. So all this code is pretty useless there, not
> that we are supporting 8660 at this time, just as an example.

On 8660 we still have to tell the secure code where to jump to
when we come out of power collapse. The only difference is if
we execute or don't execute the wfi in the kernel.

The only thing that really matters to me is that we don't add
useless devices and do things unnecessarily on non-qcom devices
when this code is compiled in. The easiest way to do that is to
register a saw driver and then do stuff in probe when the saw
device appears. We can probably add individual cpuidle devices
when each CPU's saw probes and register a cpuidle driver once
based on some static variable after we register the first cpuidle
device.

> >
> >a) We know the SPM hardware is configured and ready to be used
> Its just one line check to see if the SPM hardware exists. There is no
> error otherwise.

I'm talking about a probe ordering dependency that would become
explicit if we had one file instead of two.

> >b) We don't get this annoying warning and have some weird device
> >in sysfs on non-qcom devices
> We wont compile this file on non-qcom device, so the point is moot.
> Well, we may see the error, if the cpuidle framework is not compiled in.
> But putthing this in SPM doesnt solve that problem either.

See above, this file will be compiled and included.

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

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

[Stephen Boyd]

On 10/09, Stephen Boyd wrote:
> On 10/09, Lina Iyer wrote:
> > On Wed, Oct 08 2014 at 19:17 -0600, Stephen Boyd wrote:
> > 
> > >>+static int __init qcom_pm_device_init(void)
> > >>+{
> > >>+	platform_device_register(&qcom_cpuidle_device);
> > >>+
> > >
> > >This is wrong. We're going to register a platform device whenever
> > >this file is included in a kernel. This is then going to try and
> > >probe the qcom_cpuidle device which is going to fail and print an
> > >error message if we're not running on a qcom device.
> > Why would this file be compiled on a non-qcom target? The file has a
> > dependency on ARCH_QCOM (as it should be) and would not be compiled on a
> > non-qcom target.
> 
> We will compile this file on non-qcom devices in a multi-platform
> kernel build. Actually that looks like it would be a problem
> because cpuidle_register() will blow away any other registered
> driver on non-qcom devices.

Oh right, this won't happen because we won't have the idle states
in the CPU node. We'll just get the annoying error message
instead.

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

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

[Stephen Boyd]

On 10/09, Lina Iyer wrote:
> On Wed, Oct 08 2014 at 19:12 -0600, Stephen Boyd wrote:
> >On 10/07/2014 02:41 PM, Lina Iyer wrote:
> >>+		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) {
> >>+			if (saw_node == pdev->dev.of_node)
> >>+				drv = &per_cpu(cpu_spm_drv, cpu);
> >
> >How does this work with the logical cpu map? cpu0 in hardware may
> >be cpu1 in software for example.
> >
> As long as the DT link to the right cpu is correct, we should be fine.
> No?

Yes I was worried we wanted to know the physical CPU for some
reason. As long as everything is logical then we're good.

> >>+		writel_relaxed(drv->reg_data->pmic_dly, reg_base +
> >>+				drv->reg_data->reg_offset[SPM_REG_PMIC_DLY]);
> >>+
> >>+	/* Write the PMIC data */
> >>+	if (drv->reg_data->reg_offset[SPM_REG_PMIC_DATA_0])
> >>+		for (i = 0; i < MAX_PMIC_DATA; i++)
> >>+			writel_relaxed(drv->reg_data->pmic_data[i], reg_base +
> >>+				drv->reg_data->reg_offset[SPM_REG_PMIC_DATA_0] +
> >>+				4 * i);
> >
> >This looks unused. I'm not sure we even want to do it though?
> >Would it be better if we wrote these registers in the SMP boot
> >code with whatever value we're using to boot secondary CPUs? That
> >way we don't have a dependency between the SMP code and this code
> >to know to use the same values.
> No, no, these are the registers that we need to bring the core out of
> Standalone PC. When I add voltage control to SPM, this register will be
> modified in this driver too. One of the voltage would be active votlage
> and that would shadow the running voltage for the core.

Right and the SMP boot code sets that initial voltage, hence the
dependency we could try to avoid. If the SMP boot code just wrote
this register as well with whatever it set the voltage to then we
don't need to do it again here.

> 
> >
> >We should also think about moving that SMP boot code into this
> >file so that such dependencies are implicit.
> Not sure, we need this register for SMP boot. But I will be open to
> your ideas in this regard.

Otherwise we move that SMP boot code into this file so that we
can share the initial voltage with this driver via some private
per-cpu variable or something. Or we just ignore this whole thing
and just leave it hardcoded to match the SMP boot code.

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

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

[Daniel Lezcano]

On 10/07/2014 11:41 PM, 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 Standby and Standalone Power Collapse.

Why not the retention mode which is in between the standby and the 
retention ?

> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
>   .../bindings/arm/msm/qcom,idle-state.txt           | 81 ++++++++++++++++++++++
>   drivers/cpuidle/Kconfig.arm                        |  7 ++
>   drivers/cpuidle/Makefile                           |  1 +
>   drivers/cpuidle/cpuidle-qcom.c                     | 79 +++++++++++++++++++++
>   4 files changed, 168 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..87f1742
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,idle-state.txt
> @@ -0,0 +1,81 @@
> +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 -
> +
> +    * Standby
> +    * Retention
> +    * Standalone Power Collapse (Standalone PC or SPC)
> +    * Power Collapse (PC)
> +
> +Standby: Standby does a little more in addition to architectural clock gating.
> +When the WFI instruction is executed the ARM core would gate its internal
> +clocks. In addition to gating the clocks, QCOM cpus use this instruction as a
> +trigger to execute the SPM state machine.  The SPM state machine waits for the
> +interrupt to trigger the core back in to active. This triggers the cache
> +heirarchy to enter standby states, when all cpus are idle. An interrupt brings

s/heirarchy/hierarchy/

> +the SPM state machine out of its wait, the next step is to ensure that the
> +cache heirarchy is also out of standby, and then the cpu is allowed to resume

s/heirarchy/hierarchy/

> +execution.
> +
> +Retention: Retention is a low power state where the core is clock gated 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. 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 Standby.
> +
> +Standalone PC: A cpu can power down and warmboot if there is a sufficient time
> +between the time it enters idle and the next known 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.  The SPM
> +sequence for this idle state is programmed to power down the supply to the
> +core, wait for the interrupt,  restore power to the core, and ensure the

                                 ^^ extra space

> +system state including cache hierarchy is ready before allowing core to
> +resume.  Applying power and resetting the core causes the core to warmboot

           ^^ extra space

> +back into Elevation Level (EL) which trampolines the control back to the
> +kernel.  Entering a power down state for the cpu, needs to be done by trapping

           ^^ extra space

> +into a EL. Failing to do so, would result in a crash enforced by the warm boot
> +code in the EL for the SoC. On SoCs with write-back L1 cache, the cache has to
> +be flushed in s/w, before powering down the core.
> +
> +Power Collapse: This state is similar to the SPC mode, but distinguishes
> +itself in 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 clock gated and
> +voltages reduced, provided all cpus enter this state.  Since the span of low

                                                         ^^ extra space

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

                                       ^^ extra space

> +with the Resource power manager processor in the SoC to indicate a complete
> +application processor subsystem shut down.
> +
> +The idle-state for QCOM SoCs are distinguished by the compatible property of
> +the idle-states device node.
> +The devicetree representation of the idle state should be -
> +
> +Required properties:
> +
> +- compatible: Must be one of -
> +			"qcom,idle-state-stby",
> +			"qcom,idle-state-ret",
> +			"qcom,idle-state-spc",
> +			"qcom,idle-state-pc",
> +		and "arm,idle-state".
> +
> +Other required and optional properties are specified in [1].
> +
> +Example:
> +
> +	idle-states {
> +		CPU_SPC: spc {
> +			compatible = "qcom,idle-state-spc", "arm,idle-state";
> +			entry-latency-us = <150>;
> +			exit-latency-us = <200>;
> +			min-residency-us = <2000>;
> +		};
> +	};
> +
> +[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

+ depends on ARCH_QCOM

> +	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..0a65065
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-qcom.c
> @@ -0,0 +1,79 @@
> +/*
> + * 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/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_stby(struct cpuidle_device *dev,
> +				struct cpuidle_driver *drv, int index)
> +{
> +	qcom_idle_enter(PM_SLEEP_MODE_STBY);

Could you replace this function by a generic one ?

It would be nice to have qcom_cpu_standby(void) and 
qcom_cpu_powerdown(void) and let behind the mysterious words 'Single 
Power Collapse' in the low level code which is qcom specific :)

I guess you had to create a single "qcom_idle_enter" because of a single 
pointer in the platform data. I am working on a common structure to be 
shared across the drivers as a common way to pass the different 
callbacks without including a soc specific header.

struct cpuidle_ops {
	int (*standby)(void *data);
	int (*retention)(void *data);
	int (*poweroff)(void *data);
};

So maybe you can either:

1. wait I post this structure and provide the driver with this one
2. create a similar structure and I will take care to upgrade when I 
post the patchset with the common structure.

The issue I see with this common structure is the initialization of the 
qcom_idle_state_match array.

> +	local_irq_enable();

local_irq_enable() is handled by the cpuidle framework.
Please remove all occurrences of this function in the driver otherwise 
time measurement will include irq time processing and will no longer be 
valid.

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

Where is cpu_suspend ?

> +	cpu_pm_exit();
> +	local_irq_enable();
> +
> +	return index;
> +}
> +
> +static struct cpuidle_driver qcom_cpuidle_driver = {
> +	.name	= "qcom_cpuidle",
> +};
> +
> +static const struct of_device_id qcom_idle_state_match[] = {
> +	{ .compatible = "qcom,idle-state-stby", .data = qcom_lpm_enter_stby},
> +	{ .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 = pdev->dev.platform_data;
> +	if (!qcom_idle_enter)
> +		return -EFAULT;

It shouldn't fail because if the probe is called then the cpuidle device 
was registered with its callback which is hardcoded.

> +	 /* Probe for other states, including standby */
> +	ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0);

Are you sure it is not worth to add the simple WFI state ? It may have 
less latency than the standby mode, no ?

> +	if (ret < 0)
> +		return ret;
> +
> +	return cpuidle_register(drv, NULL);
> +}
> +
> +static struct platform_driver qcom_cpuidle_plat_driver = {
> +	.probe	= qcom_cpuidle_probe,
> +	.driver = {
> +		.name = "qcom_cpuidle",
> +	},
> +};
> +
> +module_platform_driver(qcom_cpuidle_plat_driver);


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

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

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

[Lorenzo Pieralisi]

On Thu, Oct 23, 2014 at 12:05:39PM +0100, Daniel Lezcano wrote:

[...]

> > diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
> > new file mode 100644
> > index 0000000..0a65065
> > --- /dev/null
> > +++ b/drivers/cpuidle/cpuidle-qcom.c
> > @@ -0,0 +1,79 @@
> > +/*
> > + * 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/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_stby(struct cpuidle_device *dev,
> > +                             struct cpuidle_driver *drv, int index)
> > +{
> > +     qcom_idle_enter(PM_SLEEP_MODE_STBY);
> 
> Could you replace this function by a generic one ?
> 
> It would be nice to have qcom_cpu_standby(void) and
> qcom_cpu_powerdown(void) and let behind the mysterious words 'Single
> Power Collapse' in the low level code which is qcom specific :)
> 
> I guess you had to create a single "qcom_idle_enter" because of a single
> pointer in the platform data. I am working on a common structure to be
> shared across the drivers as a common way to pass the different
> callbacks without including a soc specific header.
> 
> struct cpuidle_ops {
>         int (*standby)(void *data);
>         int (*retention)(void *data);
>         int (*poweroff)(void *data);
> };
> 
> So maybe you can either:
> 
> 1. wait I post this structure and provide the driver with this one
> 2. create a similar structure and I will take care to upgrade when I
> post the patchset with the common structure.
> 
> The issue I see with this common structure is the initialization of the
> qcom_idle_state_match array.

Because you do not know which function to call when right ? That's why
I think it is up to the CPUidle back-end to make that decision and why
I think that the contract between the CPUidle driver and the back-end
should be the idle index. Even if you have pointers to functions,
the CPUidle driver do not know what parameter it has to chuck into
the void data, which is likely to be platform specific too. Granted,
you can make those cpuidle_ops a set of pairs, {function, parameter}
associated with an idle index, but then you will end up implementing
what I suggest below.

If you have a look at how the MCPM wrapper works on bL driver, that's
exactly the same problem. At the moment we are supporting only cluster
shutdown. If we were to add a core gating idle state, how could the
MCPM back-end differentiate between core and cluster states ? At the
moment the only way would consist in passing the residency through
mcpm_suspend() parameter. We could pass the idle state index, it is the
same story.

That's the reasoning behind cpu_suspend on ARM64, the index determines
what should be done, it is up to the platform back end to execute the
required actions (and it is not because we have PSCI on ARM64, that
concept is generic and should be made similar on ARM32 IMHO).

DT idle states are sorted by definition, and they can be parsed by
the arch back-end too to determine the actions required by an idle
state index (eg most likely information coming from power domains).

The glue code on arm64 is cpu_init_idle(), which takes charge of
initializing the idle state platform specific actions/parameters/etc.

Everything is open to debate, as long as we nail down an interface on
arm32 and we stick to that from now onwards, I do not think you are far
from achieving that at all.

Cheers,
Lorenzo

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

[Ivan T. Ivanov]

Hi, 

On Tue, 2014-10-07 at 15:41 -0600, Lina Iyer wrote:
> Hi,
> 
> This v8 revision of the cpuidle driver is available at
> git.linaro.org:/people/lina.iyer/linux-next cpuidle-v8
> 

Probably I missing something, but should I expect that
once these patches are applied driver could be successfully
 compiled? 

Patches definitely break bisectability. For example [PATCH v8 1/7]
is using <soc/qcom/pm.h> which is introduced [PATCH v8 4/7].

Regards,
Ivan

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

[Lina Iyer]

On Thu, Oct 23 2014 at 09:31 -0600, Ivan T. Ivanov wrote:
>
>Hi,
>
>On Tue, 2014-10-07 at 15:41 -0600, Lina Iyer wrote:
>> Hi,
>>
>> This v8 revision of the cpuidle driver is available at
>> git.linaro.org:/people/lina.iyer/linux-next cpuidle-v8
>>
>
>Probably I missing something, but should I expect that
>once these patches are applied driver could be successfully
> compiled?
Yes they should.
>
>Patches definitely break bisectability. For example [PATCH v8 1/7]
>is using <soc/qcom/pm.h> which is introduced [PATCH v8 4/7].
>
You are right. I missed checking compilation against each patch.
Based on some discussion, I need to see if pm.h is even needed.

>Regards,
>Ivan
>
>

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

[Lina Iyer]

On Thu, Oct 23 2014 at 06:43 -0600, Lorenzo Pieralisi wrote:
>On Thu, Oct 23, 2014 at 12:05:39PM +0100, Daniel Lezcano wrote:
>
>[...]
>
>> > diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
>> > new file mode 100644
>> > index 0000000..0a65065
>> > --- /dev/null
>> > +++ b/drivers/cpuidle/cpuidle-qcom.c
>> > @@ -0,0 +1,79 @@
>> > +/*
>> > + * 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/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_stby(struct cpuidle_device *dev,
>> > +                             struct cpuidle_driver *drv, int index)
>> > +{
>> > +     qcom_idle_enter(PM_SLEEP_MODE_STBY);
>>
>> Could you replace this function by a generic one ?
>>
>> It would be nice to have qcom_cpu_standby(void) and
>> qcom_cpu_powerdown(void) and let behind the mysterious words 'Single
>> Power Collapse' in the low level code which is qcom specific :)
>>
>> I guess you had to create a single "qcom_idle_enter" because of a single
>> pointer in the platform data. I am working on a common structure to be
>> shared across the drivers as a common way to pass the different
>> callbacks without including a soc specific header.
>>
>> struct cpuidle_ops {
>>         int (*standby)(void *data);
>>         int (*retention)(void *data);
>>         int (*poweroff)(void *data);
>> };
>>
>> So maybe you can either:
>>
>> 1. wait I post this structure and provide the driver with this one
>> 2. create a similar structure and I will take care to upgrade when I
>> post the patchset with the common structure.
>>
>> The issue I see with this common structure is the initialization of the
>> qcom_idle_state_match array.
>
>Because you do not know which function to call when right ? That's why
>I think it is up to the CPUidle back-end to make that decision and why
>I think that the contract between the CPUidle driver and the back-end
>should be the idle index. Even if you have pointers to functions,
>the CPUidle driver do not know what parameter it has to chuck into
>the void data, which is likely to be platform specific too. Granted,
>you can make those cpuidle_ops a set of pairs, {function, parameter}
>associated with an idle index, but then you will end up implementing
>what I suggest below.
>
>If you have a look at how the MCPM wrapper works on bL driver, that's
>exactly the same problem. At the moment we are supporting only cluster
>shutdown. If we were to add a core gating idle state, how could the
>MCPM back-end differentiate between core and cluster states ? At the
>moment the only way would consist in passing the residency through
>mcpm_suspend() parameter. We could pass the idle state index, it is the
>same story.
>
>That's the reasoning behind cpu_suspend on ARM64, the index determines
>what should be done, it is up to the platform back end to execute the
>required actions (and it is not because we have PSCI on ARM64, that
>concept is generic and should be made similar on ARM32 IMHO).
>
>DT idle states are sorted by definition, and they can be parsed by
>the arch back-end too to determine the actions required by an idle
>state index (eg most likely information coming from power domains).

This is where it makes it difficult for me. QCOM SoC's can differ from
each other in the states supported. For example here, I dont have
retention state, while other SoC's would have it. And to have cpuidle
states with increasing order of power saving (or latency), retention
would creep in between standby and power collpase. This makes the
mapping, a real issue to figure out what the SoC cpuidle driver chose to
define and to what functions they map to. I was thinking in lines of the 
ops structure, which could work for PSCI as well.

>
>The glue code on arm64 is cpu_init_idle(), which takes charge of
>initializing the idle state platform specific actions/parameters/etc.
>
>Everything is open to debate, as long as we nail down an interface on
>arm32 and we stick to that from now onwards, I do not think you are far
>from achieving that at all.
>
Thanks, I will try and get the QCOM idle drivers conform to the common
ideas.

>Cheers,
>Lorenzo
>

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

[Lina Iyer]

On Thu, Oct 23 2014 at 05:05 -0600, Daniel Lezcano wrote:
>On 10/07/2014 11:41 PM, 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 Standby and Standalone Power Collapse.
>
>Why not the retention mode which is in between the standby and the 
>retention ?
>
Retention would appear 'hacky' in comparision to these code, and has
dependencies on clocks. So at some point, yes, I will submit a patch to
address this deficiency. 

>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
>
>+ depends on ARCH_QCOM
>
I may have removed it, which I will check, QCOM_PM used to depend on
ARCH_QCOM

>+
>>+static void (*qcom_idle_enter)(enum pm_sleep_mode);
>>+
>>+static int qcom_lpm_enter_stby(struct cpuidle_device *dev,
>>+				struct cpuidle_driver *drv, int index)
>>+{
>>+	qcom_idle_enter(PM_SLEEP_MODE_STBY);
>
>Could you replace this function by a generic one ?
>
>It would be nice to have qcom_cpu_standby(void) and 
>qcom_cpu_powerdown(void) and let behind the mysterious words 'Single 
>Power Collapse' in the low level code which is qcom specific :)
>
>I guess you had to create a single "qcom_idle_enter" because of a 
>single pointer in the platform data. I am working on a common 
>structure to be shared across the drivers as a common way to pass the 
>different callbacks without including a soc specific header.
>
>struct cpuidle_ops {
>	int (*standby)(void *data);
>	int (*retention)(void *data);
>	int (*poweroff)(void *data);
>};
>
>So maybe you can either:
>
>1. wait I post this structure and provide the driver with this one
>2. create a similar structure and I will take care to upgrade when I 
>post the patchset with the common structure.
>
>The issue I see with this common structure is the initialization of 
>the qcom_idle_state_match array.
>
>>+	local_irq_enable();
>
>local_irq_enable() is handled by the cpuidle framework.
>Please remove all occurrences of this function in the driver otherwise 
>time measurement will include irq time processing and will no longer 
>be valid.
OK. Thanks for pointing that out.

>
>>+	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);
>
>Where is cpu_suspend ?
>
Inside that function qcom_idle_enter() in the SoC layer (pm.c)

>>+	cpu_pm_exit();
>>+	local_irq_enable();
>>+
>>+	return index;
>>+}
>>+
>>+static struct cpuidle_driver qcom_cpuidle_driver = {
>>+	.name	= "qcom_cpuidle",
>>+};
>>+
>>+static const struct of_device_id qcom_idle_state_match[] = {
>>+	{ .compatible = "qcom,idle-state-stby", .data = qcom_lpm_enter_stby},
>>+	{ .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 = pdev->dev.platform_data;
>>+	if (!qcom_idle_enter)
>>+		return -EFAULT;
>
>It shouldn't fail because if the probe is called then the cpuidle 
>device was registered with its callback which is hardcoded.
>
Yeah, I see the paradigm shift. Even though I know how the caller is, I
am always backing up the expectation with an error check. Will remove
that.

>>+	 /* Probe for other states, including standby */
>>+	ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0);
>
>Are you sure it is not worth to add the simple WFI state ? It may have 
>less latency than the standby mode, no ?
>
May be. But it would split the bucket between wfi and the cpu plus
allowing the L2 and the power hierarachy to enter their standby states.
This could very well be useful and possible, when there is a QoS request
that disallows power down and high latency states.
IMO, the benefit of the possible heirarchical standby seems to outweigh the
latency gain we may get by just doing a core's clock gating.

>>+	if (ret < 0)
>>+		return ret;
>>+
>>+	return cpuidle_register(drv, NULL);
>>+}
>>+
>>+static struct platform_driver qcom_cpuidle_plat_driver = {
>>+	.probe	= qcom_cpuidle_probe,
>>+	.driver = {
>>+		.name = "qcom_cpuidle",
>>+	},
>>+};
>>+
>>+module_platform_driver(qcom_cpuidle_plat_driver);
>
>
>-- 
> <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
>
>Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
><http://twitter.com/#!/linaroorg> Twitter |
><http://www.linaro.org/linaro-blog/> Blog
>

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

[Amit Kucheria]

On Thu, Oct 23, 2014 at 9:24 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Thu, Oct 23 2014 at 09:31 -0600, Ivan T. Ivanov wrote:
>>
>>
>> Hi,
>>
>> On Tue, 2014-10-07 at 15:41 -0600, Lina Iyer wrote:
>>>
>>> Hi,
>>>
>>> This v8 revision of the cpuidle driver is available at
>>> git.linaro.org:/people/lina.iyer/linux-next cpuidle-v8
>>>
>>
>> Probably I missing something, but should I expect that
>> once these patches are applied driver could be successfully
>> compiled?
>
> Yes they should.
>>
>>
>> Patches definitely break bisectability. For example [PATCH v8 1/7]
>> is using <soc/qcom/pm.h> which is introduced [PATCH v8 4/7].
>>
> You are right. I missed checking compilation against each patch.
> Based on some discussion, I need to see if pm.h is even needed.
>

git test-sequence[1] is your friend.

[1] http://dustin.sallings.org/2010/03/28/git-test-sequence.html

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

[Daniel Lezcano]

On 10/23/2014 06:58 PM, Lina Iyer wrote:
> On Thu, Oct 23 2014 at 05:05 -0600, Daniel Lezcano wrote:
>> On 10/07/2014 11:41 PM, 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 Standby and Standalone Power Collapse.
>>
>> Why not the retention mode which is in between the standby and the
>> retention ?
>>
> Retention would appear 'hacky' in comparision to these code, and has
> dependencies on clocks. So at some point, yes, I will submit a patch to
> address this deficiency.
>> 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
>>
>> + depends on ARCH_QCOM
>>
> I may have removed it, which I will check, QCOM_PM used to depend on
> ARCH_QCOM

If QCOM_PM depends on ARCH_QCOM, then yes you can remove the QCOM_PM dep.

>>> +static void (*qcom_idle_enter)(enum pm_sleep_mode);
>>> +
>>> +static int qcom_lpm_enter_stby(struct cpuidle_device *dev,
>>> +                struct cpuidle_driver *drv, int index)
>>> +{
>>> +    qcom_idle_enter(PM_SLEEP_MODE_STBY);
>>
>> Could you replace this function by a generic one ?
>>
>> It would be nice to have qcom_cpu_standby(void) and
>> qcom_cpu_powerdown(void) and let behind the mysterious words 'Single
>> Power Collapse' in the low level code which is qcom specific :)
>>
>> I guess you had to create a single "qcom_idle_enter" because of a
>> single pointer in the platform data. I am working on a common
>> structure to be shared across the drivers as a common way to pass the
>> different callbacks without including a soc specific header.
>>
>> struct cpuidle_ops {
>>     int (*standby)(void *data);
>>     int (*retention)(void *data);
>>     int (*poweroff)(void *data);
>> };
>>
>> So maybe you can either:
>>
>> 1. wait I post this structure and provide the driver with this one
>> 2. create a similar structure and I will take care to upgrade when I
>> post the patchset with the common structure.
>>
>> The issue I see with this common structure is the initialization of
>> the qcom_idle_state_match array.
>>
>>> +    local_irq_enable();
>>
>> local_irq_enable() is handled by the cpuidle framework.
>> Please remove all occurrences of this function in the driver otherwise
>> time measurement will include irq time processing and will no longer
>> be valid.
> OK. Thanks for pointing that out.
>
>>
>>> +    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);
>>
>> Where is cpu_suspend ?
>>
> Inside that function qcom_idle_enter() in the SoC layer (pm.c)

It would be preferable to group cpu_suspend with cpu_pm_enter/exit in 
this function.

>>> +    cpu_pm_exit();
>>> +    local_irq_enable();
>>> +
>>> +    return index;
>>> +}
>>> +
>>> +static struct cpuidle_driver qcom_cpuidle_driver = {
>>> +    .name    = "qcom_cpuidle",
>>> +};
>>> +
>>> +static const struct of_device_id qcom_idle_state_match[] = {
>>> +    { .compatible = "qcom,idle-state-stby", .data =
>>> qcom_lpm_enter_stby},
>>> +    { .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 = pdev->dev.platform_data;
>>> +    if (!qcom_idle_enter)
>>> +        return -EFAULT;
>>
>> It shouldn't fail because if the probe is called then the cpuidle
>> device was registered with its callback which is hardcoded.
>>
> Yeah, I see the paradigm shift. Even though I know how the caller is, I
> am always backing up the expectation with an error check. Will remove
> that.
>
>>> +     /* Probe for other states, including standby */
>>> +    ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0);
>>
>> Are you sure it is not worth to add the simple WFI state ? It may have
>> less latency than the standby mode, no ?
>>
> May be. But it would split the bucket between wfi and the cpu plus
> allowing the L2 and the power hierarachy to enter their standby states.
> This could very well be useful and possible, when there is a QoS request
> that disallows power down and high latency states.

Not necessarly a QoS, it could be a state selection from the governor 
with very short latencies.

> IMO, the benefit of the possible heirarchical standby seems to outweigh the
> latency gain we may get by just doing a core's clock gating.

It is up to the governor/scheduler to figure out this :)

>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    return cpuidle_register(drv, NULL);
>>> +}
>>> +
>>> +static struct platform_driver qcom_cpuidle_plat_driver = {
>>> +    .probe    = qcom_cpuidle_probe,
>>> +    .driver = {
>>> +        .name = "qcom_cpuidle",
>>> +    },
>>> +};
>>> +
>>> +module_platform_driver(qcom_cpuidle_plat_driver);
>>
>>
>> --
>> <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
>>
>> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
>> <http://twitter.com/#!/linaroorg> Twitter |
>> <http://www.linaro.org/linaro-blog/> Blog
>>


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

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

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

[Daniel Lezcano]

On 10/23/2014 02:43 PM, Lorenzo Pieralisi wrote:
> On Thu, Oct 23, 2014 at 12:05:39PM +0100, Daniel Lezcano wrote:
>
> [...]
>
>>> diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
>>> new file mode 100644
>>> index 0000000..0a65065
>>> --- /dev/null
>>> +++ b/drivers/cpuidle/cpuidle-qcom.c
>>> @@ -0,0 +1,79 @@
>>> +/*
>>> + * 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/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_stby(struct cpuidle_device *dev,
>>> +                             struct cpuidle_driver *drv, int index)
>>> +{
>>> +     qcom_idle_enter(PM_SLEEP_MODE_STBY);
>>
>> Could you replace this function by a generic one ?
>>
>> It would be nice to have qcom_cpu_standby(void) and
>> qcom_cpu_powerdown(void) and let behind the mysterious words 'Single
>> Power Collapse' in the low level code which is qcom specific :)
>>
>> I guess you had to create a single "qcom_idle_enter" because of a single
>> pointer in the platform data. I am working on a common structure to be
>> shared across the drivers as a common way to pass the different
>> callbacks without including a soc specific header.
>>
>> struct cpuidle_ops {
>>          int (*standby)(void *data);
>>          int (*retention)(void *data);
>>          int (*poweroff)(void *data);
>> };
>>
>> So maybe you can either:
>>
>> 1. wait I post this structure and provide the driver with this one
>> 2. create a similar structure and I will take care to upgrade when I
>> post the patchset with the common structure.
>>
>> The issue I see with this common structure is the initialization of the
>> qcom_idle_state_match array.
>
> Because you do not know which function to call when right ?

Hi Lorenzo,

that's correct.

Perhaps an alternative could be to use the 'weak' attribute instead of a 
structure and define the different ops like the cpu_do_idle() does.

> That's why
> I think it is up to the CPUidle back-end to make that decision and why
> I think that the contract between the CPUidle driver and the back-end
> should be the idle index. Even if you have pointers to functions,
> the CPUidle driver do not know what parameter it has to chuck into
> the void data, which is likely to be platform specific too. Granted,
> you can make those cpuidle_ops a set of pairs, {function, parameter}
> associated with an idle index, but then you will end up implementing
> what I suggest below.

I don't see where is the problem if the backend driver passes the 
different CPU PM ops. If the cpuidle driver and the backend PM code want 
to pass an index as 'data', I don't like that but it is ok. But I don't 
want to provide the index as part of the API.

We can see the mess when the code began to play with the indexes as a 
specific idle state (cf. CPUIDLE_DRIVER_STATE_START magic).

> If you have a look at how the MCPM wrapper works on bL driver, that's
> exactly the same problem. At the moment we are supporting only cluster
> shutdown. If we were to add a core gating idle state, how could the
> MCPM back-end differentiate between core and cluster states ? At the
> moment the only way would consist in passing the residency through
> mcpm_suspend() parameter. We could pass the idle state index, it is the
> same story.
>
> That's the reasoning behind cpu_suspend on ARM64, the index determines
> what should be done, it is up to the platform back end to execute the
> required actions (and it is not because we have PSCI on ARM64, that
> concept is generic and should be made similar on ARM32 IMHO).
>
> DT idle states are sorted by definition, and they can be parsed by
> the arch back-end too to determine the actions required by an idle
> state index (eg most likely information coming from power domains).
>
> The glue code on arm64 is cpu_init_idle(), which takes charge of
> initializing the idle state platform specific actions/parameters/etc.
>
> Everything is open to debate, as long as we nail down an interface on
> arm32 and we stick to that from now onwards, I do not think you are far
> from achieving that at all.




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

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

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

[Ivan T. Ivanov]

On Thu, 2014-10-23 at 09:54 -0600, Lina Iyer wrote:
> On Thu, Oct 23 2014 at 09:31 -0600, Ivan T. Ivanov wrote:
> > >
> > >Hi,
> > >
> > >On Tue, 2014-10-07 at 15:41 -0600, Lina Iyer wrote:
> > >  Hi,
> > > >
> > >  This v8 revision of the cpuidle driver is available at
> > >  git.linaro.org:/people/lina.iyer/linux-next cpuidle-v8
> > > >
> > >
> > >Probably I missing something, but should I expect that
> > >once these patches are applied driver could be successfully
> >  compiled?
> Yes they should.

Do these patches depends on other patches, which I am missing.
I am applying them on current linus/master. At least <soc/qcom/scm.h> 
and <soc/qcom/scm-boot.h> are missing from this patch-set v8 :-|

Ivan

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

[Lorenzo Pieralisi]

On Fri, Oct 24, 2014 at 09:56:35AM +0100, Daniel Lezcano wrote:
> On 10/23/2014 02:43 PM, Lorenzo Pieralisi wrote:
> > On Thu, Oct 23, 2014 at 12:05:39PM +0100, Daniel Lezcano wrote:
> >
> > [...]
> >
> >>> diff --git a/drivers/cpuidle/cpuidle-qcom.c b/drivers/cpuidle/cpuidle-qcom.c
> >>> new file mode 100644
> >>> index 0000000..0a65065
> >>> --- /dev/null
> >>> +++ b/drivers/cpuidle/cpuidle-qcom.c
> >>> @@ -0,0 +1,79 @@
> >>> +/*
> >>> + * 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/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_stby(struct cpuidle_device *dev,
> >>> +                             struct cpuidle_driver *drv, int index)
> >>> +{
> >>> +     qcom_idle_enter(PM_SLEEP_MODE_STBY);
> >>
> >> Could you replace this function by a generic one ?
> >>
> >> It would be nice to have qcom_cpu_standby(void) and
> >> qcom_cpu_powerdown(void) and let behind the mysterious words 'Single
> >> Power Collapse' in the low level code which is qcom specific :)
> >>
> >> I guess you had to create a single "qcom_idle_enter" because of a single
> >> pointer in the platform data. I am working on a common structure to be
> >> shared across the drivers as a common way to pass the different
> >> callbacks without including a soc specific header.
> >>
> >> struct cpuidle_ops {
> >>          int (*standby)(void *data);
> >>          int (*retention)(void *data);
> >>          int (*poweroff)(void *data);
> >> };
> >>
> >> So maybe you can either:
> >>
> >> 1. wait I post this structure and provide the driver with this one
> >> 2. create a similar structure and I will take care to upgrade when I
> >> post the patchset with the common structure.
> >>
> >> The issue I see with this common structure is the initialization of the
> >> qcom_idle_state_match array.
> >
> > Because you do not know which function to call when right ?
> 
> Hi Lorenzo,
> 
> that's correct.
> 
> Perhaps an alternative could be to use the 'weak' attribute instead of a 
> structure and define the different ops like the cpu_do_idle() does.

Do you mean arch_cpu_idle() ? I still do not see how the CPUidle driver
can make out what function to call when, unless it is the driver that
initializes those functions itself, I think we should see how it works
with an example.

> > That's why
> > I think it is up to the CPUidle back-end to make that decision and why
> > I think that the contract between the CPUidle driver and the back-end
> > should be the idle index. Even if you have pointers to functions,
> > the CPUidle driver do not know what parameter it has to chuck into
> > the void data, which is likely to be platform specific too. Granted,
> > you can make those cpuidle_ops a set of pairs, {function, parameter}
> > associated with an idle index, but then you will end up implementing
> > what I suggest below.
> 
> I don't see where is the problem if the backend driver passes the 
> different CPU PM ops. If the cpuidle driver and the backend PM code want 
> to pass an index as 'data', I don't like that but it is ok. But I don't 
> want to provide the index as part of the API.
> 
> We can see the mess when the code began to play with the indexes as a 
> specific idle state (cf. CPUIDLE_DRIVER_STATE_START magic).

I think that it can be done, but I also agree that there is a lot
of legacy and you are looking for an interface to unify the idle
drivers, inclusive of legacy ones so that's convoluted.

As I mentioned below, we need a solution that works well on the majority
of idle drivers, inclusive of bL one MCPM based, otherwise things get
out of control. I already have to update the bL driver for core gating
states, and in the process that requires a clean interface between MCPM
calls and the backend, which at the moment is based on passing the
state residency. We can't deploy different solutions to the same
problem, that's my point.

Thanks,
Lorenzo

> > If you have a look at how the MCPM wrapper works on bL driver, that's
> > exactly the same problem. At the moment we are supporting only cluster
> > shutdown. If we were to add a core gating idle state, how could the
> > MCPM back-end differentiate between core and cluster states ? At the
> > moment the only way would consist in passing the residency through
> > mcpm_suspend() parameter. We could pass the idle state index, it is the
> > same story.
> >
> > That's the reasoning behind cpu_suspend on ARM64, the index determines
> > what should be done, it is up to the platform back end to execute the
> > required actions (and it is not because we have PSCI on ARM64, that
> > concept is generic and should be made similar on ARM32 IMHO).
> >
> > DT idle states are sorted by definition, and they can be parsed by
> > the arch back-end too to determine the actions required by an idle
> > state index (eg most likely information coming from power domains).
> >
> > The glue code on arm64 is cpu_init_idle(), which takes charge of
> > initializing the idle state platform specific actions/parameters/etc.
> >
> > Everything is open to debate, as long as we nail down an interface on
> > arm32 and we stick to that from now onwards, I do not think you are far
> > from achieving that at all.
> 
> 
> 
> 
> -- 
>   <http://www.linaro.org/> Linaro.org ? Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter |
> <http://www.linaro.org/linaro-blog/> Blog
> 
> 

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

[Lina Iyer]

On Fri, Oct 24 2014 at 04:01 -0600, Ivan T. Ivanov wrote:
>
>On Thu, 2014-10-23 at 09:54 -0600, Lina Iyer wrote:
>> On Thu, Oct 23 2014 at 09:31 -0600, Ivan T. Ivanov wrote:
>> > >
>> > >Hi,
>> > >
>> > >On Tue, 2014-10-07 at 15:41 -0600, Lina Iyer wrote:
>> > >  Hi,
>> > > >
>> > >  This v8 revision of the cpuidle driver is available at
>> > >  git.linaro.org:/people/lina.iyer/linux-next cpuidle-v8
>> > > >
>> > >
>> > >Probably I missing something, but should I expect that
>> > >once these patches are applied driver could be successfully
>> >  compiled?
>> Yes they should.
>
>Do these patches depends on other patches, which I am missing.
>I am applying them on current linus/master. At least <soc/qcom/scm.h>
>and <soc/qcom/scm-boot.h> are missing from this patch-set v8 :-|
>

Oh sorry, if you look at the cover letter, in the past, these patches
have been pulled in or being staged.

You can get the tree and the the patches from 
https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/cpuidle-v8

which are on top of 3.17-rc1, but you should be able to pull it on top
of yours, without any issue. Do note that some of these patches that I
have on top of 3.17-rc1 (like the ARM idle states) may already be
available in linus/master.

Hope that helps.

Lina

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

[Ivan T. Ivanov]

On Fri, 2014-10-24 at 08:30 -0600, Lina Iyer wrote:
> On Fri, Oct 24 2014 at 04:01 -0600, Ivan T. Ivanov wrote:
> > >
> > >On Thu, 2014-10-23 at 09:54 -0600, Lina Iyer wrote:
> > >  On Thu, Oct 23 2014 at 09:31 -0600, Ivan T. Ivanov wrote:
> > > > > >
> > > > > >Hi,
> > > > > >
> > > > > >On Tue, 2014-10-07 at 15:41 -0600, Lina Iyer wrote:
> > > > >   Hi,
> > > > > > >
> > > > >   This v8 revision of the cpuidle driver is available at
> > > > >   git.linaro.org:/people/lina.iyer/linux-next cpuidle-v8
> > > > > > >
> > > > > >
> > > > > >Probably I missing something, but should I expect that
> > > > > >once these patches are applied driver could be successfully
> > > >   compiled?
> > >  Yes they should.
> > >
> > >Do these patches depends on other patches, which I am missing.
> > >I am applying them on current linus/master. At least 
> > 
> > >and <soc/qcom/scm-boot.h> are missing from this patch-set v8 :-|
> > >
> 
> Oh sorry, if you look at the cover letter, in the past, these patches
> have been pulled in or being staged.
> 
> You can get the tree and the the patches from

> https://git.linaro.org/people/lina.iyer/linux-next.git/shortlog/refs/heads/cpuidle-v8
> 
> which are on top of 3.17-rc1, but you should be able to pull it on 
> top
> of yours, without any issue. Do note that some of these patches that 
> I
> have on top of 3.17-rc1 (like the ARM idle states) may already be
> available in linus/master.
> 
> Hope that helps.

Yep, this is better.

Thank you,
Ivan

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

[Lina Iyer]

On Fri, Oct 24 2014 at 02:42 -0600, Daniel Lezcano wrote:
>On 10/23/2014 06:58 PM, Lina Iyer wrote:
>>On Thu, Oct 23 2014 at 05:05 -0600, Daniel Lezcano wrote:
>>>On 10/07/2014 11:41 PM, Lina Iyer wrote:

>>>+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);
>>>
>>>Where is cpu_suspend ?
>>>
>>Inside that function qcom_idle_enter() in the SoC layer (pm.c)
>
>It would be preferable to group cpu_suspend with cpu_pm_enter/exit in 
>this function.
>
OK. Sure.


>>>+     /* Probe for other states, including standby */
>>>>+    ret = dt_init_idle_driver(drv, qcom_idle_state_match, 0);
>>>
>>>Are you sure it is not worth to add the simple WFI state ? It may have
>>>less latency than the standby mode, no ?
>>>
>>May be. But it would split the bucket between wfi and the cpu plus
>>allowing the L2 and the power hierarachy to enter their standby states.
>>This could very well be useful and possible, when there is a QoS request
>>that disallows power down and high latency states.
>
>Not necessarly a QoS, it could be a state selection from the governor 
>with very short latencies.
>
>>IMO, the benefit of the possible heirarchical standby seems to outweigh the
>>latency gain we may get by just doing a core's clock gating.
>
>It is up to the governor/scheduler to figure out this :)
>
There is a bit more problem than that, which I should have mentioned
earlier.
If we have SPM available and configured for the SoC, then unless we
explictly disable the SPM, the WFI instruction executed by the cpu core
would trigger SPM state machine and since we decided to set up the SPM
state machine to do its SPM sequence for standby only when we choose
standby in cpuidle, SPM might remain configured to do the previous idle
state and as such the WFI instruction from the core would do that
(possibly deeper), uintentionally, even when the scheduler decided only
to do cpu clock gating.

So we have to disable SPM everytime, per our pattern, if we want to do
that, that means, we still have to enter the SoC specific code, and
cannot get-by with the WFI instruction alone.


Lina