QCOM 8974 and 8084 cpuidle driver

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

index 1505fb8..a18e8fc 100644

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

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

drivers/soc/qcom/Makefile +1 -0

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

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

new file mode 100644
index 0000000..c1dd04b
@@ -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;

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

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

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

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

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt SPM AVS Wrapper 2 (SAW2) · line 8

Quoted 13 lines · Lina Iyer

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

index 1505fb8..a18e8fc 100644
@@ -2,11 +2,20 @@ SPM AVS Wrapper 2 (SAW2)
 The SAW2 is a wrapper around the Subsystem Power Manager (SPM) and the
 Adaptive Voltage Scaling (AVS) hardware. The SPM is a programmable
-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/

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt SPM AVS Wrapper 2 (SAW2) · line 12

Quoted 2 lines

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

+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

+

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt SPM AVS Wrapper 2 (SAW2) · line 12

Quoted 2 lines

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

+struct spm_driver_data {
+	void __iomem *reg_base_addr;

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

+ */

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt SPM AVS Wrapper 2 (SAW2) · line 15

Quoted 3 lines

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

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

Quoted 5 lines

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

+	u32 start_index;
+	u32 ctl_val;
+
+	if (!drv->reg_base_addr)
+		return -ENXIO;

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt SPM AVS Wrapper 2 (SAW2)

Quoted 6 lines

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

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

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt

Quoted 2 lines

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

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

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt

Quoted 4 lines

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

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

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt

Quoted 11 lines

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

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

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt

Quoted 4 lines

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

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

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

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt

Quoted 7 lines

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

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

+

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt

Quoted 1 line

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

+	/* Get the SPM register data for this instance */

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

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt

Quoted 9 lines

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

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

Quoted 4 lines

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

+
+	/**
+	 *  Write the SPM registers.
+	 *  An offset of 0, indicates that the SPM version does not support

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt

Quoted 6 lines

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

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

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt

Quoted 4 lines

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

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

Documentation/devicetree/bindings/arm/msm/qcom,saw2.txt

Quoted 9 lines

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

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

Quoted 12 lines · Stephen Boyd

drivers/soc/qcom/spm.c line 130

Quoted 7 lines · Lina Iyer

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

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

Quoted 7 lines

Quoted 7 lines

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

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

Quoted 8 lines

drivers/soc/qcom/spm.c line 203

Quoted 2 lines

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

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

Quoted 4 lines

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.

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 <[email protected]>, Ai Li <[email protected]>, Praveen Chidambaram <[email protected]> Original tree available at -
git://codeaurora.org/quic/la/kernel/msm-3.10.git

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

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

Quoted 35 lines · Lina Iyer

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

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

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

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

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

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

[...]

drivers/soc/qcom/spm.c line 124

Quoted 11 lines

drivers/soc/qcom/spm.c new file +11 -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);

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

Quoted 41 lines · Sudeep Holla

Quoted 35 lines · Lina Iyer

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

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

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

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

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

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

[...]

Quoted 16 lines

drivers/soc/qcom/spm.c line 124

Quoted 11 lines

drivers/soc/qcom/spm.c new file +11 -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);

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.

Quoted 58 lines · Lina Iyer

Quoted 41 lines · Sudeep Holla

Quoted 35 lines · Lina Iyer

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

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

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

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

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

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

[...]

Quoted 16 lines

drivers/soc/qcom/spm.c line 124

Quoted 11 lines

drivers/soc/qcom/spm.c new file +11 -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);

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.

Quoted 4 lines

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

arch/arm/boot/dts/qcom-msm8974.dtsi line 152

Quoted 7 lines · Lina Iyer

arch/arm/boot/dts/qcom-msm8974.dtsi +3 -1

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

Signature · 3 lines

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

drivers/soc/qcom/pm.c line 108

Quoted 7 lines · Lina Iyer

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

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

drivers/soc/qcom/pm.c line 112

Quoted 4 lines

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

+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

Quoted 9 lines · Stephen Boyd

drivers/soc/qcom/pm.c line 112

Quoted 4 lines

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

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

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

Quoted 11 lines · Lina Iyer

Quoted 9 lines · Stephen Boyd

drivers/soc/qcom/pm.c line 112

Quoted 4 lines

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

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

Quoted 3 lines

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.

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 <[email protected]>

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

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

new file mode 100644
index 0000000..87f1742
@@ -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

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

+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

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

+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

drivers/cpuidle/Kconfig.arm +0 -0

index 38cff69..6a9ee12 100644

drivers/cpuidle/Kconfig.arm +3 -0

+	select DT_IDLE_STATES
+	help
+	  Select this to enable cpuidle for QCOM processors

drivers/cpuidle/Makefile +1 -0

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

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

new file mode 100644
index 0000000..0a65065
@@ -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>

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

+	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 },
+	{ },

drivers/cpuidle/cpuidle-qcom.c new file +14 -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);

Quoted 24 lines

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

new file mode 100644
index 0000000..0a65065
@@ -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>

drivers/cpuidle/cpuidle-qcom.c line 28

Quoted 10 lines

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

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

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

[...]

Quoted 52 lines

Quoted 24 lines

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

new file mode 100644
index 0000000..0a65065
@@ -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>

drivers/cpuidle/cpuidle-qcom.c line 28

Quoted 10 lines

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

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

Quoted 8 lines

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

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

[...]

Quoted 54 lines

Quoted 24 lines

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

new file mode 100644
index 0000000..0a65065
@@ -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>

drivers/cpuidle/cpuidle-qcom.c line 28

Quoted 10 lines

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

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

Quoted 6 lines

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.

Quoted 65 lines · Lorenzo Pieralisi

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

[...]

Quoted 60 lines

Quoted 24 lines

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

new file mode 100644
index 0000000..0a65065
@@ -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>

drivers/cpuidle/cpuidle-qcom.c line 28

Quoted 10 lines

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

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

Quoted 1 line

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.

Quoted 9 lines

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

Quoted 23 lines

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.

Signature · 5 lines

  <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

Quoted 5 lines · Lina Iyer

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

Supported modes at this time are Standby and Standalone Power Collapse.

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

drivers/cpuidle/Kconfig.arm +0 -0

Quoted 5 lines

index 38cff69..6a9ee12 100644

drivers/cpuidle/Kconfig.arm +0 -0

@@ -62,3 +62,10 @@ config ARM_MVEBU_V7_CPUIDLE
 	depends on ARCH_MVEBU

drivers/cpuidle/Kconfig.arm config ARM_MVEBU_V7_CPUIDLE · line 68

Quoted 6 lines

drivers/cpuidle/Kconfig.arm +4 -0

 	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

+

drivers/cpuidle/Kconfig.arm config ARM_MVEBU_V7_CPUIDLE

Quoted 6 lines

drivers/cpuidle/Kconfig.arm +6 -0

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

drivers/cpuidle/Kconfig.arm

Quoted 1 line

drivers/cpuidle/Kconfig.arm +1 -0

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

drivers/cpuidle/Kconfig.arm

Quoted 9 lines

drivers/cpuidle/Kconfig.arm +8 -0

+	return index;
+}
+
+static int qcom_lpm_enter_spc(struct cpuidle_device *dev,
+				struct cpuidle_driver *drv, int index)
+{
+	cpu_pm_enter();
+	qcom_idle_enter(PM_SLEEP_MODE_SPC);

Where is cpu_suspend ?

Quoted 19 lines

drivers/cpuidle/Kconfig.arm +19 -0

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

drivers/cpuidle/Kconfig.arm

Quoted 5 lines

drivers/cpuidle/Kconfig.arm +5 -0

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

drivers/cpuidle/Kconfig.arm

Quoted 2 lines

drivers/cpuidle/Kconfig.arm +2 -0

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

Quoted 14 lines

drivers/cpuidle/Kconfig.arm +14 -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);

Signature · 5 lines

<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

Quoted 10 lines · Daniel Lezcano

Quoted 5 lines · Lina Iyer

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

Supported modes at this time are 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.

Quoted 7 lines

drivers/cpuidle/Kconfig.arm +0 -0

Quoted 6 lines

index 38cff69..6a9ee12 100644

drivers/cpuidle/Kconfig.arm +0 -0

@@ -62,3 +62,10 @@ config ARM_MVEBU_V7_CPUIDLE
     depends on ARCH_MVEBU
     help

Quoted 9 lines

drivers/cpuidle/Kconfig.arm config ARM_MVEBU_V7_CPUIDLE · line 68

Quoted 6 lines

drivers/cpuidle/Kconfig.arm +0 -0

       Select this to enable cpuidle on Armada 370, 38x and XP

processors.

drivers/cpuidle/Kconfig.arm +4 -0

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

Quoted 33 lines

drivers/cpuidle/Kconfig.arm

Quoted 6 lines

drivers/cpuidle/Kconfig.arm +6 -0

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

Quoted 6 lines

Quoted 1 line

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

Quoted 12 lines

drivers/cpuidle/Kconfig.arm

Quoted 9 lines

drivers/cpuidle/Kconfig.arm +8 -0

+    return index;
+}
+
+static int qcom_lpm_enter_spc(struct cpuidle_device *dev,
+                struct cpuidle_driver *drv, int index)
+{
+    cpu_pm_enter();
+    qcom_idle_enter(PM_SLEEP_MODE_SPC);

Where is cpu_suspend ?

Inside that function qcom_idle_enter() in the SoC layer (pm.c)

Quoted 21 lines

Quoted 21 lines

drivers/cpuidle/Kconfig.arm +12 -0

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

drivers/cpuidle/Kconfig.arm +6 -0

+    { },
+};
+
+static int qcom_cpuidle_probe(struct platform_device *pdev)
+{
+    struct cpuidle_driver *drv = &qcom_cpuidle_driver;

Quoted 9 lines

drivers/cpuidle/Kconfig.arm

Quoted 5 lines

drivers/cpuidle/Kconfig.arm +5 -0

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

Quoted 3 lines

Quoted 2 lines

drivers/cpuidle/Kconfig.arm +2 -0

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

Quoted 3 lines

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.

Quoted 23 lines

Quoted 4 lines

drivers/cpuidle/Kconfig.arm +4 -0

+    if (ret < 0)
+        return ret;
+
+    return cpuidle_register(drv, NULL);

drivers/cpuidle/Kconfig.arm

Quoted 10 lines

drivers/cpuidle/Kconfig.arm +10 -0

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

Quoted 9 lines · Lina Iyer

Quoted 8 lines · Daniel Lezcano

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

+static int qcom_lpm_enter_spc(struct cpuidle_device *dev,

Quoted 4 lines

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

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

Quoted 3 lines

Quoted 2 lines

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.

Quoted 10 lines

Quoted 6 lines

+ /* Probe for other states, including standby */

Quoted 1 line

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

Quoted 2 lines

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 :)

Hi,

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

Hi,

Quoted 2 lines · Lina Iyer

Hi,

Quoted 3 lines

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

Quoted 8 lines · Ivan T. Ivanov

Hi,

Quoted 3 lines · Lina Iyer

Hi,

Quoted 7 lines

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 6 lines · Ivan T. Ivanov

Quoted 6 lines

Hi,

On Tue, 2014-10-07 at 15:41 -0600, Lina Iyer wrote:
Hi,

Quoted 1 line

Quoted 7 lines

Quoted 6 lines

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

Quoted 1 line

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

compiled?

Yes they should.

Quoted 11 lines · Lina Iyer

Quoted 10 lines · Ivan T. Ivanov

Quoted 10 lines

Hi,

On Tue, 2014-10-07 at 15:41 -0600, Lina Iyer wrote:
Hi,

Quoted 1 line

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

Quoted 1 line

Quoted 4 lines

Quoted 3 lines

Quoted 2 lines

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

Quoted 15 lines · Ivan T. Ivanov

Quoted 15 lines · Lina Iyer

Quoted 12 lines · Ivan T. Ivanov

Quoted 12 lines

Quoted 4 lines

Hi,

On Tue, 2014-10-07 at 15:41 -0600, Lina Iyer wrote:

Hi,

Quoted 1 line

Quoted 1 line

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

Quoted 4 lines

Quoted 1 line

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

Quoted 8 lines

Quoted 8 lines

Quoted 1 line

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.