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

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

Signed-off-by: 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:

- ***@2099000 {
+ power-***@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

Stephen Boyd

2014-10-09 01:12:03 UTC

Post by Lina Iyer
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/

Post by Lina Iyer
+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

Post by Lina Iyer
+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
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.

Post by Lina Iyer
+ 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
+ */
+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()

Post by Lina Iyer
+ 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?

Post by Lina Iyer
+ 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;

Post by Lina Iyer
+ 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?

Post by Lina Iyer
+ /* 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

Post by Lina Iyer
+
+ for_each_possible_cpu(cpu) {
+ if (drv)
+ break;

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

Post by Lina Iyer
+ 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.

Post by Lina Iyer
+ 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?

Post by Lina Iyer
+ 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.

Post by Lina Iyer
+
+ /**
+ * 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.

Post by Lina Iyer
+ 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.

Post by Lina Iyer
+ 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

Lina Iyer

2014-10-09 16:18:29 UTC

s/is/are/

Post by Lina Iyer
+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.

Post by Stephen Boyd
+

Post by Lina Iyer
+struct spm_driver_data {
+ void __iomem *reg_base_addr;

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

Sure.

Post by Stephen Boyd
+ */

Post by Lina Iyer
+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.

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

Post by Lina Iyer
+ 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.

Post by Lina Iyer
+ 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.

int instead of u32

Post by Lina Iyer
+
+ 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.

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?

Post by Stephen Boyd
+

Post by Lina Iyer
+ /* Get the SPM register data for this instance */

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

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

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.

Post by Lina Iyer
+ 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.

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.

Post by Stephen Boyd
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Stephen Boyd

2014-10-09 20:20:00 UTC

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.

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.

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.

Post by Stephen Boyd
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

Sudeep Holla

2014-10-09 16:53:32 UTC

Post by 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.
Original tree available at -
git://codeaurora.org/quic/la/kernel/msm-3.10.git
---
.../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

[...]

Post by Lina Iyer
+
+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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lina Iyer

2014-10-09 17:12:16 UTC

[...]

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.

Post by Sudeep Holla
Regards,
Sudeep

Sudeep Holla

2014-10-09 17:23:39 UTC

[...]

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

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

Lina Iyer

2014-10-09 17:25:07 UTC

[...]

Ah, Ok.

Post by Sudeep Holla
Regards,
Sudeep

Lina Iyer

2014-10-07 21:41:41 UTC

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 <***@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>;
};

***@1 {
@@ -30,6 +31,7 @@
reg = <1>;
next-level-cache = <&L2>;
qcom,acc = <&acc1>;
+ qcom,saw = <&saw1>;
};

***@2 {
@@ -39,6 +41,7 @@
reg = <2>;
next-level-cache = <&L2>;
qcom,acc = <&acc2>;
+ qcom,saw = <&saw2>;
};

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

--
1.9.1

Stephen Boyd

2014-10-07 23:17:07 UTC

Post by Lina Iyer
@@ -144,7 +148,27 @@
};
};
+ 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

Lina Iyer

2014-10-09 15:57:04 UTC

Post by Lina Iyer
@@ -144,7 +148,27 @@
};
};
+ compatible = "qcom,msm8974-saw2-v2.1-cpu";
+ reg = <0xf9089000 0x1000>;

Please add the aliases.

Will do.

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

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

Lina Iyer

2014-10-07 21:41:43 UTC

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 <***@codeaurora.org>,
Praveen Chidambaram <***@codeaurora.org>, Murali Nalajala
<***@codeaurora.org>
Original tree available at -
git://codeaurora.org/quic/la/kernel/msm-3.10.git

Signed-off-by: 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

Stephen Boyd

2014-10-09 01:17:57 UTC

Post by Lina Iyer
+
+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.

Post by Lina Iyer
+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

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

Lina Iyer

2014-10-09 15:56:43 UTC

Post by Lina Iyer
+
+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.

Post by Lina Iyer
+static int __init qcom_pm_device_init(void)
+{
+ platform_device_register(&qcom_cpuidle_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.

Post by Stephen Boyd
This is one reason why I've
been arguing to get rid of this file and just put it inside the spm

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.

Post by Stephen Boyd
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.

Post by Stephen Boyd
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.

Stephen Boyd

2014-10-09 19:00:57 UTC

Post by Lina Iyer
+static int __init qcom_pm_device_init(void)
+{
+ platform_device_register(&qcom_cpuidle_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.

Post by Stephen Boyd
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

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.

Post by Stephen Boyd
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.

Post by Stephen Boyd
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Stephen Boyd

2014-10-09 19:26:20 UTC

Post by Lina Iyer
+static int __init qcom_pm_device_init(void)
+{
+ platform_device_register(&qcom_cpuidle_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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lina Iyer

2014-10-07 21:41:42 UTC

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 <***@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>;
};

***@1 {
@@ -27,6 +28,7 @@
enable-method = "qcom,kpss-acc-v2";
next-level-cache = <&L2>;
qcom,acc = <&acc1>;
+ qcom,saw = <&saw1>;
};

***@2 {
@@ -36,6 +38,7 @@
enable-method = "qcom,kpss-acc-v2";
next-level-cache = <&L2>;
qcom,acc = <&acc2>;
+ qcom,saw = <&saw2>;
};

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

--
1.9.1

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

Lina Iyer

2014-10-07 21:41:44 UTC

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

Stephen Boyd

2014-10-09 01:22:07 UTC

Post by Lina Iyer
+
+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

Daniel Lezcano

2014-10-23 11:05:39 UTC

Add cpuidle driver interface to allow cpus to go into C-States. Use t=

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 Collaps=

e.

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

---
.../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,i=

dle-state.txt

create mode 100644 drivers/cpuidle/cpuidle-qcom.c
diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,idle-stat=

e.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 define=

d 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 clo=

ck gating.

+When the WFI instruction is executed the ARM core would gate its int=

ernal

+clocks. In addition to gating the clocks, QCOM cpus use this instruc=

tion as a

+trigger to execute the SPM state machine. The SPM state machine wai=

ts for the

+interrupt to trigger the core back in to active. This triggers the c=

ache

+heirarchy to enter standby states, when all cpus are idle. An interr=

upt brings

s/heirarchy/hierarchy/

+the SPM state machine out of its wait, the next step is to ensure th=

at 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 ga=

ted 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 proce=

ssor

+registers active. The SPM should be configured to execute the retent=

ion

+sequence and would wait for interrupt, before restoring the cpu to e=

xecution

+state. Retention may have a slightly higher latency than Standby.
+
+Standalone PC: A cpu can power down and warmboot if there is a suffi=

cient 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 an=

y other

+cpu or the system resources. This helps save power only on that core=

=2E 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=

+resume. Applying power and resetting the core causes the core to wa=

rmboot

^^ 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 b=

y 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 ca=

che has to

+be flushed in s/w, before powering down the core.
+
+Power Collapse: This state is similar to the SPC mode, but distingui=

shes

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

per sleep

+modes. In a hierarchical power domain SoC, this means L2 and other c=

aches can

+be flushed, system bus, clocks - lowered, and SoC main XO clock gate=

d and

+voltages reduced, provided all cpus enter this state. Since the spa=

n 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 c=

omplete

+application processor subsystem shut down.
+
+The idle-state for QCOM SoCs are distinguished by the compatible pro=

perty of

+the idle-states device node.
+The devicetree representation of the idle state should be -
+
+
+- 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].
+
+
+ idle-states {
+ CPU_SPC: spc {
+ compatible =3D "qcom,idle-state-spc", "arm,idle-state";
+ entry-latency-us =3D <150>;
+ exit-latency-us =3D <200>;
+ min-residency-us =3D <2000>;
+ };
+ };
+
+[1]. Documentation/devicetree/bindings/arm/idle-states.txt
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.ar=

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 processo=

rs.

+
+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) +=3D cpuidle-zynq.o
obj-$(CONFIG_ARM_U8500_CPUIDLE) +=3D cpuidle-ux500.o
obj-$(CONFIG_ARM_AT91_CPUIDLE) +=3D cpuidle-at91.o
obj-$(CONFIG_ARM_EXYNOS_CPUIDLE) +=3D cpuidle-exynos.o
+obj-$(CONFIG_ARM_QCOM_CPUIDLE) +=3D 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 mod=

ify

+ * it under the terms of the GNU General Public License version 2 an=

+ * 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=20
qcom_cpu_powerdown(void) and let behind the mysterious words 'Single=20
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 singl=
e=20
pointer in the platform data. I am working on a common structure to be=20
shared across the drivers as a common way to pass the different=20
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=20
post the patchset with the common structure.

The issue I see with this common structure is the initialization of the=
=20
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=20
time measurement will include irq time processing and will no longer be=
=20
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 =3D {
+ .name =3D "qcom_cpuidle",
+};
+
+static const struct of_device_id qcom_idle_state_match[] =3D {
+ { .compatible =3D "qcom,idle-state-stby", .data =3D qcom_lpm_enter_=

stby},

+ { .compatible =3D "qcom,idle-state-spc", .data =3D qcom_lpm_enter_s=

pc },

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

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

+ /* Probe for other states, including standby */
+ ret =3D 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=20
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 =3D {
+ .probe =3D qcom_cpuidle_probe,
+ .driver =3D {
+ .name =3D "qcom_cpuidle",
+ },
+};
+
+module_platform_driver(qcom_cpuidle_plat_driver);

--=20
<http://www.linaro.org/> Linaro.org =E2=94=82 Open source software fo=
r ARM SoCs

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

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

Lorenzo Pieralisi

2014-10-23 12:43:03 UTC

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

[...]

Post by Lina Iyer
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);
};
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

Lina Iyer

2014-10-23 16:18:15 UTC

Post by Lorenzo Pieralisi
[...]

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.

Post by Lorenzo Pieralisi
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.

Post by Lorenzo Pieralisi
Cheers,
Lorenzo

Lina Iyer

2014-10-23 16:58:08 UTC

Add cpuidle driver interface to allow cpus to go into C-States. Use t=

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 Collaps=

Post by Daniel Lezcano
Why not the retention mode which is in between the standby and the=20
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.=20

Post by Daniel Lezcano
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm

rs.

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

Post by Daniel Lezcano
+

+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=20
qcom_cpu_powerdown(void) and let behind the mysterious words 'Single=20
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=20
single pointer in the platform data. I am working on a common=20
structure to be shared across the drivers as a common way to pass the=20
different callbacks without including a soc specific header.
struct cpuidle_ops {
int (*standby)(void *data);
int (*retention)(void *data);
int (*poweroff)(void *data);
};
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=20
post the patchset with the common structure.
The issue I see with this common structure is the initialization of=20
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=

=20

Post by Daniel Lezcano
time measurement will include irq time processing and will no longer=20
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)

stby},

+ { .compatible =3D "qcom,idle-state-spc", .data =3D qcom_lpm_enter_s=

pc },

It shouldn't fail because if the probe is called then the cpuidle=20
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 =3D 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=

=20

Post by Daniel Lezcano
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 reques=
t
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.

--=20
<http://www.linaro.org/> Linaro.org =E2=94=82 Open source software fo=

r ARM SoCs

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

Lina Iyer

2014-10-07 21:41:45 UTC

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 <***@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>;
};

***@1 {
@@ -32,6 +33,7 @@
next-level-cache = <&L2>;
qcom,acc = <&acc1>;
qcom,saw = <&saw1>;
+ cpu-idle-states = <&CPU_STBY &CPU_SPC>;
};

***@2 {
@@ -42,6 +44,7 @@
next-level-cache = <&L2>;
qcom,acc = <&acc2>;
qcom,saw = <&saw2>;
+ cpu-idle-states = <&CPU_STBY &CPU_SPC>;
};

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

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

Lina Iyer

2014-10-07 21:41:46 UTC

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 <***@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>;
};

***@1 {
@@ -29,6 +30,7 @@
next-level-cache = <&L2>;
qcom,acc = <&acc1>;
qcom,saw = <&saw1>;
+ cpu-idle-states = <&CPU_STBY &CPU_SPC>;
};

***@2 {
@@ -39,6 +41,7 @@
next-level-cache = <&L2>;
qcom,acc = <&acc2>;
qcom,saw = <&saw2>;
+ cpu-idle-states = <&CPU_STBY &CPU_SPC>;
};

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

Ivan T. Ivanov

2014-10-23 15:31:57 UTC

Hi,

Post by Lina Iyer
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

Lina Iyer

2014-10-23 15:54:35 UTC

Post by Ivan T. Ivanov
Hi,

Post by Lina Iyer
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.

Post by Ivan T. Ivanov
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.

Post by Ivan T. Ivanov
Regards,
Ivan

Amit Kucheria

2014-10-24 04:21:15 UTC