* [PATCH v4 1/8] msm: scm: Move scm-boot files to drivers/soc and include/soc
2014-08-19 22:15 [PATCH v4 0/8] QCOM 8074 cpuidle driver Lina Iyer
@ 2014-08-19 22:15 ` Lina Iyer
2014-08-19 22:15 ` [PATCH v4 2/8] msm: scm: Add SCM warmboot flags for quad core targets Lina Iyer
` (6 subsequent siblings)
7 siblings, 0 replies; 26+ messages in thread
From: Lina Iyer @ 2014-08-19 22:15 UTC (permalink / raw)
To: daniel.lezcano, khilman, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi
Cc: msivasub, Lina Iyer
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
arch/arm/mach-qcom/Makefile | 1 -
arch/arm/mach-qcom/platsmp.c | 2 +-
drivers/soc/qcom/Makefile | 2 +-
{arch/arm/mach-qcom => drivers/soc/qcom}/scm-boot.c | 4 ++--
{arch/arm/mach-qcom => include/soc/qcom}/scm-boot.h | 0
5 files changed, 4 insertions(+), 5 deletions(-)
rename {arch/arm/mach-qcom => drivers/soc/qcom}/scm-boot.c (97%)
rename {arch/arm/mach-qcom => include/soc/qcom}/scm-boot.h (100%)
diff --git a/arch/arm/mach-qcom/Makefile b/arch/arm/mach-qcom/Makefile
index db41e8c..e324375 100644
--- a/arch/arm/mach-qcom/Makefile
+++ b/arch/arm/mach-qcom/Makefile
@@ -1,3 +1,2 @@
obj-y := board.o
obj-$(CONFIG_SMP) += platsmp.o
-obj-$(CONFIG_QCOM_SCM) += scm-boot.o
diff --git a/arch/arm/mach-qcom/platsmp.c b/arch/arm/mach-qcom/platsmp.c
index d690856..a692bcb 100644
--- a/arch/arm/mach-qcom/platsmp.c
+++ b/arch/arm/mach-qcom/platsmp.c
@@ -20,7 +20,7 @@
#include <asm/smp_plat.h>
-#include "scm-boot.h"
+#include <soc/qcom/scm-boot.h>
#define VDD_SC1_ARRAY_CLAMP_GFS_CTL 0x35a0
#define SCSS_CPU1CORE_RESET 0x2d80
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index a39446d..70d52ed 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -1,3 +1,3 @@
obj-$(CONFIG_QCOM_GSBI) += qcom_gsbi.o
CFLAGS_scm.o :=$(call as-instr,.arch_extension sec,-DREQUIRES_SEC=1)
-obj-$(CONFIG_QCOM_SCM) += scm.o
+obj-$(CONFIG_QCOM_SCM) += scm.o scm-boot.o
diff --git a/arch/arm/mach-qcom/scm-boot.c b/drivers/soc/qcom/scm-boot.c
similarity index 97%
rename from arch/arm/mach-qcom/scm-boot.c
rename to drivers/soc/qcom/scm-boot.c
index 5add20e..60ff7b4 100644
--- a/arch/arm/mach-qcom/scm-boot.c
+++ b/drivers/soc/qcom/scm-boot.c
@@ -17,9 +17,9 @@
#include <linux/module.h>
#include <linux/slab.h>
-#include <soc/qcom/scm.h>
-#include "scm-boot.h"
+#include <soc/qcom/scm.h>
+#include <soc/qcom/scm-boot.h>
/*
* Set the cold/warm boot address for one of the CPU cores.
diff --git a/arch/arm/mach-qcom/scm-boot.h b/include/soc/qcom/scm-boot.h
similarity index 100%
rename from arch/arm/mach-qcom/scm-boot.h
rename to include/soc/qcom/scm-boot.h
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 2/8] msm: scm: Add SCM warmboot flags for quad core targets.
2014-08-19 22:15 [PATCH v4 0/8] QCOM 8074 cpuidle driver Lina Iyer
2014-08-19 22:15 ` [PATCH v4 1/8] msm: scm: Move scm-boot files to drivers/soc and include/soc Lina Iyer
@ 2014-08-19 22:15 ` Lina Iyer
2014-08-19 22:15 ` [PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2) Lina Iyer
` (5 subsequent siblings)
7 siblings, 0 replies; 26+ messages in thread
From: Lina Iyer @ 2014-08-19 22:15 UTC (permalink / raw)
To: daniel.lezcano, khilman, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi
Cc: msivasub, Lina Iyer
Quad core targets like APQ8074, APQ8064, APQ8084 need SCM support set up
warm boot addresses in the Secure Monitor. Extend the SCM flags to
support warmboot addresses for seconday cores.
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
include/soc/qcom/scm-boot.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/soc/qcom/scm-boot.h b/include/soc/qcom/scm-boot.h
index 6aabb24..02b445c 100644
--- a/include/soc/qcom/scm-boot.h
+++ b/include/soc/qcom/scm-boot.h
@@ -18,6 +18,8 @@
#define SCM_FLAG_COLDBOOT_CPU3 0x20
#define SCM_FLAG_WARMBOOT_CPU0 0x04
#define SCM_FLAG_WARMBOOT_CPU1 0x02
+#define SCM_FLAG_WARMBOOT_CPU2 0x10
+#define SCM_FLAG_WARMBOOT_CPU3 0x40
int scm_set_boot_addr(phys_addr_t addr, int flags);
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2)
2014-08-19 22:15 [PATCH v4 0/8] QCOM 8074 cpuidle driver Lina Iyer
2014-08-19 22:15 ` [PATCH v4 1/8] msm: scm: Move scm-boot files to drivers/soc and include/soc Lina Iyer
2014-08-19 22:15 ` [PATCH v4 2/8] msm: scm: Add SCM warmboot flags for quad core targets Lina Iyer
@ 2014-08-19 22:15 ` Lina Iyer
2014-08-20 2:01 ` Stephen Boyd
2014-08-19 22:15 ` [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC Lina Iyer
` (4 subsequent siblings)
7 siblings, 1 reply; 26+ messages in thread
From: Lina Iyer @ 2014-08-19 22:15 UTC (permalink / raw)
To: daniel.lezcano, khilman, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi
Cc: msivasub, Lina Iyer, Praveen Chidambaram
SPM is a hardware block that controls the peripheral logic surrounding
the application cores (cpu/l$). When the core executes WFI instruction,
the SPM takes over the putting the core in low power state as
configured. The wake up for the SPM is an interrupt at the GIC, which
then completes the rest of low power mode sequence and brings the core
out of low power mode.
The SPM has a set of control registers that configure the SPMs
individually based on the type of the core and the runtime conditions.
SPM is a finite state machine block to which a sequence is provided and
it interprets the bytes and executes them in sequence. Each low power
mode that the core can enter into is provided to the SPM as a sequence.
Configure the SPM to set the core (cpu or L2) into its low power mode,
the index of the first command in the sequence is set in the SPM_CTL
register. When the core executes ARM wfi instruction, it triggers the
SPM state machine to start executing from that index. The SPM state
machine waits until the interrupt occurs and starts executing the rest
of the sequence until it hits the end of the sequence. The end of the
sequence jumps the core out of its low power mode.
Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/spm-drv.h | 71 +++++++++++++++++
drivers/soc/qcom/spm.c | 195 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 267 insertions(+)
create mode 100644 drivers/soc/qcom/spm-drv.h
create mode 100644 drivers/soc/qcom/spm.c
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-drv.h b/drivers/soc/qcom/spm-drv.h
new file mode 100644
index 0000000..4f41f1b
--- /dev/null
+++ b/drivers/soc/qcom/spm-drv.h
@@ -0,0 +1,71 @@
+/* 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.
+ */
+#ifndef __QCOM_SPM_DRIVER_H
+#define __QCOM_SPM_DRIVER_H
+
+enum {
+ MSM_SPM_REG_SAW2_CFG,
+ MSM_SPM_REG_SAW2_AVS_CTL,
+ MSM_SPM_REG_SAW2_AVS_HYSTERESIS,
+ MSM_SPM_REG_SAW2_SPM_CTL,
+ MSM_SPM_REG_SAW2_PMIC_DLY,
+ MSM_SPM_REG_SAW2_AVS_LIMIT,
+ MSM_SPM_REG_SAW2_AVS_DLY,
+ MSM_SPM_REG_SAW2_SPM_DLY,
+ MSM_SPM_REG_SAW2_PMIC_DATA_0,
+ MSM_SPM_REG_SAW2_PMIC_DATA_1,
+ MSM_SPM_REG_SAW2_PMIC_DATA_2,
+ MSM_SPM_REG_SAW2_PMIC_DATA_3,
+ MSM_SPM_REG_SAW2_PMIC_DATA_4,
+ MSM_SPM_REG_SAW2_PMIC_DATA_5,
+ MSM_SPM_REG_SAW2_PMIC_DATA_6,
+ MSM_SPM_REG_SAW2_PMIC_DATA_7,
+ MSM_SPM_REG_SAW2_RST,
+
+ MSM_SPM_REG_NR_INITIALIZE = MSM_SPM_REG_SAW2_RST,
+
+ MSM_SPM_REG_SAW2_ID,
+ MSM_SPM_REG_SAW2_SECURE,
+ MSM_SPM_REG_SAW2_STS0,
+ MSM_SPM_REG_SAW2_STS1,
+ MSM_SPM_REG_SAW2_STS2,
+ MSM_SPM_REG_SAW2_VCTL,
+ MSM_SPM_REG_SAW2_SEQ_ENTRY,
+ MSM_SPM_REG_SAW2_SPM_STS,
+ MSM_SPM_REG_SAW2_AVS_STS,
+ MSM_SPM_REG_SAW2_PMIC_STS,
+ MSM_SPM_REG_SAW2_VERSION,
+
+ MSM_SPM_REG_NR,
+};
+
+struct msm_spm_mode {
+ uint32_t mode;
+ uint8_t *cmd;
+ uint32_t start_addr;
+};
+
+struct msm_spm_driver_data {
+ void __iomem *reg_base_addr;
+ uint32_t reg_shadow[MSM_SPM_REG_NR];
+ uint32_t *reg_offsets;
+ struct msm_spm_mode *modes;
+ uint32_t num_modes;
+};
+
+int msm_spm_drv_init(struct msm_spm_driver_data *dev);
+int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *dev,
+ uint32_t addr);
+int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *dev,
+ bool enable);
+
+#endif /* __QCOM_SPM_DRIVER_H */
diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
new file mode 100644
index 0000000..19b79d0
--- /dev/null
+++ b/drivers/soc/qcom/spm.c
@@ -0,0 +1,195 @@
+/* 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 "spm-drv.h"
+
+#define NUM_SEQ_ENTRY 32
+
+static uint32_t msm_spm_reg_offsets_saw2_v2_1[MSM_SPM_REG_NR] = {
+ [MSM_SPM_REG_SAW2_SECURE] = 0x00,
+ [MSM_SPM_REG_SAW2_ID] = 0x04,
+ [MSM_SPM_REG_SAW2_CFG] = 0x08,
+ [MSM_SPM_REG_SAW2_SPM_STS] = 0x0C,
+ [MSM_SPM_REG_SAW2_AVS_STS] = 0x10,
+ [MSM_SPM_REG_SAW2_PMIC_STS] = 0x14,
+ [MSM_SPM_REG_SAW2_RST] = 0x18,
+ [MSM_SPM_REG_SAW2_VCTL] = 0x1C,
+ [MSM_SPM_REG_SAW2_AVS_CTL] = 0x20,
+ [MSM_SPM_REG_SAW2_AVS_LIMIT] = 0x24,
+ [MSM_SPM_REG_SAW2_AVS_DLY] = 0x28,
+ [MSM_SPM_REG_SAW2_AVS_HYSTERESIS] = 0x2C,
+ [MSM_SPM_REG_SAW2_SPM_CTL] = 0x30,
+ [MSM_SPM_REG_SAW2_SPM_DLY] = 0x34,
+ [MSM_SPM_REG_SAW2_PMIC_DATA_0] = 0x40,
+ [MSM_SPM_REG_SAW2_PMIC_DATA_1] = 0x44,
+ [MSM_SPM_REG_SAW2_PMIC_DATA_2] = 0x48,
+ [MSM_SPM_REG_SAW2_PMIC_DATA_3] = 0x4C,
+ [MSM_SPM_REG_SAW2_PMIC_DATA_4] = 0x50,
+ [MSM_SPM_REG_SAW2_PMIC_DATA_5] = 0x54,
+ [MSM_SPM_REG_SAW2_PMIC_DATA_6] = 0x58,
+ [MSM_SPM_REG_SAW2_PMIC_DATA_7] = 0x5C,
+ [MSM_SPM_REG_SAW2_SEQ_ENTRY] = 0x80,
+ [MSM_SPM_REG_SAW2_VERSION] = 0xFD0,
+};
+
+static void flush_shadow(struct msm_spm_driver_data *drv,
+ unsigned int reg_index)
+{
+ __raw_writel(drv->reg_shadow[reg_index],
+ drv->reg_base_addr + drv->reg_offsets[reg_index]);
+}
+
+static void load_shadow(struct msm_spm_driver_data *drv,
+ unsigned int reg_index)
+{
+ drv->reg_shadow[reg_index] = __raw_readl(drv->reg_base_addr +
+ drv->reg_offsets[reg_index]);
+}
+
+static inline void set_start_addr(struct msm_spm_driver_data *drv,
+ uint32_t addr)
+{
+ /* Update bits 10:4 in the SPM CTL register */
+ addr &= 0x7F;
+ addr <<= 4;
+ drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] &= 0xFFFFF80F;
+ drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] |= addr;
+}
+
+int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *drv,
+ uint32_t mode)
+{
+ int i;
+ uint32_t start_addr = 0;
+
+ for (i = 0; i < drv->num_modes; i++) {
+ if (drv->modes[i].mode == mode) {
+ start_addr = drv->modes[i].start_addr;
+ break;
+ }
+ }
+
+ if (i == drv->num_modes)
+ return -EINVAL;
+
+ set_start_addr(drv, start_addr);
+ flush_shadow(drv, MSM_SPM_REG_SAW2_SPM_CTL);
+ /* Barrier to ensure we have written the start address */
+ wmb();
+
+ /* Update our shadow with the status changes, if any */
+ load_shadow(drv, MSM_SPM_REG_SAW2_SPM_STS);
+
+ return 0;
+}
+
+int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *drv, bool enable)
+{
+ uint32_t value = enable ? 0x01 : 0x00;
+
+ /* Update BIT(0) of SPM_CTL to enable/disable the SPM */
+ if ((drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] & 0x01) ^ value) {
+ /* Clear the existing value and update */
+ drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] &= ~0x1;
+ drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] |= value;
+ flush_shadow(drv, MSM_SPM_REG_SAW2_SPM_CTL);
+ /* Ensure we have enabled/disabled before returning */
+ wmb();
+ }
+
+ return 0;
+}
+
+static void flush_seq_data(struct msm_spm_driver_data *drv,
+ uint32_t *reg_seq_entry)
+{
+ int i;
+
+ /* Write the 32 byte array into the SPM registers */
+ for (i = 0; i < NUM_SEQ_ENTRY; i++) {
+ __raw_writel(reg_seq_entry[i],
+ drv->reg_base_addr
+ + drv->reg_offsets[MSM_SPM_REG_SAW2_SEQ_ENTRY]
+ + 4 * i);
+ }
+ /* Ensure that the changes are written */
+ wmb();
+}
+
+static void write_seq_data(struct msm_spm_driver_data *drv,
+ uint32_t *reg_seq_entry, uint8_t *cmd, uint32_t *offset)
+{
+ uint32_t cmd_w;
+ uint32_t offset_w = *offset / 4;
+ uint8_t last_cmd;
+
+ while (1) {
+ int i;
+
+ cmd_w = 0;
+ last_cmd = 0;
+ cmd_w = reg_seq_entry[offset_w];
+
+ for (i = (*offset % 4); i < 4; i++) {
+ last_cmd = *(cmd++);
+ cmd_w |= last_cmd << (i * 8);
+ (*offset)++;
+ if (last_cmd == 0x0f)
+ break;
+ }
+
+ reg_seq_entry[offset_w++] = cmd_w;
+ if (last_cmd == 0x0f)
+ break;
+ }
+
+}
+
+int msm_spm_drv_init(struct msm_spm_driver_data *drv)
+{
+ int i;
+ int offset = 0;
+ uint32_t sequences[NUM_SEQ_ENTRY/sizeof(uint8_t)] = {0};
+
+ drv->reg_offsets = msm_spm_reg_offsets_saw2_v2_1;
+
+ /**
+ * Compose the uint32 array based on the individual bytes of the SPM
+ * sequence for each low power mode that we read from the DT.
+ * The sequences are appended if there is space available in the
+ * integer after the end of the previous sequence.
+ */
+ for (i = 0; i < drv->num_modes; i++) {
+ drv->modes[i].start_addr = offset;
+ write_seq_data(drv, &sequences[0], drv->modes[i].cmd, &offset);
+ }
+
+ /* Flush the integer array */
+ flush_seq_data(drv, &sequences[0]);
+
+ /**
+ * Initialize the hardware with the control registers that
+ * we have read.
+ */
+ for (i = 0; i < MSM_SPM_REG_SAW2_PMIC_DATA_0; i++)
+ flush_shadow(drv, i);
+
+ return 0;
+}
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2)
2014-08-19 22:15 ` [PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2) Lina Iyer
@ 2014-08-20 2:01 ` Stephen Boyd
2014-08-20 3:24 ` Lina Iyer
0 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2014-08-20 2:01 UTC (permalink / raw)
To: Lina Iyer
Cc: daniel.lezcano, khilman, davidb, galak, linux-arm-msm,
lorenzo.pieralisi, msivasub, Praveen Chidambaram
On 08/19/14 15:15, Lina Iyer wrote:
> SPM is a hardware block that controls the peripheral logic surrounding
> the application cores (cpu/l$). When the core executes WFI instruction,
> the SPM takes over the putting the core in low power state as
> configured. The wake up for the SPM is an interrupt at the GIC, which
> then completes the rest of low power mode sequence and brings the core
> out of low power mode.
>
> The SPM has a set of control registers that configure the SPMs
> individually based on the type of the core and the runtime conditions.
> SPM is a finite state machine block to which a sequence is provided and
> it interprets the bytes and executes them in sequence. Each low power
> mode that the core can enter into is provided to the SPM as a sequence.
>
> Configure the SPM to set the core (cpu or L2) into its low power mode,
> the index of the first command in the sequence is set in the SPM_CTL
> register. When the core executes ARM wfi instruction, it triggers the
> SPM state machine to start executing from that index. The SPM state
> machine waits until the interrupt occurs and starts executing the rest
> of the sequence until it hits the end of the sequence. The end of the
> sequence jumps the core out of its low power mode.
>
> Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
Why isn't this code combined with the cpuidle driver in qcom-cpuidle.c,
spm-devices.c, and qcom-pm.c? All these files are interacting with the
same hardware, I'm confused why we have to have 4 different files and
all these different patches to support this. Basically patches 3, 4, 6
and 7 should be one file under drivers/cpuidle/ named qcom-cpuidle or
saw-cpuidle.
>
> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
> new file mode 100644
> index 0000000..19b79d0
> --- /dev/null
> +++ b/drivers/soc/qcom/spm.c
> @@ -0,0 +1,195 @@
[..]
> +
> +static void flush_shadow(struct msm_spm_driver_data *drv,
> + unsigned int reg_index)
> +{
> + __raw_writel(drv->reg_shadow[reg_index],
> + drv->reg_base_addr + drv->reg_offsets[reg_index]);
> +}
What's the use of the "shadow" functionality? Why can't we just read and
write the registers directly without having to go through a register cache?
> +
> +static void load_shadow(struct msm_spm_driver_data *drv,
> + unsigned int reg_index)
> +{
> + drv->reg_shadow[reg_index] = __raw_readl(drv->reg_base_addr +
> + drv->reg_offsets[reg_index]);
Please use readl_relaxed/writel_relaxed. The raw accessors don't
byte-swap and fail horribly for big-endian kernels.
> +}
> +
> +static inline void set_start_addr(struct msm_spm_driver_data *drv,
> + uint32_t addr)
> +{
> + /* Update bits 10:4 in the SPM CTL register */
> + addr &= 0x7F;
> + addr <<= 4;
> + drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] &= 0xFFFFF80F;
> + drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] |= addr;
> +}
> +
> +int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *drv,
> + uint32_t mode)
> +{
> + int i;
> + uint32_t start_addr = 0;
> +
> + for (i = 0; i < drv->num_modes; i++) {
> + if (drv->modes[i].mode == mode) {
> + start_addr = drv->modes[i].start_addr;
> + break;
> + }
> + }
> +
> + if (i == drv->num_modes)
> + return -EINVAL;
> +
> + set_start_addr(drv, start_addr);
> + flush_shadow(drv, MSM_SPM_REG_SAW2_SPM_CTL);
> + /* Barrier to ensure we have written the start address */
> + wmb();
> +
> + /* Update our shadow with the status changes, if any */
> + load_shadow(drv, MSM_SPM_REG_SAW2_SPM_STS);
> +
> + return 0;
> +}
> +
> +int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *drv, bool enable)
> +{
> + uint32_t value = enable ? 0x01 : 0x00;
I would prefer u32/u8 instead of uint32_t. Makes lines shorter and
easier to read.
> +
> + /* Update BIT(0) of SPM_CTL to enable/disable the SPM */
This comment could be replaced with code that's more english-like.
#define SPM_CTL_ENABLE BIT(0)
if ((drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] & SPM_CTL_ENABLE) !=
value)
> + if ((drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] & 0x01) ^ value) {
> + /* Clear the existing value and update */
> + drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] &= ~0x1;
> + drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] |= value;
> + flush_shadow(drv, MSM_SPM_REG_SAW2_SPM_CTL);
> + /* Ensure we have enabled/disabled before returning */
> + wmb();
> + }
> +
> + return 0;
Why have a return value at all?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2)
2014-08-20 2:01 ` Stephen Boyd
@ 2014-08-20 3:24 ` Lina Iyer
2014-08-21 0:25 ` Stephen Boyd
0 siblings, 1 reply; 26+ messages in thread
From: Lina Iyer @ 2014-08-20 3:24 UTC (permalink / raw)
To: Stephen Boyd
Cc: daniel.lezcano, khilman, davidb, galak, linux-arm-msm,
lorenzo.pieralisi, msivasub, Praveen Chidambaram
On Tue, Aug 19, 2014 at 07:01:53PM -0700, Stephen Boyd wrote:
>On 08/19/14 15:15, Lina Iyer wrote:
>> SPM is a hardware block that controls the peripheral logic surrounding
>> the application cores (cpu/l$). When the core executes WFI instruction,
>> the SPM takes over the putting the core in low power state as
>> configured. The wake up for the SPM is an interrupt at the GIC, which
>> then completes the rest of low power mode sequence and brings the core
>> out of low power mode.
>>
>> The SPM has a set of control registers that configure the SPMs
>> individually based on the type of the core and the runtime conditions.
>> SPM is a finite state machine block to which a sequence is provided and
>> it interprets the bytes and executes them in sequence. Each low power
>> mode that the core can enter into is provided to the SPM as a sequence.
>>
>> Configure the SPM to set the core (cpu or L2) into its low power mode,
>> the index of the first command in the sequence is set in the SPM_CTL
>> register. When the core executes ARM wfi instruction, it triggers the
>> SPM state machine to start executing from that index. The SPM state
>> machine waits until the interrupt occurs and starts executing the rest
>> of the sequence until it hits the end of the sequence. The end of the
>> sequence jumps the core out of its low power mode.
>>
>> Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>
>Why isn't this code combined with the cpuidle driver in qcom-cpuidle.c,
>spm-devices.c, and qcom-pm.c? All these files are interacting with the
>same hardware, I'm confused why we have to have 4 different files and
>all these different patches to support this. Basically patches 3, 4, 6
>and 7 should be one file under drivers/cpuidle/ named qcom-cpuidle or
>saw-cpuidle.
They all interact with the one hardware, you are right about that. But each
of these have a responsibility and provide certain functionality that
builds up into the idle framework.
Let me explain - The hardware driver spm.c doesnt care what the code
calling the driver, intends to do. All it knows is to write to right
register. And it can write to only one SPM block. There are multiple SPM
blocks which need to be managed and thats provided by spm-devices. The
cpuidle framework or the hotplug framework needs to do the same thing.
The common functionality between idle and hotplug is abstraced out in
msm-pm.c. platsmp.c and cpuidle-qcom.c both would need the same
functionality.
The SPM is not simple register write. It needs quite a bit of
configuration and to make it functional and then you may do register
writes. SPM also provides voltage control functionality, which again
has a lot of support code that need to ensure the hardware is in the
correct state before you do that one register write to set the voltage.
Again, this and other functionality add up to a whole lot of code to be
clumped up in qcom-cpuidle.c and duplicated for hotplug again. It is
beneficial to have them this way. Bear with me, as I build up this
framework to support the idle and hotplug idle framework.
>
>>
>> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
>> new file mode 100644
>> index 0000000..19b79d0
>> --- /dev/null
>> +++ b/drivers/soc/qcom/spm.c
>> @@ -0,0 +1,195 @@
>[..]
>> +
>> +static void flush_shadow(struct msm_spm_driver_data *drv,
>> + unsigned int reg_index)
>> +{
>> + __raw_writel(drv->reg_shadow[reg_index],
>> + drv->reg_base_addr + drv->reg_offsets[reg_index]);
>> +}
>
>What's the use of the "shadow" functionality? Why can't we just read and
>write the registers directly without having to go through a register cache?
Helps speed up reads and write, when you have multiple writes. Also, is
an excellent mechanism to know the state SPM was configured to, in the
event of a mishap.
>
>> +
>> +static void load_shadow(struct msm_spm_driver_data *drv,
>> + unsigned int reg_index)
>> +{
>> + drv->reg_shadow[reg_index] = __raw_readl(drv->reg_base_addr +
>> + drv->reg_offsets[reg_index]);
>
>Please use readl_relaxed/writel_relaxed. The raw accessors don't
>byte-swap and fail horribly for big-endian kernels.
OK. But does it matter that the SoC the code is expected to run is
little-endian?
>
>> +}
>> +
>> +static inline void set_start_addr(struct msm_spm_driver_data *drv,
>> + uint32_t addr)
>> +{
>> + /* Update bits 10:4 in the SPM CTL register */
>> + addr &= 0x7F;
>> + addr <<= 4;
>> + drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] &= 0xFFFFF80F;
>> + drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] |= addr;
>> +}
>> +
>> +int msm_spm_drv_set_low_power_mode(struct msm_spm_driver_data *drv,
>> + uint32_t mode)
>> +{
>> + int i;
>> + uint32_t start_addr = 0;
>> +
>> + for (i = 0; i < drv->num_modes; i++) {
>> + if (drv->modes[i].mode == mode) {
>> + start_addr = drv->modes[i].start_addr;
>> + break;
>> + }
>> + }
>> +
>> + if (i == drv->num_modes)
>> + return -EINVAL;
>> +
>> + set_start_addr(drv, start_addr);
>> + flush_shadow(drv, MSM_SPM_REG_SAW2_SPM_CTL);
>> + /* Barrier to ensure we have written the start address */
>> + wmb();
>> +
>> + /* Update our shadow with the status changes, if any */
>> + load_shadow(drv, MSM_SPM_REG_SAW2_SPM_STS);
>> +
>> + return 0;
>> +}
>> +
>> +int msm_spm_drv_set_spm_enable(struct msm_spm_driver_data *drv, bool enable)
>> +{
>> + uint32_t value = enable ? 0x01 : 0x00;
>
>I would prefer u32/u8 instead of uint32_t. Makes lines shorter and
>easier to read.
>
Hmm.. okay
>> +
>> + /* Update BIT(0) of SPM_CTL to enable/disable the SPM */
>
>This comment could be replaced with code that's more english-like.
Ok.
>
> #define SPM_CTL_ENABLE BIT(0)
>
> if ((drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] & SPM_CTL_ENABLE) !=
>value)
>
>> + if ((drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] & 0x01) ^ value) {
>> + /* Clear the existing value and update */
>> + drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] &= ~0x1;
>> + drv->reg_shadow[MSM_SPM_REG_SAW2_SPM_CTL] |= value;
>> + flush_shadow(drv, MSM_SPM_REG_SAW2_SPM_CTL);
>> + /* Ensure we have enabled/disabled before returning */
>> + wmb();
>> + }
>> +
>> + return 0;
>
>Why have a return value at all?
>
Well, it was intended to be an export function. Made sense that way that the
caller may expect an error. if the hardware was not intialized. I
removed that code here, but did not take out the return value.
>--
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>hosted by The Linux Foundation
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2)
2014-08-20 3:24 ` Lina Iyer
@ 2014-08-21 0:25 ` Stephen Boyd
2014-08-21 15:50 ` Lina Iyer
0 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2014-08-21 0:25 UTC (permalink / raw)
To: Lina Iyer
Cc: daniel.lezcano, khilman, davidb, galak, linux-arm-msm,
lorenzo.pieralisi, msivasub, Praveen Chidambaram,
linux-arm-kernel@lists.infradead.org
On 08/19/14 20:24, Lina Iyer wrote:
> On Tue, Aug 19, 2014 at 07:01:53PM -0700, Stephen Boyd wrote:
>> On 08/19/14 15:15, Lina Iyer wrote:
>>> SPM is a hardware block that controls the peripheral logic surrounding
>>> the application cores (cpu/l$). When the core executes WFI instruction,
>>> the SPM takes over the putting the core in low power state as
>>> configured. The wake up for the SPM is an interrupt at the GIC, which
>>> then completes the rest of low power mode sequence and brings the core
>>> out of low power mode.
>>>
>>> The SPM has a set of control registers that configure the SPMs
>>> individually based on the type of the core and the runtime conditions.
>>> SPM is a finite state machine block to which a sequence is provided and
>>> it interprets the bytes and executes them in sequence. Each low power
>>> mode that the core can enter into is provided to the SPM as a sequence.
>>>
>>> Configure the SPM to set the core (cpu or L2) into its low power mode,
>>> the index of the first command in the sequence is set in the SPM_CTL
>>> register. When the core executes ARM wfi instruction, it triggers the
>>> SPM state machine to start executing from that index. The SPM state
>>> machine waits until the interrupt occurs and starts executing the rest
>>> of the sequence until it hits the end of the sequence. The end of the
>>> sequence jumps the core out of its low power mode.
>>>
>>> Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>
>> Why isn't this code combined with the cpuidle driver in qcom-cpuidle.c,
>> spm-devices.c, and qcom-pm.c? All these files are interacting with the
>> same hardware, I'm confused why we have to have 4 different files and
>> all these different patches to support this. Basically patches 3, 4, 6
>> and 7 should be one file under drivers/cpuidle/ named qcom-cpuidle or
>> saw-cpuidle.
>
> They all interact with the one hardware, you are right about that. But
> each
> of these have a responsibility and provide certain functionality that
> builds up into the idle framework.
> Let me explain - The hardware driver spm.c doesnt care what the code
> calling the driver, intends to do. All it knows is to write to right
> register. And it can write to only one SPM block. There are multiple SPM
> blocks which need to be managed and thats provided by spm-devices. The
> cpuidle framework or the hotplug framework needs to do the same thing.
> The common functionality between idle and hotplug is abstraced out in
> msm-pm.c. platsmp.c and cpuidle-qcom.c both would need the same
> functionality.
I can see the end result in the downstream vendor tree and it isn't
pretty. The code winds through a handful of different files. The spm.c
file is basically just a bunch of functions to read and write to an SPM.
By itself it's pretty much worthless and doesn't stand on its own.
spm-devices is where we would actually probe a driver for the device
that is read/written in spm.c. At the least, these two files should be
combined into one driver.
Right now just to support cpuidle it seems that it could all be in one
file. When we get to hotplug, why don't we use cpuidle_play_dead() on
ARM so that this cpuidle driver can configure the SPM for the deepest
idle state and power off the CPU? The only problem I see is that we
would need another hook for the cpu_kill() op so that we can wait for
the state machine to finish bringing down the CPU. That would remove the
need for msm-pm.c?
This would handle the cpuidle_play_dead() part. Alternatively we call
cpuidle_play_dead() from the cpu_die() hook in the platform layer, but
I'd prefer we call it directly in arch code if we can.
diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index 7c4fada440f0..fef953ecde22 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -26,6 +26,7 @@
#include <linux/completion.h>
#include <linux/cpufreq.h>
#include <linux/irq_work.h>
+#include <linux/cpuidle.h>
#include <linux/atomic.h>
#include <asm/smp.h>
@@ -290,8 +291,9 @@ void __ref cpu_die(void)
* The return path should not be used for platforms which can
* power off the CPU.
*/
- if (smp_ops.cpu_die)
- smp_ops.cpu_die(cpu);
+ if (cpuidle_play_dead())
+ if (smp_ops.cpu_die)
+ smp_ops.cpu_die(cpu);
pr_warn("CPU%u: smp_ops.cpu_die() returned, trying to resuscitate\n",
cpu);
>
> The SPM is not simple register write. It needs quite a bit of
> configuration and to make it functional and then you may do register
> writes. SPM also provides voltage control functionality, which again
> has a lot of support code that need to ensure the hardware is in the
> correct state before you do that one register write to set the voltage.
> Again, this and other functionality add up to a whole lot of code to be
> clumped up in qcom-cpuidle.c and duplicated for hotplug again. It is
> beneficial to have them this way. Bear with me, as I build up this
> framework to support the idle and hotplug idle framework.
>
Yes I'm not quite sure how we would handle adding the regulator code. In
some cases each CPU's SAW is controlling a regulator and in other cases
there's only the L2 SAW controlling a regulator. We would need these
regulators to exist even if we don't support cpuidle, so we'd need to
register it somehow. I was wondering if we shouldn't be using the
register in the SAW to change the voltage, instead we should go directly
to the PMIC and write the regulator driver on top of the PMIC interface.
The spm code could then look at those regulators to figure out what
voltage the supply is configured to so that it can be programmed into
the SPM and used during any power restoring activities. I'm not sure if
we can even write the PMIC registers though, or if those registers are
locked down thus requiring us to use the SPM interface to change
voltages. It would be nice to split this out somehow though. Another
idea would be to make some shim driver that creates two child devices
for the regulator and cpuidle drivers and have the shim driver make a
regmap that both drivers use.
>>
>>>
>>> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
>>> new file mode 100644
>>> index 0000000..19b79d0
>>> --- /dev/null
>>> +++ b/drivers/soc/qcom/spm.c
>>> @@ -0,0 +1,195 @@
>> [..]
>>> +
>>> +static void flush_shadow(struct msm_spm_driver_data *drv,
>>> + unsigned int reg_index)
>>> +{
>>> + __raw_writel(drv->reg_shadow[reg_index],
>>> + drv->reg_base_addr + drv->reg_offsets[reg_index]);
>>> +}
>>
>> What's the use of the "shadow" functionality? Why can't we just read and
>> write the registers directly without having to go through a register
>> cache?
> Helps speed up reads and write, when you have multiple writes. Also, is
> an excellent mechanism to know the state SPM was configured to, in the
> event of a mishap.
Huh? How can writing something into memory and then writing it to the
device be faster than writing it directly to the device? If we need to
know how the SPM was configured I assume we would know by printks or by
inspecting the code. I still don't see a reason for this.
>>
>>> +
>>> +static void load_shadow(struct msm_spm_driver_data *drv,
>>> + unsigned int reg_index)
>>> +{
>>> + drv->reg_shadow[reg_index] = __raw_readl(drv->reg_base_addr +
>>> + drv->reg_offsets[reg_index]);
>>
>> Please use readl_relaxed/writel_relaxed. The raw accessors don't
>> byte-swap and fail horribly for big-endian kernels.
> OK. But does it matter that the SoC the code is expected to run is
> little-endian?
big-endian kernels work on these platforms. Nobody is extensively
testing it but putting down more roadblocks to using it isn't helpful.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2)
2014-08-21 0:25 ` Stephen Boyd
@ 2014-08-21 15:50 ` Lina Iyer
0 siblings, 0 replies; 26+ messages in thread
From: Lina Iyer @ 2014-08-21 15:50 UTC (permalink / raw)
To: Stephen Boyd
Cc: daniel.lezcano, khilman, davidb, galak, linux-arm-msm,
lorenzo.pieralisi, msivasub, Praveen Chidambaram,
linux-arm-kernel@lists.infradead.org
On Wed, Aug 20, 2014 at 05:25:49PM -0700, Stephen Boyd wrote:
>On 08/19/14 20:24, Lina Iyer wrote:
>> On Tue, Aug 19, 2014 at 07:01:53PM -0700, Stephen Boyd wrote:
>>> On 08/19/14 15:15, Lina Iyer wrote:
>>>> SPM is a hardware block that controls the peripheral logic surrounding
>>>> the application cores (cpu/l$). When the core executes WFI instruction,
>>>> the SPM takes over the putting the core in low power state as
>>>> configured. The wake up for the SPM is an interrupt at the GIC, which
>>>> then completes the rest of low power mode sequence and brings the core
>>>> out of low power mode.
>>>>
>>>> The SPM has a set of control registers that configure the SPMs
>>>> individually based on the type of the core and the runtime conditions.
>>>> SPM is a finite state machine block to which a sequence is provided and
>>>> it interprets the bytes and executes them in sequence. Each low power
>>>> mode that the core can enter into is provided to the SPM as a sequence.
>>>>
>>>> Configure the SPM to set the core (cpu or L2) into its low power mode,
>>>> the index of the first command in the sequence is set in the SPM_CTL
>>>> register. When the core executes ARM wfi instruction, it triggers the
>>>> SPM state machine to start executing from that index. The SPM state
>>>> machine waits until the interrupt occurs and starts executing the rest
>>>> of the sequence until it hits the end of the sequence. The end of the
>>>> sequence jumps the core out of its low power mode.
>>>>
>>>> Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
>>>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>>
>>> Why isn't this code combined with the cpuidle driver in qcom-cpuidle.c,
>>> spm-devices.c, and qcom-pm.c? All these files are interacting with the
>>> same hardware, I'm confused why we have to have 4 different files and
>>> all these different patches to support this. Basically patches 3, 4, 6
>>> and 7 should be one file under drivers/cpuidle/ named qcom-cpuidle or
>>> saw-cpuidle.
>>
>> They all interact with the one hardware, you are right about that. But
>> each
>> of these have a responsibility and provide certain functionality that
>> builds up into the idle framework.
>> Let me explain - The hardware driver spm.c doesnt care what the code
>> calling the driver, intends to do. All it knows is to write to right
>> register. And it can write to only one SPM block. There are multiple SPM
>> blocks which need to be managed and thats provided by spm-devices. The
>> cpuidle framework or the hotplug framework needs to do the same thing.
>> The common functionality between idle and hotplug is abstraced out in
>> msm-pm.c. platsmp.c and cpuidle-qcom.c both would need the same
>> functionality.
>
>I can see the end result in the downstream vendor tree and it isn't
>pretty. The code winds through a handful of different files. The spm.c
>file is basically just a bunch of functions to read and write to an SPM.
>By itself it's pretty much worthless and doesn't stand on its own.
I wouldnt put it that way. It abstracts a whole of device functionality
away from rest of the code. If look at the history of the driver in the
downstream, you would undertand that multiple version of SPM exists and
at some point, they all were supported by the same code base. The spm.c
is valuable that way in abstracting the bit manipulations away from
managing the device. Different version manipulate the same registers
differently and I can guarantee that this wont be the last of the h/w
revision either. A separate file helps keep the implementations clean
and manage different versions of SPM better.
>spm-devices is where we would actually probe a driver for the device
>that is read/written in spm.c. At the least, these two files should be
>combined into one driver.
>
Thats very simplistic approach. Reiterating, what I mention earlier, the
driver when clubbed, would get complicated and real huge, very soon.
>Right now just to support cpuidle it seems that it could all be in one
>file. When we get to hotplug, why don't we use cpuidle_play_dead() on
>ARM so that this cpuidle driver can configure the SPM for the deepest
>idle state and power off the CPU? The only problem I see is that we
>would need another hook for the cpu_kill() op so that we can wait for
>the state machine to finish bringing down the CPU. That would remove the
>need for msm-pm.c?
I will explore the cpuidle_play_dead() to see how much we can reuse
there.
This file is at its very infancy, only switches the low power mode to the
right function, but wait until you have to handle two different kinds of
processors - Krait and vanilla ARM cores using the same code base for
the same vendor. And the many corner cases you need to solve. What we do
if we remove msm-pm.c at this point is build up all the code in cpuidle
and when it comes to differenciating and handle corner cases, we will
split the file again to abstract things better.
If you had followed the previous versions of this patch, you would
realize, that I had pruned a whole lot of code. Removing this file would
make putting those code back really difficult and the driver needs all that
code.
Downstream is dictated by the milliamp of power saving you can squeeze
out of the SoC and it is inevitable that we would have to handle atleast
some of these usecases. Broad spectrum generic low power mode, barely
gets the debug boards running. If our goal is to have upstream code
any close to being as functional as downstream, you would need to do a
multitude of things and clubbing them all in one procedural file does
not help the effort. The cover letter explains how the hierarchy is and
the patches will evolve the hierarchy to full functionality.
>This would handle the cpuidle_play_dead() part. Alternatively we call
>cpuidle_play_dead() from the cpu_die() hook in the platform layer, but
>I'd prefer we call it directly in arch code if we can.
>
>diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
>index 7c4fada440f0..fef953ecde22 100644
>--- a/arch/arm/kernel/smp.c
>+++ b/arch/arm/kernel/smp.c
>@@ -26,6 +26,7 @@
> #include <linux/completion.h>
> #include <linux/cpufreq.h>
> #include <linux/irq_work.h>
>+#include <linux/cpuidle.h>
>
> #include <linux/atomic.h>
> #include <asm/smp.h>
>@@ -290,8 +291,9 @@ void __ref cpu_die(void)
> * The return path should not be used for platforms which can
> * power off the CPU.
> */
>- if (smp_ops.cpu_die)
>- smp_ops.cpu_die(cpu);
>+ if (cpuidle_play_dead())
>+ if (smp_ops.cpu_die)
>+ smp_ops.cpu_die(cpu);
>
> pr_warn("CPU%u: smp_ops.cpu_die() returned, trying to resuscitate\n",
> cpu);
>
>
>>
>> The SPM is not simple register write. It needs quite a bit of
>> configuration and to make it functional and then you may do register
>> writes. SPM also provides voltage control functionality, which again
>> has a lot of support code that need to ensure the hardware is in the
>> correct state before you do that one register write to set the voltage.
>> Again, this and other functionality add up to a whole lot of code to be
>> clumped up in qcom-cpuidle.c and duplicated for hotplug again. It is
>> beneficial to have them this way. Bear with me, as I build up this
>> framework to support the idle and hotplug idle framework.
>>
>
>Yes I'm not quite sure how we would handle adding the regulator code. In
>some cases each CPU's SAW is controlling a regulator and in other cases
>there's only the L2 SAW controlling a regulator. We would need these
>regulators to exist even if we don't support cpuidle, so we'd need to
>register it somehow. I was wondering if we shouldn't be using the
>register in the SAW to change the voltage, instead we should go directly
>to the PMIC and write the regulator driver on top of the PMIC interface.
>The spm code could then look at those regulators to figure out what
>voltage the supply is configured to so that it can be programmed into
>the SPM and used during any power restoring activities. I'm not sure if
>we can even write the PMIC registers though, or if those registers are
>locked down thus requiring us to use the SPM interface to change
>voltages. It would be nice to split this out somehow though. Another
>idea would be to make some shim driver that creates two child devices
>for the regulator and cpuidle drivers and have the shim driver make a
>regmap that both drivers use.
>
There is not just one SPM in the system. If you can split between the
regulator and idle functionality of the same hardware, imaging having
10s of such SPMs that you need to manage. Like you mention, the
functionality of an instance of SPM varies depending on the SoC
architecture. And to keep a track of which SPM changes the voltage is
not functionality that you want the regulator code to remember. Today
you might get by, bit banging or I2C or SPMI write to the PMIC, but it
inevitable in the future and it would unacceptably inefficient to do so.
>>>>
>>>> diff --git a/drivers/soc/qcom/spm.c b/drivers/soc/qcom/spm.c
>>>> new file mode 100644
>>>> index 0000000..19b79d0
>>>> --- /dev/null
>>>> +++ b/drivers/soc/qcom/spm.c
>>>> @@ -0,0 +1,195 @@
>>> [..]
>>>> +
>>>> +static void flush_shadow(struct msm_spm_driver_data *drv,
>>>> + unsigned int reg_index)
>>>> +{
>>>> + __raw_writel(drv->reg_shadow[reg_index],
>>>> + drv->reg_base_addr + drv->reg_offsets[reg_index]);
>>>> +}
>>>
>>> What's the use of the "shadow" functionality? Why can't we just read and
>>> write the registers directly without having to go through a register
>>> cache?
>> Helps speed up reads and write, when you have multiple writes. Also, is
>> an excellent mechanism to know the state SPM was configured to, in the
>> event of a mishap.
>
>Huh? How can writing something into memory and then writing it to the
>device be faster than writing it directly to the device? If we need to
>know how the SPM was configured I assume we would know by printks or by
>inspecting the code. I still don't see a reason for this.
>
It is if you dont flush after every write. There are explicit flushes to
help flush the whole register set, whenever many elements in the set are
modified. printks/traces are not very helpful when the log buffer is
not completely written in the power down/up path. When you dont
have a running kernel, looking at system state and predicting what could
have happened to crash the system or not jump back into warmboot entry
point, is very tough. This is even more bare than the bare minimum of
code that you need to debug and maintain a PM driver on a production device.
>>>> +
>>>> +static void load_shadow(struct msm_spm_driver_data *drv,
>>>> + unsigned int reg_index)
>>>> +{
>>>> + drv->reg_shadow[reg_index] = __raw_readl(drv->reg_base_addr +
>>>> + drv->reg_offsets[reg_index]);
>>>
>>> Please use readl_relaxed/writel_relaxed. The raw accessors don't
>>> byte-swap and fail horribly for big-endian kernels.
>> OK. But does it matter that the SoC the code is expected to run is
>> little-endian?
>
>big-endian kernels work on these platforms. Nobody is extensively
>testing it but putting down more roadblocks to using it isn't helpful.
>
Fair enough. I will change the functions.
Thanks for your time.
Lina
>--
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>hosted by The Linux Foundation
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC
2014-08-19 22:15 [PATCH v4 0/8] QCOM 8074 cpuidle driver Lina Iyer
` (2 preceding siblings ...)
2014-08-19 22:15 ` [PATCH v4 3/8] qcom: spm: Add Subsystem Power Manager driver (SAW2) Lina Iyer
@ 2014-08-19 22:15 ` Lina Iyer
2014-08-25 23:40 ` Stephen Boyd
2014-08-27 14:00 ` Kumar Gala
2014-08-19 22:15 ` [PATCH v4 5/8] arm: dts: qcom: Add SPM device bindings for 8974 Lina Iyer
` (3 subsequent siblings)
7 siblings, 2 replies; 26+ messages in thread
From: Lina Iyer @ 2014-08-19 22:15 UTC (permalink / raw)
To: daniel.lezcano, khilman, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi
Cc: msivasub, Lina Iyer, Praveen Chidamabram, Murali Nalajala
Each cpu or an L2$ has an SPM device. They are identical instances of
the same SPM block. This allows for multiple instances be grouped and
managed collectively. spm-devices.c is the SPM device manager managing
multiple SPM devices on top of the driver layer.
Device configuration of each SPM is picked up from the DTS. The hardware
configuration of each of the SPM is handled by the spm.c driver.
Signed-off-by: Praveen Chidamabram <pchidamb@codeaurora.org>
Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
Documentation/devicetree/bindings/arm/msm/spm.txt | 47 +++++
drivers/soc/qcom/Kconfig | 8 +
drivers/soc/qcom/Makefile | 2 +-
drivers/soc/qcom/spm-devices.c | 198 ++++++++++++++++++++++
include/soc/qcom/spm.h | 34 ++++
5 files changed, 288 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
create mode 100644 drivers/soc/qcom/spm-devices.c
create mode 100644 include/soc/qcom/spm.h
diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
new file mode 100644
index 0000000..318e024
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
@@ -0,0 +1,47 @@
+* Subsystem Power Manager (SAW2)
+
+S4 generation of MSMs have SPM hardware blocks to control the Application
+Processor Sub-System power. These SPM blocks run individual state machine
+to determine what the core (L2 or Krait/Scorpion) would do when the WFI
+instruction is executed by the core.
+
+The devicetree representation of the SPM block should be:
+
+Required properties
+
+- compatible: Could be one of -
+ "qcom,spm-v2.1"
+ "qcom,spm-v3.0"
+- reg: The physical address and the size of the SPM's memory mapped registers
+- qcom,cpu: phandle for the CPU that the SPM block is attached to.
+ This field is required on only for SPMs that control the CPU.
+- qcom,saw2-cfg: SAW2 configuration register
+- qcom,saw2-spm-dly: Provides the values for the SPM delay command in the SPM
+ sequence
+- qcom,saw2-spm-ctl: The SPM control register
+
+Optional properties
+
+- qcom,saw2-spm-cmd-wfi: The WFI command sequence
+- qcom,saw2-spm-cmd-ret: The Retention command sequence
+- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
+- qcom,saw2-spm-cmd-pc: The Power Collapse command sequence. This sequence may
+ turn off other SoC components.
+- qcom,saw2-spm-cmd-gdhs: GDHS (Globally Distributed Head Switch) command
+ sequence. This sequence will retain the memory but turn off the logic.
+-
+Example:
+ spm@f9089000 {
+ compatible = "qcom,spm-v2.1";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0xf9089000 0x1000>;
+ qcom,cpu = <&CPU0>;
+ qcom,saw2-cfg = <0x1>;
+ qcom,saw2-spm-dly= <0x20000400>;
+ qcom,saw2-spm-ctl = <0x1>;
+ qcom,saw2-spm-cmd-wfi = [03 0b 0f];
+ qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
+ a0 b0 03 68 70 3b 92 a0 b0
+ 82 2b 50 10 30 02 22 30 0f];
+ };
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 7dcd554..d467767 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -11,3 +11,11 @@ config QCOM_GSBI
config QCOM_SCM
bool
+
+config QCOM_PM
+ bool "Qualcomm Power Management"
+ depends on PM && ARCH_QCOM && OF
+ 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 20b329f..9457b2a 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 spm-devices.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-devices.c b/drivers/soc/qcom/spm-devices.c
new file mode 100644
index 0000000..2175a81
--- /dev/null
+++ b/drivers/soc/qcom/spm-devices.c
@@ -0,0 +1,198 @@
+/* Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+
+#include <soc/qcom/spm.h>
+
+#include "spm-drv.h"
+
+/**
+ * All related information for an SPM device
+ * Helps manage the collective.
+ */
+struct msm_spm_device {
+ bool initialized;
+ struct msm_spm_driver_data drv;
+};
+
+static DEFINE_PER_CPU_SHARED_ALIGNED(struct msm_spm_device, msm_cpu_spm_device);
+
+/**
+ * msm_spm_set_low_power_mode() - Configure SPM start address for low power mode
+ * @mode: SPM LPM mode to enter
+ */
+int msm_spm_set_low_power_mode(unsigned int mode)
+{
+ struct msm_spm_device *dev = &__get_cpu_var(msm_cpu_spm_device);
+ int ret = -EINVAL;
+
+ if (!dev->initialized)
+ return -ENXIO;
+
+ if (mode == MSM_SPM_MODE_DISABLED)
+ ret = msm_spm_drv_set_spm_enable(&dev->drv, false);
+ else if (!msm_spm_drv_set_spm_enable(&dev->drv, true))
+ ret = msm_spm_drv_set_low_power_mode(&dev->drv, mode);
+
+ return ret;
+}
+EXPORT_SYMBOL(msm_spm_set_low_power_mode);
+
+static int get_cpu_id(struct device_node *node)
+{
+ struct device_node *cpu_node;
+ u32 cpu;
+ int ret = -EINVAL;
+ char *key = "qcom,cpu";
+
+ cpu_node = of_parse_phandle(node, key, 0);
+ if (cpu_node) {
+ for_each_possible_cpu(cpu) {
+ if (of_get_cpu_node(cpu, NULL) == cpu_node)
+ return cpu;
+ }
+ }
+ return ret;
+}
+
+static struct msm_spm_device *msm_spm_get_device(struct platform_device *pdev)
+{
+ struct msm_spm_device *dev = NULL;
+ int cpu = get_cpu_id(pdev->dev.of_node);
+
+ if ((cpu >= 0) && cpu < num_possible_cpus())
+ dev = &per_cpu(msm_cpu_spm_device, cpu);
+
+ return dev;
+}
+
+static int msm_spm_dev_probe(struct platform_device *pdev)
+{
+ int ret;
+ int i;
+ struct device_node *node = pdev->dev.of_node;
+ char *key;
+ uint32_t val;
+ struct msm_spm_mode modes[MSM_SPM_MODE_NR];
+ struct msm_spm_device *spm_dev;
+ struct resource *res;
+ uint32_t mode_count = 0;
+
+ struct spm_of {
+ char *key;
+ uint32_t id;
+ };
+
+ /* SPM Configuration registers */
+ struct spm_of spm_of_data[] = {
+ {"qcom,saw2-clk-div", MSM_SPM_REG_SAW2_CFG},
+ {"qcom,saw2-enable", MSM_SPM_REG_SAW2_SPM_CTL},
+ {"qcom,saw2-delays", MSM_SPM_REG_SAW2_SPM_DLY},
+ };
+
+ /* SPM sleep sequences */
+ struct spm_of mode_of_data[] = {
+ {"qcom,saw2-spm-cmd-wfi", MSM_SPM_MODE_CLOCK_GATING},
+ {"qcom,saw2-spm-cmd-spc", MSM_SPM_MODE_POWER_COLLAPSE},
+ {"qcom,saw2-spm-cmd-ret", MSM_SPM_MODE_RETENTION},
+ };
+
+ /* Get the right SPM device */
+ spm_dev = msm_spm_get_device(pdev);
+ if (IS_ERR_OR_NULL(spm_dev))
+ return -EINVAL;
+
+ /* Get the SAW start address */
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ ret = -EINVAL;
+ goto fail;
+ }
+ spm_dev->drv.reg_base_addr = devm_ioremap(&pdev->dev, res->start,
+ resource_size(res));
+ if (!spm_dev->drv.reg_base_addr) {
+ ret = -ENOMEM;
+ goto fail;
+ }
+
+ /* Read the SPM configuration register values */
+ for (i = 0; i < ARRAY_SIZE(spm_of_data); i++) {
+ ret = of_property_read_u32(node, spm_of_data[i].key, &val);
+ if (ret)
+ continue;
+ spm_dev->drv.reg_shadow[spm_of_data[i].id] = val;
+ }
+
+ /* Read the byte arrays for the SPM sleep sequences */
+ for (i = 0; i < ARRAY_SIZE(mode_of_data); i++) {
+ modes[mode_count].start_addr = 0;
+ key = mode_of_data[i].key;
+ modes[mode_count].cmd =
+ (uint8_t *)of_get_property(node, key, &val);
+ if (!modes[mode_count].cmd)
+ continue;
+ modes[mode_count].mode = mode_of_data[i].id;
+ mode_count++;
+ }
+
+ spm_dev->drv.modes = devm_kcalloc(&pdev->dev, mode_count,
+ sizeof(modes[0]), GFP_KERNEL);
+ if (!spm_dev->drv.modes)
+ return -ENOMEM;
+ spm_dev->drv.num_modes = mode_count;
+ memcpy(spm_dev->drv.modes, &modes[0], sizeof(modes[0]) * mode_count);
+
+ /* Initialize the hardware */
+ ret = msm_spm_drv_init(&spm_dev->drv);
+ if (ret) {
+ kfree(spm_dev->drv.modes);
+ return ret;
+ }
+
+ spm_dev->initialized = true;
+ return ret;
+
+fail:
+ dev_err(&pdev->dev, "SPM device probe failed: %d\n", ret);
+ return ret;
+}
+
+static struct of_device_id msm_spm_match_table[] = {
+ {.compatible = "qcom,spm-v2.1"},
+ {},
+};
+
+static struct platform_driver msm_spm_device_driver = {
+ .probe = msm_spm_dev_probe,
+ .driver = {
+ .name = "spm-v2",
+ .owner = THIS_MODULE,
+ .of_match_table = msm_spm_match_table,
+ },
+};
+
+static int __init msm_spm_device_init(void)
+{
+ return platform_driver_register(&msm_spm_device_driver);
+}
+device_initcall(msm_spm_device_init);
diff --git a/include/soc/qcom/spm.h b/include/soc/qcom/spm.h
new file mode 100644
index 0000000..99a1d12
--- /dev/null
+++ b/include/soc/qcom/spm.h
@@ -0,0 +1,34 @@
+/* Copyright (c) 2010-2014, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __QCOM_SPM_H
+#define __QCOM_SPM_H
+
+enum {
+ MSM_SPM_MODE_DISABLED,
+ MSM_SPM_MODE_CLOCK_GATING,
+ MSM_SPM_MODE_RETENTION,
+ MSM_SPM_MODE_GDHS,
+ MSM_SPM_MODE_POWER_COLLAPSE,
+ MSM_SPM_MODE_NR
+};
+
+struct msm_spm_device;
+
+#if defined(CONFIG_QCOM_PM)
+int msm_spm_set_low_power_mode(unsigned int mode);
+#else /* defined(CONFIG_QCOM_PM) */
+static inline int msm_spm_set_low_power_mode(unsigned int mode)
+{ return -ENOSYS; }
+#endif /* defined (CONFIG_QCOM_PM) */
+
+#endif /* __QCOM_SPM_H */
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC
2014-08-19 22:15 ` [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC Lina Iyer
@ 2014-08-25 23:40 ` Stephen Boyd
2014-08-26 0:31 ` Lina Iyer
2014-08-27 14:00 ` Kumar Gala
1 sibling, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2014-08-25 23:40 UTC (permalink / raw)
To: Lina Iyer
Cc: daniel.lezcano, khilman, davidb, galak, linux-arm-msm,
lorenzo.pieralisi, msivasub, Praveen Chidamabram, Murali Nalajala
On 08/19/14 15:15, Lina Iyer wrote:
> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
> new file mode 100644
> index 0000000..318e024
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
We already have a binding document for SAW. Please add stuff there
because SPM is just a component of SAW.
> @@ -0,0 +1,47 @@
> +* Subsystem Power Manager (SAW2)
> +
> +S4 generation of MSMs have SPM hardware blocks to control the Application
> +Processor Sub-System power. These SPM blocks run individual state machine
> +to determine what the core (L2 or Krait/Scorpion) would do when the WFI
> +instruction is executed by the core.
> +
> +The devicetree representation of the SPM block should be:
> +
> +Required properties
> +
> +- compatible: Could be one of -
> + "qcom,spm-v2.1"
> + "qcom,spm-v3.0"
> +- reg: The physical address and the size of the SPM's memory mapped registers
> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
> + This field is required on only for SPMs that control the CPU.
We have a phandle from the CPU/L2 to the SAW, so this isn't necessary.
> +- qcom,saw2-cfg: SAW2 configuration register
Why? Compatible should indicate where this register is.
> +- qcom,saw2-spm-dly: Provides the values for the SPM delay command in the SPM
> + sequence
This is actually three values packed into one register for three
different selectable delays, right? We don't typically do register jam
tables in DT. Perhaps it should be split out into 3 different
properties. Or maybe it shouldn't be specified in DT at all and should
be determined algorithmically from the command sequences? From what I
can tell most of the sequences don't even use these delays.
> +- qcom,saw2-spm-ctl: The SPM control register
Why? Compatible should indicate where this register is.
> +
> +Optional properties
> +
> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence
> +- qcom,saw2-spm-cmd-ret: The Retention command sequence
> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
> +- qcom,saw2-spm-cmd-pc: The Power Collapse command sequence. This sequence may
> + turn off other SoC components.
> +- qcom,saw2-spm-cmd-gdhs: GDHS (Globally Distributed Head Switch) command
> + sequence. This sequence will retain the memory but turn off the logic.
I wonder if these should be properties of the idle states? That way the
driver isn't searching for them by name in DT, instead it knows what
state is associated with what sequence that the SPM needs to have
programmed.
> +-
> +Example:
> + spm@f9089000 {
> + compatible = "qcom,spm-v2.1";
> + #address-cells = <1>;
> + #size-cells = <1>;
Why is this in the example? Are there subnodes?
> + reg = <0xf9089000 0x1000>;
> + qcom,cpu = <&CPU0>;
> + qcom,saw2-cfg = <0x1>;
> + qcom,saw2-spm-dly= <0x20000400>;
> + qcom,saw2-spm-ctl = <0x1>;
> + qcom,saw2-spm-cmd-wfi = [03 0b 0f];
> + qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
> + a0 b0 03 68 70 3b 92 a0 b0
> + 82 2b 50 10 30 02 22 30 0f];
> + };
> diff --git a/drivers/soc/qcom/spm-devices.c b/drivers/soc/qcom/spm-devices.c
> new file mode 100644
> index 0000000..2175a81
> --- /dev/null
> +++ b/drivers/soc/qcom/spm-devices.c
> @@ -0,0 +1,198 @@
> +/* Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/err.h>
> +#include <linux/platform_device.h>
> +
> +#include <soc/qcom/spm.h>
> +
> +#include "spm-drv.h"
> +
> +/**
> + * All related information for an SPM device
> + * Helps manage the collective.
> + */
> +struct msm_spm_device {
> + bool initialized;
> + struct msm_spm_driver_data drv;
> +};
> +
> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct msm_spm_device, msm_cpu_spm_device);
> +
> +/**
> + * msm_spm_set_low_power_mode() - Configure SPM start address for low power mode
> + * @mode: SPM LPM mode to enter
> + */
> +int msm_spm_set_low_power_mode(unsigned int mode)
> +{
> + struct msm_spm_device *dev = &__get_cpu_var(msm_cpu_spm_device);
> + int ret = -EINVAL;
> +
> + if (!dev->initialized)
> + return -ENXIO;
> +
> + if (mode == MSM_SPM_MODE_DISABLED)
> + ret = msm_spm_drv_set_spm_enable(&dev->drv, false);
> + else if (!msm_spm_drv_set_spm_enable(&dev->drv, true))
> + ret = msm_spm_drv_set_low_power_mode(&dev->drv, mode);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(msm_spm_set_low_power_mode);
> +
> +static int get_cpu_id(struct device_node *node)
> +{
> + struct device_node *cpu_node;
> + u32 cpu;
> + int ret = -EINVAL;
> + char *key = "qcom,cpu";
> +
> + cpu_node = of_parse_phandle(node, key, 0);
> + if (cpu_node) {
> + for_each_possible_cpu(cpu) {
> + if (of_get_cpu_node(cpu, NULL) == cpu_node)
> + return cpu;
> + }
> + }
> + return ret;
> +}
> +
> +static struct msm_spm_device *msm_spm_get_device(struct platform_device *pdev)
> +{
> + struct msm_spm_device *dev = NULL;
> + int cpu = get_cpu_id(pdev->dev.of_node);
> +
> + if ((cpu >= 0) && cpu < num_possible_cpus())
> + dev = &per_cpu(msm_cpu_spm_device, cpu);
> +
> + return dev;
> +}
> +
> +static int msm_spm_dev_probe(struct platform_device *pdev)
> +{
> + int ret;
> + int i;
> + struct device_node *node = pdev->dev.of_node;
> + char *key;
> + uint32_t val;
> + struct msm_spm_mode modes[MSM_SPM_MODE_NR];
> + struct msm_spm_device *spm_dev;
> + struct resource *res;
> + uint32_t mode_count = 0;
> +
> + struct spm_of {
> + char *key;
> + uint32_t id;
> + };
> +
> + /* SPM Configuration registers */
> + struct spm_of spm_of_data[] = {
> + {"qcom,saw2-clk-div", MSM_SPM_REG_SAW2_CFG},
> + {"qcom,saw2-enable", MSM_SPM_REG_SAW2_SPM_CTL},
> + {"qcom,saw2-delays", MSM_SPM_REG_SAW2_SPM_DLY},
> + };
> +
> + /* SPM sleep sequences */
> + struct spm_of mode_of_data[] = {
> + {"qcom,saw2-spm-cmd-wfi", MSM_SPM_MODE_CLOCK_GATING},
> + {"qcom,saw2-spm-cmd-spc", MSM_SPM_MODE_POWER_COLLAPSE},
> + {"qcom,saw2-spm-cmd-ret", MSM_SPM_MODE_RETENTION},
> + };
> +
> + /* Get the right SPM device */
> + spm_dev = msm_spm_get_device(pdev);
> + if (IS_ERR_OR_NULL(spm_dev))
> + return -EINVAL;
> +
> + /* Get the SAW start address */
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + ret = -EINVAL;
> + goto fail;
> + }
> + spm_dev->drv.reg_base_addr = devm_ioremap(&pdev->dev, res->start,
> + resource_size(res));
> + if (!spm_dev->drv.reg_base_addr) {
> + ret = -ENOMEM;
> + goto fail;
> + }
> +
> + /* Read the SPM configuration register values */
> + for (i = 0; i < ARRAY_SIZE(spm_of_data); i++) {
> + ret = of_property_read_u32(node, spm_of_data[i].key, &val);
> + if (ret)
> + continue;
> + spm_dev->drv.reg_shadow[spm_of_data[i].id] = val;
> + }
> +
> + /* Read the byte arrays for the SPM sleep sequences */
> + for (i = 0; i < ARRAY_SIZE(mode_of_data); i++) {
> + modes[mode_count].start_addr = 0;
> + key = mode_of_data[i].key;
> + modes[mode_count].cmd =
> + (uint8_t *)of_get_property(node, key, &val);
> + if (!modes[mode_count].cmd)
> + continue;
> + modes[mode_count].mode = mode_of_data[i].id;
> + mode_count++;
> + }
> +
> + spm_dev->drv.modes = devm_kcalloc(&pdev->dev, mode_count,
> + sizeof(modes[0]), GFP_KERNEL);
> + if (!spm_dev->drv.modes)
> + return -ENOMEM;
> + spm_dev->drv.num_modes = mode_count;
> + memcpy(spm_dev->drv.modes, &modes[0], sizeof(modes[0]) * mode_count);
> +
> + /* Initialize the hardware */
> + ret = msm_spm_drv_init(&spm_dev->drv);
> + if (ret) {
> + kfree(spm_dev->drv.modes);
> + return ret;
> + }
> +
> + spm_dev->initialized = true;
> + return ret;
> +
> +fail:
> + dev_err(&pdev->dev, "SPM device probe failed: %d\n", ret);
> + return ret;
> +}
> +
> +static struct of_device_id msm_spm_match_table[] = {
const.
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC
2014-08-25 23:40 ` Stephen Boyd
@ 2014-08-26 0:31 ` Lina Iyer
2014-08-26 2:17 ` Stephen Boyd
0 siblings, 1 reply; 26+ messages in thread
From: Lina Iyer @ 2014-08-26 0:31 UTC (permalink / raw)
To: Stephen Boyd
Cc: daniel.lezcano, khilman, davidb, galak, linux-arm-msm,
lorenzo.pieralisi, msivasub, Praveen Chidamabram, Murali Nalajala
On Mon, Aug 25, 2014 at 04:40:33PM -0700, Stephen Boyd wrote:
>On 08/19/14 15:15, Lina Iyer wrote:
>> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
>> new file mode 100644
>> index 0000000..318e024
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
>
>We already have a binding document for SAW. Please add stuff there
>because SPM is just a component of SAW.
>
I agree that SPM is just a component of the SAW. But the document there
seems to indicate regulator details, totally unrelated to the actual SAW
hardware functionality.
>> @@ -0,0 +1,47 @@
>> +* Subsystem Power Manager (SAW2)
>> +
>> +S4 generation of MSMs have SPM hardware blocks to control the Application
>> +Processor Sub-System power. These SPM blocks run individual state machine
>> +to determine what the core (L2 or Krait/Scorpion) would do when the WFI
>> +instruction is executed by the core.
>> +
>> +The devicetree representation of the SPM block should be:
>> +
>> +Required properties
>> +
>> +- compatible: Could be one of -
>> + "qcom,spm-v2.1"
>> + "qcom,spm-v3.0"
>> +- reg: The physical address and the size of the SPM's memory mapped registers
>> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
>> + This field is required on only for SPMs that control the CPU.
>
>We have a phandle from the CPU/L2 to the SAW, so this isn't necessary.
>
Sorry, I dont understand. Care to explain pls? Its necessary to know
what SPM instance controls which CPU or L2, so as to pick the right SPM
device to configure.
>> +- qcom,saw2-cfg: SAW2 configuration register
>
>Why? Compatible should indicate where this register is.
>
There are multiple versions of saw handled by the same driver and
distinguished by the version register. These SAW revisions differ in the
register offset organization. The variable holds the value to be
configured in the saw2-cfg register. I will update the documentation to
be more clear.
>> +- qcom,saw2-spm-dly: Provides the values for the SPM delay command in the SPM
>> + sequence
>
>This is actually three values packed into one register for three
>different selectable delays, right? We don't typically do register jam
>tables in DT. Perhaps it should be split out into 3 different
>properties. Or maybe it shouldn't be specified in DT at all and should
>be determined algorithmically from the command sequences? From what I
>can tell most of the sequences don't even use these delays.
>
Not at all sequences use the delays. These cannot be determined
algorithmatically, They may be added to the sequence for changes in
hardware. Let me revisit the sequences to see if they need to be set
with the current sequence in use.
>> +- qcom,saw2-spm-ctl: The SPM control register
>
>Why? Compatible should indicate where this register is.
>
See above.
>> +
>> +Optional properties
>> +
>> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence
>> +- qcom,saw2-spm-cmd-ret: The Retention command sequence
>> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
>> +- qcom,saw2-spm-cmd-pc: The Power Collapse command sequence. This sequence may
>> + turn off other SoC components.
>> +- qcom,saw2-spm-cmd-gdhs: GDHS (Globally Distributed Head Switch) command
>> + sequence. This sequence will retain the memory but turn off the logic.
>
>I wonder if these should be properties of the idle states? That way the
>driver isn't searching for them by name in DT, instead it knows what
>state is associated with what sequence that the SPM needs to have
>programmed.
>
I see the relation you are seeing. But its not a property of the idle
state. Its an SoC specific property that the idle uses to indicate a
state. Better off lying here. I doubt there would be a good support for
holding SoC specific stuff in the ARM idle-states nodes.
>> +-
>> +Example:
>> + spm@f9089000 {
>> + compatible = "qcom,spm-v2.1";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>
>Why is this in the example? Are there subnodes?
>
No, there arent. I guess I carried over from downstream.
>> + reg = <0xf9089000 0x1000>;
>> + qcom,cpu = <&CPU0>;
>> + qcom,saw2-cfg = <0x1>;
>> + qcom,saw2-spm-dly= <0x20000400>;
>> + qcom,saw2-spm-ctl = <0x1>;
>> + qcom,saw2-spm-cmd-wfi = [03 0b 0f];
>> + qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
>> + a0 b0 03 68 70 3b 92 a0 b0
>> + 82 2b 50 10 30 02 22 30 0f];
>> + };
>> diff --git a/drivers/soc/qcom/spm-devices.c b/drivers/soc/qcom/spm-devices.c
>> new file mode 100644
>> index 0000000..2175a81
>> --- /dev/null
>> +++ b/drivers/soc/qcom/spm-devices.c
>> @@ -0,0 +1,198 @@
>> +/* Copyright (c) 2011-2014, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/delay.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/err.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include <soc/qcom/spm.h>
>> +
>> +#include "spm-drv.h"
>> +
>> +/**
>> + * All related information for an SPM device
>> + * Helps manage the collective.
>> + */
>> +struct msm_spm_device {
>> + bool initialized;
>> + struct msm_spm_driver_data drv;
>> +};
>> +
>> +static DEFINE_PER_CPU_SHARED_ALIGNED(struct msm_spm_device, msm_cpu_spm_device);
>> +
>> +/**
>> + * msm_spm_set_low_power_mode() - Configure SPM start address for low power mode
>> + * @mode: SPM LPM mode to enter
>> + */
>> +int msm_spm_set_low_power_mode(unsigned int mode)
>> +{
>> + struct msm_spm_device *dev = &__get_cpu_var(msm_cpu_spm_device);
>> + int ret = -EINVAL;
>> +
>> + if (!dev->initialized)
>> + return -ENXIO;
>> +
>> + if (mode == MSM_SPM_MODE_DISABLED)
>> + ret = msm_spm_drv_set_spm_enable(&dev->drv, false);
>> + else if (!msm_spm_drv_set_spm_enable(&dev->drv, true))
>> + ret = msm_spm_drv_set_low_power_mode(&dev->drv, mode);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(msm_spm_set_low_power_mode);
>> +
>> +static int get_cpu_id(struct device_node *node)
>> +{
>> + struct device_node *cpu_node;
>> + u32 cpu;
>> + int ret = -EINVAL;
>> + char *key = "qcom,cpu";
>> +
>> + cpu_node = of_parse_phandle(node, key, 0);
>> + if (cpu_node) {
>> + for_each_possible_cpu(cpu) {
>> + if (of_get_cpu_node(cpu, NULL) == cpu_node)
>> + return cpu;
>> + }
>> + }
>> + return ret;
>> +}
>> +
>> +static struct msm_spm_device *msm_spm_get_device(struct platform_device *pdev)
>> +{
>> + struct msm_spm_device *dev = NULL;
>> + int cpu = get_cpu_id(pdev->dev.of_node);
>> +
>> + if ((cpu >= 0) && cpu < num_possible_cpus())
>> + dev = &per_cpu(msm_cpu_spm_device, cpu);
>> +
>> + return dev;
>> +}
>> +
>> +static int msm_spm_dev_probe(struct platform_device *pdev)
>> +{
>> + int ret;
>> + int i;
>> + struct device_node *node = pdev->dev.of_node;
>> + char *key;
>> + uint32_t val;
>> + struct msm_spm_mode modes[MSM_SPM_MODE_NR];
>> + struct msm_spm_device *spm_dev;
>> + struct resource *res;
>> + uint32_t mode_count = 0;
>> +
>> + struct spm_of {
>> + char *key;
>> + uint32_t id;
>> + };
>> +
>> + /* SPM Configuration registers */
>> + struct spm_of spm_of_data[] = {
>> + {"qcom,saw2-clk-div", MSM_SPM_REG_SAW2_CFG},
>> + {"qcom,saw2-enable", MSM_SPM_REG_SAW2_SPM_CTL},
>> + {"qcom,saw2-delays", MSM_SPM_REG_SAW2_SPM_DLY},
>> + };
>> +
>> + /* SPM sleep sequences */
>> + struct spm_of mode_of_data[] = {
>> + {"qcom,saw2-spm-cmd-wfi", MSM_SPM_MODE_CLOCK_GATING},
>> + {"qcom,saw2-spm-cmd-spc", MSM_SPM_MODE_POWER_COLLAPSE},
>> + {"qcom,saw2-spm-cmd-ret", MSM_SPM_MODE_RETENTION},
>> + };
>> +
>> + /* Get the right SPM device */
>> + spm_dev = msm_spm_get_device(pdev);
>> + if (IS_ERR_OR_NULL(spm_dev))
>> + return -EINVAL;
>> +
>> + /* Get the SAW start address */
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> + spm_dev->drv.reg_base_addr = devm_ioremap(&pdev->dev, res->start,
>> + resource_size(res));
>> + if (!spm_dev->drv.reg_base_addr) {
>> + ret = -ENOMEM;
>> + goto fail;
>> + }
>> +
>> + /* Read the SPM configuration register values */
>> + for (i = 0; i < ARRAY_SIZE(spm_of_data); i++) {
>> + ret = of_property_read_u32(node, spm_of_data[i].key, &val);
>> + if (ret)
>> + continue;
>> + spm_dev->drv.reg_shadow[spm_of_data[i].id] = val;
>> + }
>> +
>> + /* Read the byte arrays for the SPM sleep sequences */
>> + for (i = 0; i < ARRAY_SIZE(mode_of_data); i++) {
>> + modes[mode_count].start_addr = 0;
>> + key = mode_of_data[i].key;
>> + modes[mode_count].cmd =
>> + (uint8_t *)of_get_property(node, key, &val);
>> + if (!modes[mode_count].cmd)
>> + continue;
>> + modes[mode_count].mode = mode_of_data[i].id;
>> + mode_count++;
>> + }
>> +
>> + spm_dev->drv.modes = devm_kcalloc(&pdev->dev, mode_count,
>> + sizeof(modes[0]), GFP_KERNEL);
>> + if (!spm_dev->drv.modes)
>> + return -ENOMEM;
>> + spm_dev->drv.num_modes = mode_count;
>> + memcpy(spm_dev->drv.modes, &modes[0], sizeof(modes[0]) * mode_count);
>> +
>> + /* Initialize the hardware */
>> + ret = msm_spm_drv_init(&spm_dev->drv);
>> + if (ret) {
>> + kfree(spm_dev->drv.modes);
>> + return ret;
>> + }
>> +
>> + spm_dev->initialized = true;
>> + return ret;
>> +
>> +fail:
>> + dev_err(&pdev->dev, "SPM device probe failed: %d\n", ret);
>> + return ret;
>> +}
>> +
>> +static struct of_device_id msm_spm_match_table[] = {
>
>const.
>
Will change.
>
>--
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>hosted by The Linux Foundation
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC
2014-08-26 0:31 ` Lina Iyer
@ 2014-08-26 2:17 ` Stephen Boyd
[not found] ` <53FBEE2B.7020008-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
0 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2014-08-26 2:17 UTC (permalink / raw)
To: Lina Iyer
Cc: daniel.lezcano, khilman, davidb, galak, linux-arm-msm,
lorenzo.pieralisi, msivasub, Praveen Chidamabram, Murali Nalajala,
linux-arm-kernel@lists.infradead.org, Linux Kernel Mailing List,
devicetree@vger.kernel.org
On 08/25/14 17:31, Lina Iyer wrote:
> On Mon, Aug 25, 2014 at 04:40:33PM -0700, Stephen Boyd wrote:
>> On 08/19/14 15:15, Lina Iyer wrote:
>>> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt
>>> b/Documentation/devicetree/bindings/arm/msm/spm.txt
>>> new file mode 100644
>>> index 0000000..318e024
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
>>
>> We already have a binding document for SAW. Please add stuff there
>> because SPM is just a component of SAW.
>>
> I agree that SPM is just a component of the SAW. But the document there
> seems to indicate regulator details, totally unrelated to the actual SAW
> hardware functionality.
Huh? The SAW binding should be extended with whatever properties are
necessary. Probably the only thing we need is the delays. Everything
else can be determined from the compatible?
SAW has a connection to the PMIC, does it not? If it isn't directly
connected we can come up with a different name for the node, but just
because the node name in the example is misleading doesn't mean we
should completely disregard what we already have.
>
>>> @@ -0,0 +1,47 @@
>>> +* Subsystem Power Manager (SAW2)
>>> +
>>> +S4 generation of MSMs have SPM hardware blocks to control the
>>> Application
>>> +Processor Sub-System power. These SPM blocks run individual state
>>> machine
>>> +to determine what the core (L2 or Krait/Scorpion) would do when the
>>> WFI
>>> +instruction is executed by the core.
>>> +
>>> +The devicetree representation of the SPM block should be:
>>> +
>>> +Required properties
>>> +
>>> +- compatible: Could be one of -
>>> + "qcom,spm-v2.1"
>>> + "qcom,spm-v3.0"
>>> +- reg: The physical address and the size of the SPM's memory mapped
>>> registers
>>> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
>>> + This field is required on only for SPMs that control the CPU.
>>
>> We have a phandle from the CPU/L2 to the SAW, so this isn't necessary.
>>
> Sorry, I dont understand. Care to explain pls? Its necessary to know
> what SPM instance controls which CPU or L2, so as to pick the right SPM
> device to configure.
We have a phandle in the CPU nodes pointing to the SAW that is
associated with that CPU (qcom,saw). SPM is a part of a SAW. Thus there
is no need for this qcom,cpu property once the SAW node is used.
Instead, we can search the CPU and cache nodes for a qcom,saw that
matches the of_node for the platform device we're probing.
>
>>> +- qcom,saw2-cfg: SAW2 configuration register
>>
>> Why? Compatible should indicate where this register is.
>>
> There are multiple versions of saw handled by the same driver and
> distinguished by the version register. These SAW revisions differ in the
> register offset organization. The variable holds the value to be
> configured in the saw2-cfg register. I will update the documentation to
> be more clear.
>>> +- qcom,saw2-spm-ctl: The SPM control register
>>
>> Why? Compatible should indicate where this register is.
>>
> See above.
Ah this is more register jam table in DT? cfg should probably be
described in desired clock rate and then the driver can figure out the
value by multiplying that the input clock rate. spm-ctl looks like it's
usually used to describe "enable", which seems rather obvious. Why isn't
the driver always writing the enable bit (bit 0)?
>
>>> +- qcom,saw2-spm-dly: Provides the values for the SPM delay command
>>> in the SPM
>>> + sequence
>>
>> This is actually three values packed into one register for three
>> different selectable delays, right? We don't typically do register jam
>> tables in DT. Perhaps it should be split out into 3 different
>> properties. Or maybe it shouldn't be specified in DT at all and should
>> be determined algorithmically from the command sequences? From what I
>> can tell most of the sequences don't even use these delays.
>>
> Not at all sequences use the delays. These cannot be determined
> algorithmatically, They may be added to the sequence for changes in
> hardware. Let me revisit the sequences to see if they need to be set
> with the current sequence in use.
>
I was thinking perhaps these should be more structured binary blobs that
indicate the delays that would be necessary in the first 3 bytes or
something and then the command sequence after that.
<delay1> <delay2> <delay3> <sequence>
Or perhaps
<num delays=N> <delayN-1> <delayN> <sequence>
and then the code would parse these first few bytes and compress them
into 3 values that are written into the register.
BTW, I wonder if these sequences should be firmware blobs? Or at least,
different files that we then /incbin/ into the final DT blob (if DT
reviewers approve putting blobs like this into the kernel, Cc'ed the DT
list just in case).
>
>
>>> +
>>> +Optional properties
>>> +
>>> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence
>>> +- qcom,saw2-spm-cmd-ret: The Retention command sequence
>>> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
>>> +- qcom,saw2-spm-cmd-pc: The Power Collapse command sequence. This
>>> sequence may
>>> + turn off other SoC components.
>>> +- qcom,saw2-spm-cmd-gdhs: GDHS (Globally Distributed Head Switch)
>>> command
>>> + sequence. This sequence will retain the memory but turn off the
>>> logic.
>>
>> I wonder if these should be properties of the idle states? That way the
>> driver isn't searching for them by name in DT, instead it knows what
>> state is associated with what sequence that the SPM needs to have
>> programmed.
>>
> I see the relation you are seeing. But its not a property of the idle
> state. Its an SoC specific property that the idle uses to indicate a
> state. Better off lying here. I doubt there would be a good support for
> holding SoC specific stuff in the ARM idle-states nodes.
>
>
What isn't specifically related to the 1) idle state and 2) CPU/L2/etc.
that the idle state is used in? I would think that by pointing to
different idle states from different CPU nodes we could cover all cases.
What is the SoC specific stuff in here?
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC
2014-08-19 22:15 ` [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC Lina Iyer
2014-08-25 23:40 ` Stephen Boyd
@ 2014-08-27 14:00 ` Kumar Gala
2014-08-27 15:35 ` Lina Iyer
1 sibling, 1 reply; 26+ messages in thread
From: Kumar Gala @ 2014-08-27 14:00 UTC (permalink / raw)
To: Lina Iyer
Cc: daniel.lezcano, khilman, sboyd, davidb, linux-arm-msm,
lorenzo.pieralisi, msivasub, Praveen Chidamabram, Murali Nalajala
On Aug 19, 2014, at 5:15 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
> Each cpu or an L2$ has an SPM device. They are identical instances of
> the same SPM block. This allows for multiple instances be grouped and
> managed collectively. spm-devices.c is the SPM device manager managing
> multiple SPM devices on top of the driver layer.
>
> Device configuration of each SPM is picked up from the DTS. The hardware
> configuration of each of the SPM is handled by the spm.c driver.
>
> Signed-off-by: Praveen Chidamabram <pchidamb@codeaurora.org>
> Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
> Documentation/devicetree/bindings/arm/msm/spm.txt | 47 +++++
> drivers/soc/qcom/Kconfig | 8 +
> drivers/soc/qcom/Makefile | 2 +-
> drivers/soc/qcom/spm-devices.c | 198 ++++++++++++++++++++++
> include/soc/qcom/spm.h | 34 ++++
> 5 files changed, 288 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
> create mode 100644 drivers/soc/qcom/spm-devices.c
> create mode 100644 include/soc/qcom/spm.h
>
> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
> new file mode 100644
> index 0000000..318e024
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
> @@ -0,0 +1,47 @@
> +* Subsystem Power Manager (SAW2)
> +
> +S4 generation of MSMs have SPM hardware blocks to control the Application
> +Processor Sub-System power. These SPM blocks run individual state machine
> +to determine what the core (L2 or Krait/Scorpion) would do when the WFI
> +instruction is executed by the core.
> +
> +The devicetree representation of the SPM block should be:
> +
> +Required properties
> +
> +- compatible: Could be one of -
> + "qcom,spm-v2.1"
> + "qcom,spm-v3.0"
> +- reg: The physical address and the size of the SPM's memory mapped registers
> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
> + This field is required on only for SPMs that control the CPU.
This should probably moved to optional since not ALL spm nodes would have it.
> +- qcom,saw2-cfg: SAW2 configuration register
> +- qcom,saw2-spm-dly: Provides the values for the SPM delay command in the SPM
> + sequence
> +- qcom,saw2-spm-ctl: The SPM control register
Where is the code that uses "qcom,saw2-spm-dly” & "qcom,saw2-spm-ctl"
> +
> +Optional properties
> +
> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence
> +- qcom,saw2-spm-cmd-ret: The Retention command sequence
> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
> +- qcom,saw2-spm-cmd-pc: The Power Collapse command sequence. This sequence may
> + turn off other SoC components.
> +- qcom,saw2-spm-cmd-gdhs: GDHS (Globally Distributed Head Switch) command
> + sequence. This sequence will retain the memory but turn off the logic.
> +-
> +Example:
> + spm@f9089000 {
> + compatible = "qcom,spm-v2.1";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0xf9089000 0x1000>;
> + qcom,cpu = <&CPU0>;
> + qcom,saw2-cfg = <0x1>;
> + qcom,saw2-spm-dly= <0x20000400>;
> + qcom,saw2-spm-ctl = <0x1>;
> + qcom,saw2-spm-cmd-wfi = [03 0b 0f];
> + qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
> + a0 b0 03 68 70 3b 92 a0 b0
> + 82 2b 50 10 30 02 22 30 0f];
> + };
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index 7dcd554..d467767 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -11,3 +11,11 @@ config QCOM_GSBI
>
> config QCOM_SCM
> bool
> +
> +config QCOM_PM
> + bool "Qualcomm Power Management"
> + depends on PM && ARCH_QCOM && OF
ARCH_QCOM implies OF
> + 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.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC
2014-08-27 14:00 ` Kumar Gala
@ 2014-08-27 15:35 ` Lina Iyer
0 siblings, 0 replies; 26+ messages in thread
From: Lina Iyer @ 2014-08-27 15:35 UTC (permalink / raw)
To: Kumar Gala
Cc: daniel.lezcano, khilman, sboyd, davidb, linux-arm-msm,
lorenzo.pieralisi, msivasub, Praveen Chidamabram, Murali Nalajala
On Wed, Aug 27, 2014 at 09:00:40AM -0500, Kumar Gala wrote:
>
>On Aug 19, 2014, at 5:15 PM, Lina Iyer <lina.iyer@linaro.org> wrote:
>
>> Each cpu or an L2$ has an SPM device. They are identical instances of
>> the same SPM block. This allows for multiple instances be grouped and
>> managed collectively. spm-devices.c is the SPM device manager managing
>> multiple SPM devices on top of the driver layer.
>>
>> Device configuration of each SPM is picked up from the DTS. The hardware
>> configuration of each of the SPM is handled by the spm.c driver.
>>
>> Signed-off-by: Praveen Chidamabram <pchidamb@codeaurora.org>
>> Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
>> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>> ---
>> Documentation/devicetree/bindings/arm/msm/spm.txt | 47 +++++
>> drivers/soc/qcom/Kconfig | 8 +
>> drivers/soc/qcom/Makefile | 2 +-
>> drivers/soc/qcom/spm-devices.c | 198 ++++++++++++++++++++++
>> include/soc/qcom/spm.h | 34 ++++
>> 5 files changed, 288 insertions(+), 1 deletion(-)
>> create mode 100644 Documentation/devicetree/bindings/arm/msm/spm.txt
>> create mode 100644 drivers/soc/qcom/spm-devices.c
>> create mode 100644 include/soc/qcom/spm.h
>>
>> diff --git a/Documentation/devicetree/bindings/arm/msm/spm.txt b/Documentation/devicetree/bindings/arm/msm/spm.txt
>> new file mode 100644
>> index 0000000..318e024
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/msm/spm.txt
>> @@ -0,0 +1,47 @@
>> +* Subsystem Power Manager (SAW2)
>> +
>> +S4 generation of MSMs have SPM hardware blocks to control the Application
>> +Processor Sub-System power. These SPM blocks run individual state machine
>> +to determine what the core (L2 or Krait/Scorpion) would do when the WFI
>> +instruction is executed by the core.
>> +
>> +The devicetree representation of the SPM block should be:
>> +
>> +Required properties
>> +
>> +- compatible: Could be one of -
>> + "qcom,spm-v2.1"
>> + "qcom,spm-v3.0"
>> +- reg: The physical address and the size of the SPM's memory mapped registers
>> +- qcom,cpu: phandle for the CPU that the SPM block is attached to.
>> + This field is required on only for SPMs that control the CPU.
>
>This should probably moved to optional since not ALL spm nodes would have it.
>
OK.
>> +- qcom,saw2-cfg: SAW2 configuration register
>> +- qcom,saw2-spm-dly: Provides the values for the SPM delay command in the SPM
>> + sequence
>> +- qcom,saw2-spm-ctl: The SPM control register
>
>Where is the code that uses "qcom,saw2-spm-dly” & "qcom,saw2-spm-ctl"
>
Sorry. Updated the code, but not the documentation in this version. I
noticed well after I sent the patch out. Will be fixed in the next
revision.
>> +
>> +Optional properties
>> +
>> +- qcom,saw2-spm-cmd-wfi: The WFI command sequence
>> +- qcom,saw2-spm-cmd-ret: The Retention command sequence
>> +- qcom,saw2-spm-cmd-spc: The Standalone PC command sequence
>> +- qcom,saw2-spm-cmd-pc: The Power Collapse command sequence. This sequence may
>> + turn off other SoC components.
>> +- qcom,saw2-spm-cmd-gdhs: GDHS (Globally Distributed Head Switch) command
>> + sequence. This sequence will retain the memory but turn off the logic.
>> +-
>> +Example:
>> + spm@f9089000 {
>> + compatible = "qcom,spm-v2.1";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + reg = <0xf9089000 0x1000>;
>> + qcom,cpu = <&CPU0>;
>> + qcom,saw2-cfg = <0x1>;
>> + qcom,saw2-spm-dly= <0x20000400>;
>> + qcom,saw2-spm-ctl = <0x1>;
>> + qcom,saw2-spm-cmd-wfi = [03 0b 0f];
>> + qcom,saw2-spm-cmd-spc = [00 20 50 80 60 70 10 92
>> + a0 b0 03 68 70 3b 92 a0 b0
>> + 82 2b 50 10 30 02 22 30 0f];
>> + };
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index 7dcd554..d467767 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -11,3 +11,11 @@ config QCOM_GSBI
>>
>> config QCOM_SCM
>> bool
>> +
>> +config QCOM_PM
>> + bool "Qualcomm Power Management"
>> + depends on PM && ARCH_QCOM && OF
>
>ARCH_QCOM implies OF
>
OK.
>> + 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.
>>
>
>
>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>--
>Employee of Qualcomm Innovation Center, Inc.
>Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 5/8] arm: dts: qcom: Add SPM device bindings for 8974
2014-08-19 22:15 [PATCH v4 0/8] QCOM 8074 cpuidle driver Lina Iyer
` (3 preceding siblings ...)
2014-08-19 22:15 ` [PATCH v4 4/8] qcom: spm-devices: Add SPM device manager for the SoC Lina Iyer
@ 2014-08-19 22:15 ` Lina Iyer
2014-08-19 22:15 ` [PATCH v4 6/8] qcom: msm-pm: Add cpu low power mode functions Lina Iyer
` (2 subsequent siblings)
7 siblings, 0 replies; 26+ messages in thread
From: Lina Iyer @ 2014-08-19 22:15 UTC (permalink / raw)
To: daniel.lezcano, khilman, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi
Cc: msivasub, Lina Iyer, Praveen Chidambaram
Add SPM device bindings for QCOM 8974 based cpus. SPM is the sub-system
power manager and controls the logic around the cores (cpu and L2).
Each core has an instance of SPM and controls only that core. Each cpu
SPM is configured to support WFI and SPC (standalone-power collapse) and
L2 can do retention (clock-gating).
Signed-off-by: Praveen Chidambaram <pchidamb@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
arch/arm/boot/dts/qcom-msm8974-pm.dtsi | 69 ++++++++++++++++++++++++++++++++++
arch/arm/boot/dts/qcom-msm8974.dtsi | 10 +++--
2 files changed, 75 insertions(+), 4 deletions(-)
create mode 100644 arch/arm/boot/dts/qcom-msm8974-pm.dtsi
diff --git a/arch/arm/boot/dts/qcom-msm8974-pm.dtsi b/arch/arm/boot/dts/qcom-msm8974-pm.dtsi
new file mode 100644
index 0000000..bbfb1d5
--- /dev/null
+++ b/arch/arm/boot/dts/qcom-msm8974-pm.dtsi
@@ -0,0 +1,69 @@
+/* Copyright (c) 2013-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.
+ */
+
+&soc {
+ spm@f9089000 {
+ compatible = "qcom,spm-v2.1";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0xf9089000 0x1000>;
+ qcom,cpu = <&CPU0>;
+ qcom,saw2-clk-div = <0x01>;
+ qcom,saw2-delays = <0x3C102800>;
+ qcom,saw2-enable = <0x01>;
+ qcom,saw2-spm-cmd-wfi = [03 0b 0f];
+ qcom,saw2-spm-cmd-spc = [00 20 80 10 E8 5B 03 3B E8 5B 82 10 0B
+ 30 06 26 30 0F];
+ };
+
+ spm@f9099000 {
+ compatible = "qcom,spm-v2.1";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0xf9099000 0x1000>;
+ qcom,cpu = <&CPU1>;
+ qcom,saw2-clk-div = <0x01>;
+ qcom,saw2-delays = <0x3C102800>;
+ qcom,saw2-enable = <0x01>;
+ qcom,saw2-spm-cmd-wfi = [03 0b 0f];
+ qcom,saw2-spm-cmd-spc = [00 20 80 10 E8 5B 03 3B E8 5B 82 10 0B
+ 30 06 26 30 0F];
+ };
+
+ spm@f90a9000 {
+ compatible = "qcom,spm-v2.1";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0xf90a9000 0x1000>;
+ qcom,cpu = <&CPU2>;
+ qcom,saw2-clk-div = <0x01>;
+ qcom,saw2-delays = <0x3C102800>;
+ qcom,saw2-enable = <0x01>;
+ qcom,saw2-spm-cmd-wfi = [03 0b 0f];
+ qcom,saw2-spm-cmd-spc = [00 20 80 10 E8 5B 03 3B E8 5B 82 10 0B
+ 30 06 26 30 0F];
+ };
+
+ spm@f90b9000 {
+ compatible = "qcom,spm-v2.1";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0xf90b9000 0x1000>;
+ qcom,cpu = <&CPU3>;
+ qcom,saw2-clk-div = <0x01>;
+ qcom,saw2-delays = <0x3C102800>;
+ qcom,saw2-enable = <0x01>;
+ qcom,saw2-spm-cmd-wfi = [03 0b 0f];
+ qcom,saw2-spm-cmd-spc = [00 20 80 10 E8 5B 03 3B E8 5B 82 10 0B
+ 30 06 26 30 0F];
+ };
+};
diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index 69dca2a..0580bc2 100644
--- a/arch/arm/boot/dts/qcom-msm8974.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8974.dtsi
@@ -14,7 +14,7 @@
#size-cells = <0>;
interrupts = <1 9 0xf04>;
- cpu@0 {
+ CPU0: cpu@0 {
compatible = "qcom,krait";
enable-method = "qcom,kpss-acc-v2";
device_type = "cpu";
@@ -23,7 +23,7 @@
qcom,acc = <&acc0>;
};
- cpu@1 {
+ CPU1: cpu@1 {
compatible = "qcom,krait";
enable-method = "qcom,kpss-acc-v2";
device_type = "cpu";
@@ -32,7 +32,7 @@
qcom,acc = <&acc1>;
};
- cpu@2 {
+ CPU2: cpu@2 {
compatible = "qcom,krait";
enable-method = "qcom,kpss-acc-v2";
device_type = "cpu";
@@ -41,7 +41,7 @@
qcom,acc = <&acc2>;
};
- cpu@3 {
+ CPU3: cpu@3 {
compatible = "qcom,krait";
enable-method = "qcom,kpss-acc-v2";
device_type = "cpu";
@@ -238,3 +238,5 @@
};
};
};
+
+#include "qcom-msm8974-pm.dtsi"
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 6/8] qcom: msm-pm: Add cpu low power mode functions
2014-08-19 22:15 [PATCH v4 0/8] QCOM 8074 cpuidle driver Lina Iyer
` (4 preceding siblings ...)
2014-08-19 22:15 ` [PATCH v4 5/8] arm: dts: qcom: Add SPM device bindings for 8974 Lina Iyer
@ 2014-08-19 22:15 ` Lina Iyer
2014-08-19 22:15 ` [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus Lina Iyer
2014-08-19 22:15 ` [PATCH v4 8/8] arm: dts: qcom: Add idle states device nodes for 8974 Lina Iyer
7 siblings, 0 replies; 26+ messages in thread
From: Lina Iyer @ 2014-08-19 22:15 UTC (permalink / raw)
To: daniel.lezcano, khilman, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi
Cc: msivasub, Lina Iyer, Venkat Devarasetty
Add interface layer to abstract and handle hardware specific
functionality for executing various cpu low power modes in QCOM
chipsets.
Signed-off-by: Venkat Devarasetty <vdevaras@codeaurora.org>
Signed-off-by: Mahesh Sivasubramanian <msivasub@codeaurora.org>
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
drivers/soc/qcom/Makefile | 2 +-
drivers/soc/qcom/msm-pm.c | 117 ++++++++++++++++++++++++++++++++++++++++++++++
include/soc/qcom/pm.h | 38 +++++++++++++++
3 files changed, 156 insertions(+), 1 deletion(-)
create mode 100644 drivers/soc/qcom/msm-pm.c
create mode 100644 include/soc/qcom/pm.h
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index 9457b2a..acdd6fa 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 spm-devices.o
+obj-$(CONFIG_QCOM_PM) += spm.o spm-devices.o msm-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/msm-pm.c b/drivers/soc/qcom/msm-pm.c
new file mode 100644
index 0000000..5a984e3
--- /dev/null
+++ b/drivers/soc/qcom/msm-pm.c
@@ -0,0 +1,117 @@
+/* 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/module.h>
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/smp.h>
+#include <linux/platform_device.h>
+#include <linux/cpu_pm.h>
+#include <linux/uaccess.h>
+
+#include <soc/qcom/spm.h>
+#include <soc/qcom/pm.h>
+#include <soc/qcom/scm.h>
+#include <soc/qcom/scm-boot.h>
+
+#include <asm/suspend.h>
+#include <asm/cacheflush.h>
+#include <asm/cputype.h>
+#include <asm/system_misc.h>
+
+#define SCM_CMD_TERMINATE_PC (0x2)
+#define SCM_FLUSH_FLAG_MASK (0x3)
+
+static int msm_pm_collapse(unsigned long unused)
+{
+ enum msm_pm_l2_scm_flag flag;
+
+ flag = MSM_SCM_L2_ON & SCM_FLUSH_FLAG_MASK;
+ scm_call_atomic1(SCM_SVC_BOOT, SCM_CMD_TERMINATE_PC, flag);
+
+ return 0;
+}
+
+static void set_up_boot_address(void *entry, int cpu)
+{
+ static int flags[NR_CPUS] = {
+ SCM_FLAG_WARMBOOT_CPU0,
+ SCM_FLAG_WARMBOOT_CPU1,
+ SCM_FLAG_WARMBOOT_CPU2,
+ SCM_FLAG_WARMBOOT_CPU3,
+ };
+ static DEFINE_PER_CPU(void *, last_known_entry);
+
+ if (entry == per_cpu(last_known_entry, cpu))
+ return;
+
+ per_cpu(last_known_entry, cpu) = entry;
+ scm_set_boot_addr(virt_to_phys(entry), flags[cpu]);
+}
+
+static int do_power_down(void)
+{
+ int ret;
+
+ cpu_pm_enter();
+
+ msm_spm_set_low_power_mode(MSM_SPM_MODE_POWER_COLLAPSE);
+ set_up_boot_address(cpu_resume, raw_smp_processor_id());
+ ret = cpu_suspend(0, msm_pm_collapse);
+
+ cpu_pm_exit();
+
+ return ret;
+}
+
+static inline int do_idle(void)
+{
+ msm_spm_set_low_power_mode(MSM_SPM_MODE_CLOCK_GATING);
+
+ /* Flush and clock-gate */
+ mb();
+ wfi();
+
+ return 0;
+}
+
+/**
+ * msm_cpu_pm_enter_sleep(): Enter a low power mode on current cpu
+ *
+ * @mode - sleep mode to enter
+ * @from_idle - bool to indicate that the mode is exercised during idle/suspend
+ *
+ * The code should be with interrupts disabled and on the core on which the
+ * low power is to be executed.
+ *
+ */
+int msm_cpu_pm_enter_sleep(enum msm_pm_sleep_mode mode, bool from_idle)
+{
+ int ret;
+
+ switch (mode) {
+ case MSM_PM_SLEEP_MODE_POWER_COLLAPSE_STANDALONE:
+ ret = do_power_down();
+ break;
+ default:
+ case MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT:
+ ret = do_idle();
+ break;
+ }
+
+ local_irq_enable();
+
+ return ret;
+}
+EXPORT_SYMBOL(msm_cpu_pm_enter_sleep);
diff --git a/include/soc/qcom/pm.h b/include/soc/qcom/pm.h
new file mode 100644
index 0000000..7563e09
--- /dev/null
+++ b/include/soc/qcom/pm.h
@@ -0,0 +1,38 @@
+/*
+ * 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 msm_pm_sleep_mode {
+ MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT,
+ MSM_PM_SLEEP_MODE_POWER_COLLAPSE_STANDALONE,
+ MSM_PM_SLEEP_MODE_NR,
+};
+
+
+enum msm_pm_l2_scm_flag {
+ MSM_SCM_L2_ON = 0,
+ MSM_SCM_L2_OFF = 1
+};
+
+#ifdef CONFIG_QCOM_PM
+int msm_cpu_pm_enter_sleep(enum msm_pm_sleep_mode mode, bool from_idle);
+#else
+static inline int msm_cpu_pm_enter_sleep(enum msm_pm_sleep_mode mode,
+ bool from_idle)
+{ return -ENODEV; }
+#endif
+
+#endif /* __QCOM_PM_H */
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus
2014-08-19 22:15 [PATCH v4 0/8] QCOM 8074 cpuidle driver Lina Iyer
` (5 preceding siblings ...)
2014-08-19 22:15 ` [PATCH v4 6/8] qcom: msm-pm: Add cpu low power mode functions Lina Iyer
@ 2014-08-19 22:15 ` Lina Iyer
2014-08-21 1:24 ` Daniel Lezcano
2014-08-19 22:15 ` [PATCH v4 8/8] arm: dts: qcom: Add idle states device nodes for 8974 Lina Iyer
7 siblings, 1 reply; 26+ messages in thread
From: Lina Iyer @ 2014-08-19 22:15 UTC (permalink / raw)
To: daniel.lezcano, khilman, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi
Cc: msivasub, Lina Iyer
Add cpuidle driver interface to allow cpus to go into C-States.
Use the cpuidle DT interfacecommon across ARM architectures to provide
the C-State information to the cpuidle framework.
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
drivers/cpuidle/Kconfig.arm | 7 +++
drivers/cpuidle/Makefile | 1 +
drivers/cpuidle/cpuidle-qcom.c | 119 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 127 insertions(+)
create mode 100644 drivers/cpuidle/cpuidle-qcom.c
diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
index e339c7f..26e31bd 100644
--- a/drivers/cpuidle/Kconfig.arm
+++ b/drivers/cpuidle/Kconfig.arm
@@ -63,3 +63,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..4aae672
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-qcom.c
@@ -0,0 +1,119 @@
+/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
+ * 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/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/cpuidle.h>
+#include <linux/cpumask.h>
+#include <linux/slab.h>
+#include <linux/of_device.h>
+
+#include <soc/qcom/pm.h>
+#include "dt_idle_states.h"
+
+static enum msm_pm_sleep_mode modes[CPUIDLE_STATE_MAX];
+
+static int qcom_lpm_enter(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv, int index)
+{
+ return msm_cpu_pm_enter_sleep(modes[index], true);
+}
+
+static struct cpuidle_driver qcom_cpuidle_driver = {
+ .name = "qcom_cpuidle",
+ .owner = THIS_MODULE,
+};
+
+static void parse_state_translations(struct cpuidle_driver *drv)
+{
+ struct device_node *state_node, *cpu_node;
+ const char *mode_name;
+ int i, j;
+
+ struct name_map {
+ enum msm_pm_sleep_mode mode;
+ char *name;
+ };
+ static struct name_map c_states[] = {
+ { MSM_PM_SLEEP_MODE_POWER_COLLAPSE_STANDALONE,
+ "standalone-pc" },
+ { MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT, "wfi" },
+ };
+
+ cpu_node = of_cpu_device_node_get(cpumask_first(drv->cpumask));
+ if (!cpu_node)
+ return;
+
+ /**
+ * Get the state description from idle-state node entry-method
+ * First state is always WFI, per spec.
+ */
+ modes[0] = MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT;
+ for (i = 1; i < drv->state_count; i++) {
+ mode_name = NULL;
+ state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
+ of_property_read_string(state_node, "entry-method",
+ &mode_name);
+ for (j = 0; mode_name && (j < ARRAY_SIZE(c_states)); j++) {
+ if (!strcmp(mode_name, c_states[j].name)) {
+ modes[i] = c_states[j].mode;
+ break;
+ }
+ }
+ }
+}
+
+static int qcom_cpuidle_init(void)
+{
+ struct cpuidle_driver *drv = &qcom_cpuidle_driver;
+ int ret;
+ int i;
+
+ drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
+ if (!drv->cpumask)
+ return -ENOMEM;
+ cpumask_copy(drv->cpumask, cpu_possible_mask);
+
+ /**
+ * Default to standard for WFI (index = 0)
+ * Probe only for other states
+ */
+ ret = dt_init_idle_driver(drv, 1);
+ if (ret < 0) {
+ pr_err("%s: failed to initialize idle states\n", __func__);
+ goto failed;
+ }
+
+ /* Parse the idle states for C-States on this cpu */
+ parse_state_translations(drv);
+
+ /* Register entry point for all low states */
+ for (i = 0; i < drv->state_count; i++)
+ drv->states[i].enter = qcom_lpm_enter;
+ drv->safe_state_index = 0;
+
+ ret = cpuidle_register(drv, NULL);
+ if (ret) {
+ pr_err("%s: failed to register cpuidle driver\n", __func__);
+ goto failed;
+ }
+
+ return 0;
+
+failed:
+ kfree(drv->cpumask);
+ return ret;
+}
+module_init(qcom_cpuidle_init);
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus
2014-08-19 22:15 ` [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus Lina Iyer
@ 2014-08-21 1:24 ` Daniel Lezcano
2014-08-21 14:36 ` Lina Iyer
2014-08-22 15:36 ` Lina Iyer
0 siblings, 2 replies; 26+ messages in thread
From: Daniel Lezcano @ 2014-08-21 1:24 UTC (permalink / raw)
To: Lina Iyer, khilman, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi
Cc: msivasub
On 08/20/2014 12:15 AM, Lina Iyer wrote:
> Add cpuidle driver interface to allow cpus to go into C-States.
> Use the cpuidle DT interfacecommon across ARM architectures to provide
^^^^^^^^
> the C-State information to the cpuidle framework.
>
> Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> ---
> drivers/cpuidle/Kconfig.arm | 7 +++
> drivers/cpuidle/Makefile | 1 +
> drivers/cpuidle/cpuidle-qcom.c | 119 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 127 insertions(+)
> create mode 100644 drivers/cpuidle/cpuidle-qcom.c
>
> diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> index e339c7f..26e31bd 100644
> --- a/drivers/cpuidle/Kconfig.arm
> +++ b/drivers/cpuidle/Kconfig.arm
> @@ -63,3 +63,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..4aae672
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-qcom.c
> @@ -0,0 +1,119 @@
> +/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
> + * 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/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/cpuidle.h>
> +#include <linux/cpumask.h>
> +#include <linux/slab.h>
See below, cpumask is not needed, you can remove slab.h and cpumask.h.
> +#include <linux/of_device.h>
> +
> +#include <soc/qcom/pm.h>
> +#include "dt_idle_states.h"
> +
> +static enum msm_pm_sleep_mode modes[CPUIDLE_STATE_MAX];
> +
> +static int qcom_lpm_enter(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv, int index)
> +{
> + return msm_cpu_pm_enter_sleep(modes[index], true);
You should return the index here.
> +}
> +
> +static struct cpuidle_driver qcom_cpuidle_driver = {
> + .name = "qcom_cpuidle",
> + .owner = THIS_MODULE,
> +};
> +
> +static void parse_state_translations(struct cpuidle_driver *drv)
> +{
> + struct device_node *state_node, *cpu_node;
> + const char *mode_name;
> + int i, j;
> +
> + struct name_map {
> + enum msm_pm_sleep_mode mode;
> + char *name;
> + };
> + static struct name_map c_states[] = {
> + { MSM_PM_SLEEP_MODE_POWER_COLLAPSE_STANDALONE,
> + "standalone-pc" },
What is standalone-pc and collapse ? Core power down ?
> + { MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT, "wfi" },
> + };
> +
> + cpu_node = of_cpu_device_node_get(cpumask_first(drv->cpumask));
> + if (!cpu_node)
> + return;
> +
> + /**
> + * Get the state description from idle-state node entry-method
> + * First state is always WFI, per spec.
> + */
> + modes[0] = MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT;
> + for (i = 1; i < drv->state_count; i++) {
> + mode_name = NULL;
> + state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> + of_property_read_string(state_node, "entry-method",
> + &mode_name);
> + for (j = 0; mode_name && (j < ARRAY_SIZE(c_states)); j++) {
> + if (!strcmp(mode_name, c_states[j].name)) {
> + modes[i] = c_states[j].mode;
> + break;
> + }
> + }
> + }
> +}
For the function above, I believe we can do better with the idle_dt_init
function to prevent to have to reparse the infos.
I had the opportunity to discuss with Lorenzo privately and we found a
solution he will submit to solve that.
> +static int qcom_cpuidle_init(void)
> +{
> + struct cpuidle_driver *drv = &qcom_cpuidle_driver;
> + int ret;
> + int i;
> +
> + drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
> + if (!drv->cpumask)
> + return -ENOMEM;
> + cpumask_copy(drv->cpumask, cpu_possible_mask);
You can get rid of this because the driver does not rely on the multiple
driver support. If the drv->cpumask is NULL it will be automatically set
to cpu_possible_mask by the cpuidle framework.
> +
> + /**
> + * Default to standard for WFI (index = 0)
> + * Probe only for other states
> + */
> + ret = dt_init_idle_driver(drv, 1);
So IIUC, if you specify the index 1, that means the state[0] will be the
default WFI. But you override the callback below in the loop.
I recommend you use the default arm wfi callback but you implement the
cpu_do_idle for your platform.
> + if (ret < 0) {
> + pr_err("%s: failed to initialize idle states\n", __func__);
> + goto failed;
> + }
> +
> + /* Parse the idle states for C-States on this cpu */
> + parse_state_translations(drv);
> +
> + /* Register entry point for all low states */
> + for (i = 0; i < drv->state_count; i++)
> + drv->states[i].enter = qcom_lpm_enter;
> + drv->safe_state_index = 0;
The safe_state index is only used for the couple c-states and as the
driver is statically defined, it is pointless to initialize this variable.
> +
> + ret = cpuidle_register(drv, NULL);
> + if (ret) {
> + pr_err("%s: failed to register cpuidle driver\n", __func__);
> + goto failed;
> + }
> +
> + return 0;
> +
> +failed:
> + kfree(drv->cpumask);
> + return ret;
> +}
> +module_init(qcom_cpuidle_init);
Can you stick to the platform_driver paradigm like the other drivers ?
Thanks
-- Daniel
--
<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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus
2014-08-21 1:24 ` Daniel Lezcano
@ 2014-08-21 14:36 ` Lina Iyer
2014-08-21 15:07 ` Lorenzo Pieralisi
2014-08-27 17:31 ` Kevin Hilman
2014-08-22 15:36 ` Lina Iyer
1 sibling, 2 replies; 26+ messages in thread
From: Lina Iyer @ 2014-08-21 14:36 UTC (permalink / raw)
To: Daniel Lezcano
Cc: khilman, sboyd, davidb, galak, linux-arm-msm, lorenzo.pieralisi,
msivasub
On Thu, Aug 21, 2014 at 03:24:44AM +0200, Daniel Lezcano wrote:
>On 08/20/2014 12:15 AM, Lina Iyer wrote:
>>Add cpuidle driver interface to allow cpus to go into C-States.
>>Use the cpuidle DT interfacecommon across ARM architectures to provide
>
> ^^^^^^^^
>
Thanks, will fix.
>>the C-State information to the cpuidle framework.
>>
>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>---
>> drivers/cpuidle/Kconfig.arm | 7 +++
>> drivers/cpuidle/Makefile | 1 +
>> drivers/cpuidle/cpuidle-qcom.c | 119 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 127 insertions(+)
>> create mode 100644 drivers/cpuidle/cpuidle-qcom.c
>>
>>diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
>>index e339c7f..26e31bd 100644
>>--- a/drivers/cpuidle/Kconfig.arm
>>+++ b/drivers/cpuidle/Kconfig.arm
>>@@ -63,3 +63,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..4aae672
>>--- /dev/null
>>+++ b/drivers/cpuidle/cpuidle-qcom.c
>>@@ -0,0 +1,119 @@
>>+/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>+ * 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/module.h>
>>+#include <linux/platform_device.h>
>>+#include <linux/of.h>
>>+#include <linux/cpuidle.h>
>>+#include <linux/cpumask.h>
>>+#include <linux/slab.h>
>
>See below, cpumask is not needed, you can remove slab.h and cpumask.h.
>
>>+#include <linux/of_device.h>
>>+
>>+#include <soc/qcom/pm.h>
>>+#include "dt_idle_states.h"
>>+
>>+static enum msm_pm_sleep_mode modes[CPUIDLE_STATE_MAX];
>>+
>>+static int qcom_lpm_enter(struct cpuidle_device *dev,
>>+ struct cpuidle_driver *drv, int index)
>>+{
>>+ return msm_cpu_pm_enter_sleep(modes[index], true);
>
>You should return the index here.
>
Argh. yes.
>>+}
>>+
>>+static struct cpuidle_driver qcom_cpuidle_driver = {
>>+ .name = "qcom_cpuidle",
>>+ .owner = THIS_MODULE,
>>+};
>>+
>>+static void parse_state_translations(struct cpuidle_driver *drv)
>>+{
>>+ struct device_node *state_node, *cpu_node;
>>+ const char *mode_name;
>>+ int i, j;
>>+
>>+ struct name_map {
>>+ enum msm_pm_sleep_mode mode;
>>+ char *name;
>>+ };
>>+ static struct name_map c_states[] = {
>>+ { MSM_PM_SLEEP_MODE_POWER_COLLAPSE_STANDALONE,
>>+ "standalone-pc" },
>
>What is standalone-pc and collapse ? Core power down ?
>
I had explained it in the cover letter. But let me add some comments
here or the commit text
>>+ { MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT, "wfi" },
>>+ };
>>+
>>+ cpu_node = of_cpu_device_node_get(cpumask_first(drv->cpumask));
>>+ if (!cpu_node)
>>+ return;
>>+
>>+ /**
>>+ * Get the state description from idle-state node entry-method
>>+ * First state is always WFI, per spec.
>>+ */
>>+ modes[0] = MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT;
>>+ for (i = 1; i < drv->state_count; i++) {
>>+ mode_name = NULL;
>>+ state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
>>+ of_property_read_string(state_node, "entry-method",
>>+ &mode_name);
>>+ for (j = 0; mode_name && (j < ARRAY_SIZE(c_states)); j++) {
>>+ if (!strcmp(mode_name, c_states[j].name)) {
>>+ modes[i] = c_states[j].mode;
>>+ break;
>>+ }
>>+ }
>>+ }
>>+}
>
>For the function above, I believe we can do better with the
>idle_dt_init function to prevent to have to reparse the infos.
>
>I had the opportunity to discuss with Lorenzo privately and we found a
>solution he will submit to solve that.
>
Hmm. Thanks, I was asked to use the entry-method. FWIW, I dont like this
either :)
>>+static int qcom_cpuidle_init(void)
>>+{
>>+ struct cpuidle_driver *drv = &qcom_cpuidle_driver;
>>+ int ret;
>>+ int i;
>>+
>>+ drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
>>+ if (!drv->cpumask)
>>+ return -ENOMEM;
>>+ cpumask_copy(drv->cpumask, cpu_possible_mask);
>
>You can get rid of this because the driver does not rely on the
>multiple driver support. If the drv->cpumask is NULL it will be
>automatically set to cpu_possible_mask by the cpuidle framework.
>
Ok.
>>+
>>+ /**
>>+ * Default to standard for WFI (index = 0)
>>+ * Probe only for other states
>>+ */
>>+ ret = dt_init_idle_driver(drv, 1);
>
>So IIUC, if you specify the index 1, that means the state[0] will be
>the default WFI. But you override the callback below in the loop.
>
>I recommend you use the default arm wfi callback but you implement the
>cpu_do_idle for your platform.
>
Yes, it was intended. I dont want to define two WFI states. The
architectural WFI does not buy us enough compared to WFI that SoC can
do. L2 can go into low power modes when the core is in WFI and for that
I would like to have all WFI's enter SoC framework.
>>+ if (ret < 0) {
>>+ pr_err("%s: failed to initialize idle states\n", __func__);
>>+ goto failed;
>>+ }
>>+
>>+ /* Parse the idle states for C-States on this cpu */
>>+ parse_state_translations(drv);
>>+
>>+ /* Register entry point for all low states */
>>+ for (i = 0; i < drv->state_count; i++)
>>+ drv->states[i].enter = qcom_lpm_enter;
>>+ drv->safe_state_index = 0;
>
>The safe_state index is only used for the couple c-states and as the
>driver is statically defined, it is pointless to initialize this
>variable.
>
Will remove.
>>+
>>+ ret = cpuidle_register(drv, NULL);
>>+ if (ret) {
>>+ pr_err("%s: failed to register cpuidle driver\n", __func__);
>>+ goto failed;
>>+ }
>>+
>>+ return 0;
>>+
>>+failed:
>>+ kfree(drv->cpumask);
>>+ return ret;
>>+}
>>+module_init(qcom_cpuidle_init);
>
>Can you stick to the platform_driver paradigm like the other drivers ?
>
I do not have any platform data for the driver to use at this point. I
would not prefer to initialize a platform device from board file and
there isnt a configuration available at this point for me to create a DT
node for this driver. Hence after some discussions, I decided to use the
module_init(). If its just a matter of preference, I would request you
to wait until the entire CPUIdle patch stream plays out. I have a
feeling that I might need to add device properties and would need a
platform driver, legitimately.
>Thanks
> -- Daniel
Thanks Daniel, for your time between the summits.
Lina.
>
>--
> <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
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus
2014-08-21 14:36 ` Lina Iyer
@ 2014-08-21 15:07 ` Lorenzo Pieralisi
2014-08-27 17:31 ` Kevin Hilman
1 sibling, 0 replies; 26+ messages in thread
From: Lorenzo Pieralisi @ 2014-08-21 15:07 UTC (permalink / raw)
To: Lina Iyer
Cc: Daniel Lezcano, khilman@linaro.org, sboyd@codeaurora.org,
davidb@codeaurora.org, galak@codeaurora.org,
linux-arm-msm@vger.kernel.org, msivasub@codeaurora.org
On Thu, Aug 21, 2014 at 03:36:43PM +0100, Lina Iyer wrote:
[...]
> >>+ cpu_node = of_cpu_device_node_get(cpumask_first(drv->cpumask));
> >>+ if (!cpu_node)
> >>+ return;
> >>+
> >>+ /**
> >>+ * Get the state description from idle-state node entry-method
> >>+ * First state is always WFI, per spec.
> >>+ */
> >>+ modes[0] = MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT;
> >>+ for (i = 1; i < drv->state_count; i++) {
> >>+ mode_name = NULL;
> >>+ state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> >>+ of_property_read_string(state_node, "entry-method",
> >>+ &mode_name);
> >>+ for (j = 0; mode_name && (j < ARRAY_SIZE(c_states)); j++) {
> >>+ if (!strcmp(mode_name, c_states[j].name)) {
> >>+ modes[i] = c_states[j].mode;
> >>+ break;
> >>+ }
> >>+ }
> >>+ }
> >>+}
> >
> >For the function above, I believe we can do better with the
> >idle_dt_init function to prevent to have to reparse the infos.
> >
> >I had the opportunity to discuss with Lorenzo privately and we found a
> >solution he will submit to solve that.
> >
> Hmm. Thanks, I was asked to use the entry-method. FWIW, I dont like this
> either :)
You weren't asked to use entry-method, you were asked to do what psci
does, namely defining a per-state parameter.
Anyway, we will rely on compatible string to initialize the state enter
pointer so that we are all happy again.
The driver will pass an array of pairs {compatible, enter_function},
the dt init will initialize the idle state enter pointer accordingly.
I still have a feeling that DT parsing code should also return an array to
the CPUidle driver corresponding to DT nodes that were used to
initialize idle states, this way, if it is needed, the CPUidle driver
can parse for each index the platform specific idle state properties.
Otherwise we will end up with a function pointer per-state even if two
states are almost identical and just differ in the parameter passed.
It is more a matter of taste than anything else, I will implement the
code discussed with Daniel that will be on the lists at -rc3.
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus
2014-08-21 14:36 ` Lina Iyer
2014-08-21 15:07 ` Lorenzo Pieralisi
@ 2014-08-27 17:31 ` Kevin Hilman
2014-08-27 20:35 ` Lina Iyer
1 sibling, 1 reply; 26+ messages in thread
From: Kevin Hilman @ 2014-08-27 17:31 UTC (permalink / raw)
To: Lina Iyer
Cc: Daniel Lezcano, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi, msivasub
Lina Iyer <lina.iyer@linaro.org> writes:
> On Thu, Aug 21, 2014 at 03:24:44AM +0200, Daniel Lezcano wrote:
>>On 08/20/2014 12:15 AM, Lina Iyer wrote:
[...]
>>
>> So IIUC, if you specify the index 1, that means the state[0] will be
>> the default WFI. But you override the callback below in the loop.
>>
>> I recommend you use the default arm wfi callback but you implement
>> the cpu_do_idle for your platform.
>
> Yes, it was intended. I dont want to define two WFI states. The
> architectural WFI does not buy us enough compared to WFI that SoC can
> do. L2 can go into low power modes when the core is in WFI and for that
> I would like to have all WFI's enter SoC framework.
If the L2 is going into low-power, that means there will be higher
latency coming out compared to the architectural WFI, correct?
If you have both, and the latency/residency numbers are accurate, the
governor is then left to pick the right one. So, what's wrong with
having both?
Kevin
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus
2014-08-27 17:31 ` Kevin Hilman
@ 2014-08-27 20:35 ` Lina Iyer
0 siblings, 0 replies; 26+ messages in thread
From: Lina Iyer @ 2014-08-27 20:35 UTC (permalink / raw)
To: Kevin Hilman
Cc: Daniel Lezcano, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi, msivasub
On Wed, Aug 27, 2014 at 10:31:26AM -0700, Kevin Hilman wrote:
>Lina Iyer <lina.iyer@linaro.org> writes:
>
>> On Thu, Aug 21, 2014 at 03:24:44AM +0200, Daniel Lezcano wrote:
>>>On 08/20/2014 12:15 AM, Lina Iyer wrote:
>
>[...]
>
>>>
>>> So IIUC, if you specify the index 1, that means the state[0] will be
>>> the default WFI. But you override the callback below in the loop.
>>>
>>> I recommend you use the default arm wfi callback but you implement
>>> the cpu_do_idle for your platform.
>>
>> Yes, it was intended. I dont want to define two WFI states. The
>> architectural WFI does not buy us enough compared to WFI that SoC can
>> do. L2 can go into low power modes when the core is in WFI and for that
>> I would like to have all WFI's enter SoC framework.
>
>If the L2 is going into low-power, that means there will be higher
>latency coming out compared to the architectural WFI, correct?
>
Thats right. Architecturally L2 deeper sleep modes (which include L2
logic off) may reset the core. So L2 deeper sleep modes with cores in
WFI is not a suggested possibility.
>If you have both, and the latency/residency numbers are accurate, the
>governor is then left to pick the right one. So, what's wrong with
>having both?
I agree, but there is very little benefit to doing architectural WFI vs
platform WFI in most cases. The residencies are too close and almost the
same, doesnt warrant a separate functions for that. There is no thing
wrong, just dint see a lot of benefit, thats all. What do you think?
>
>Kevin
>
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus
2014-08-21 1:24 ` Daniel Lezcano
2014-08-21 14:36 ` Lina Iyer
@ 2014-08-22 15:36 ` Lina Iyer
2014-08-23 10:37 ` Lorenzo Pieralisi
1 sibling, 1 reply; 26+ messages in thread
From: Lina Iyer @ 2014-08-22 15:36 UTC (permalink / raw)
To: Daniel Lezcano
Cc: khilman, sboyd, davidb, galak, linux-arm-msm, lorenzo.pieralisi,
msivasub
On Thu, Aug 21, 2014 at 03:24:44AM +0200, Daniel Lezcano wrote:
>On 08/20/2014 12:15 AM, Lina Iyer wrote:
>>Add cpuidle driver interface to allow cpus to go into C-States.
>>Use the cpuidle DT interfacecommon across ARM architectures to provide
>
> ^^^^^^^^
>
>>the C-State information to the cpuidle framework.
>>
>>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
>>---
>> drivers/cpuidle/Kconfig.arm | 7 +++
>> drivers/cpuidle/Makefile | 1 +
>> drivers/cpuidle/cpuidle-qcom.c | 119 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 127 insertions(+)
>> create mode 100644 drivers/cpuidle/cpuidle-qcom.c
>>
>>diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
>>index e339c7f..26e31bd 100644
>>--- a/drivers/cpuidle/Kconfig.arm
>>+++ b/drivers/cpuidle/Kconfig.arm
>>@@ -63,3 +63,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..4aae672
>>--- /dev/null
>>+++ b/drivers/cpuidle/cpuidle-qcom.c
>>@@ -0,0 +1,119 @@
>>+/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>+ * 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/module.h>
>>+#include <linux/platform_device.h>
>>+#include <linux/of.h>
>>+#include <linux/cpuidle.h>
>>+#include <linux/cpumask.h>
>>+#include <linux/slab.h>
>
>See below, cpumask is not needed, you can remove slab.h and cpumask.h.
>
>>+#include <linux/of_device.h>
>>+
>>+#include <soc/qcom/pm.h>
>>+#include "dt_idle_states.h"
>>+
>>+static enum msm_pm_sleep_mode modes[CPUIDLE_STATE_MAX];
>>+
>>+static int qcom_lpm_enter(struct cpuidle_device *dev,
>>+ struct cpuidle_driver *drv, int index)
>>+{
>>+ return msm_cpu_pm_enter_sleep(modes[index], true);
>
>You should return the index here.
>
>>+}
>>+
>>+static struct cpuidle_driver qcom_cpuidle_driver = {
>>+ .name = "qcom_cpuidle",
>>+ .owner = THIS_MODULE,
>>+};
>>+
>>+static void parse_state_translations(struct cpuidle_driver *drv)
>>+{
>>+ struct device_node *state_node, *cpu_node;
>>+ const char *mode_name;
>>+ int i, j;
>>+
>>+ struct name_map {
>>+ enum msm_pm_sleep_mode mode;
>>+ char *name;
>>+ };
>>+ static struct name_map c_states[] = {
>>+ { MSM_PM_SLEEP_MODE_POWER_COLLAPSE_STANDALONE,
>>+ "standalone-pc" },
>
>What is standalone-pc and collapse ? Core power down ?
>
>>+ { MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT, "wfi" },
>>+ };
>>+
>>+ cpu_node = of_cpu_device_node_get(cpumask_first(drv->cpumask));
>>+ if (!cpu_node)
>>+ return;
>>+
>>+ /**
>>+ * Get the state description from idle-state node entry-method
>>+ * First state is always WFI, per spec.
>>+ */
>>+ modes[0] = MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT;
>>+ for (i = 1; i < drv->state_count; i++) {
>>+ mode_name = NULL;
>>+ state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
>>+ of_property_read_string(state_node, "entry-method",
>>+ &mode_name);
>>+ for (j = 0; mode_name && (j < ARRAY_SIZE(c_states)); j++) {
>>+ if (!strcmp(mode_name, c_states[j].name)) {
>>+ modes[i] = c_states[j].mode;
>>+ break;
>>+ }
>>+ }
>>+ }
>>+}
>
>For the function above, I believe we can do better with the
>idle_dt_init function to prevent to have to reparse the infos.
>
>I had the opportunity to discuss with Lorenzo privately and we found a
>solution he will submit to solve that.
>
>>+static int qcom_cpuidle_init(void)
>>+{
>>+ struct cpuidle_driver *drv = &qcom_cpuidle_driver;
>>+ int ret;
>>+ int i;
>>+
>>+ drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
>>+ if (!drv->cpumask)
>>+ return -ENOMEM;
>>+ cpumask_copy(drv->cpumask, cpu_possible_mask);
>
>You can get rid of this because the driver does not rely on the
>multiple driver support. If the drv->cpumask is NULL it will be
>automatically set to cpu_possible_mask by the cpuidle framework.
>
Lorenzo: Seems like the dt_init_idle_driver expects to see this mask
set.
May be you can check if its NULL and revert to cpu_possible_mask in that
case. That would avoid drivers creating memory and copying
cpu_possible_mask in their init functions.
>>+
>>+ /**
>>+ * Default to standard for WFI (index = 0)
>>+ * Probe only for other states
>>+ */
>>+ ret = dt_init_idle_driver(drv, 1);
>
>So IIUC, if you specify the index 1, that means the state[0] will be
>the default WFI. But you override the callback below in the loop.
>
>I recommend you use the default arm wfi callback but you implement the
>cpu_do_idle for your platform.
>
>>+ if (ret < 0) {
>>+ pr_err("%s: failed to initialize idle states\n", __func__);
>>+ goto failed;
>>+ }
>>+
>>+ /* Parse the idle states for C-States on this cpu */
>>+ parse_state_translations(drv);
>>+
>>+ /* Register entry point for all low states */
>>+ for (i = 0; i < drv->state_count; i++)
>>+ drv->states[i].enter = qcom_lpm_enter;
>>+ drv->safe_state_index = 0;
>
>The safe_state index is only used for the couple c-states and as the
>driver is statically defined, it is pointless to initialize this
>variable.
>
>>+
>>+ ret = cpuidle_register(drv, NULL);
>>+ if (ret) {
>>+ pr_err("%s: failed to register cpuidle driver\n", __func__);
>>+ goto failed;
>>+ }
>>+
>>+ return 0;
>>+
>>+failed:
>>+ kfree(drv->cpumask);
>>+ return ret;
>>+}
>>+module_init(qcom_cpuidle_init);
>
>Can you stick to the platform_driver paradigm like the other drivers ?
>
>Thanks
> -- Daniel
>
>--
> <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
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus
2014-08-22 15:36 ` Lina Iyer
@ 2014-08-23 10:37 ` Lorenzo Pieralisi
0 siblings, 0 replies; 26+ messages in thread
From: Lorenzo Pieralisi @ 2014-08-23 10:37 UTC (permalink / raw)
To: Lina Iyer
Cc: Daniel Lezcano, khilman@linaro.org, sboyd@codeaurora.org,
davidb@codeaurora.org, galak@codeaurora.org,
linux-arm-msm@vger.kernel.org, msivasub@codeaurora.org
On Fri, Aug 22, 2014 at 04:36:20PM +0100, Lina Iyer wrote:
> On Thu, Aug 21, 2014 at 03:24:44AM +0200, Daniel Lezcano wrote:
> >On 08/20/2014 12:15 AM, Lina Iyer wrote:
> >>Add cpuidle driver interface to allow cpus to go into C-States.
> >>Use the cpuidle DT interfacecommon across ARM architectures to provide
> >
> > ^^^^^^^^
> >
> >>the C-State information to the cpuidle framework.
> >>
> >>Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
> >>---
> >> drivers/cpuidle/Kconfig.arm | 7 +++
> >> drivers/cpuidle/Makefile | 1 +
> >> drivers/cpuidle/cpuidle-qcom.c | 119 +++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 127 insertions(+)
> >> create mode 100644 drivers/cpuidle/cpuidle-qcom.c
> >>
> >>diff --git a/drivers/cpuidle/Kconfig.arm b/drivers/cpuidle/Kconfig.arm
> >>index e339c7f..26e31bd 100644
> >>--- a/drivers/cpuidle/Kconfig.arm
> >>+++ b/drivers/cpuidle/Kconfig.arm
> >>@@ -63,3 +63,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..4aae672
> >>--- /dev/null
> >>+++ b/drivers/cpuidle/cpuidle-qcom.c
> >>@@ -0,0 +1,119 @@
> >>+/* Copyright (c) 2014, The Linux Foundation. All rights reserved.
> >>+ * 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/module.h>
> >>+#include <linux/platform_device.h>
> >>+#include <linux/of.h>
> >>+#include <linux/cpuidle.h>
> >>+#include <linux/cpumask.h>
> >>+#include <linux/slab.h>
> >
> >See below, cpumask is not needed, you can remove slab.h and cpumask.h.
> >
> >>+#include <linux/of_device.h>
> >>+
> >>+#include <soc/qcom/pm.h>
> >>+#include "dt_idle_states.h"
> >>+
> >>+static enum msm_pm_sleep_mode modes[CPUIDLE_STATE_MAX];
> >>+
> >>+static int qcom_lpm_enter(struct cpuidle_device *dev,
> >>+ struct cpuidle_driver *drv, int index)
> >>+{
> >>+ return msm_cpu_pm_enter_sleep(modes[index], true);
> >
> >You should return the index here.
> >
> >>+}
> >>+
> >>+static struct cpuidle_driver qcom_cpuidle_driver = {
> >>+ .name = "qcom_cpuidle",
> >>+ .owner = THIS_MODULE,
> >>+};
> >>+
> >>+static void parse_state_translations(struct cpuidle_driver *drv)
> >>+{
> >>+ struct device_node *state_node, *cpu_node;
> >>+ const char *mode_name;
> >>+ int i, j;
> >>+
> >>+ struct name_map {
> >>+ enum msm_pm_sleep_mode mode;
> >>+ char *name;
> >>+ };
> >>+ static struct name_map c_states[] = {
> >>+ { MSM_PM_SLEEP_MODE_POWER_COLLAPSE_STANDALONE,
> >>+ "standalone-pc" },
> >
> >What is standalone-pc and collapse ? Core power down ?
> >
> >>+ { MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT, "wfi" },
> >>+ };
> >>+
> >>+ cpu_node = of_cpu_device_node_get(cpumask_first(drv->cpumask));
> >>+ if (!cpu_node)
> >>+ return;
> >>+
> >>+ /**
> >>+ * Get the state description from idle-state node entry-method
> >>+ * First state is always WFI, per spec.
> >>+ */
> >>+ modes[0] = MSM_PM_SLEEP_MODE_WAIT_FOR_INTERRUPT;
> >>+ for (i = 1; i < drv->state_count; i++) {
> >>+ mode_name = NULL;
> >>+ state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> >>+ of_property_read_string(state_node, "entry-method",
> >>+ &mode_name);
> >>+ for (j = 0; mode_name && (j < ARRAY_SIZE(c_states)); j++) {
> >>+ if (!strcmp(mode_name, c_states[j].name)) {
> >>+ modes[i] = c_states[j].mode;
> >>+ break;
> >>+ }
> >>+ }
> >>+ }
> >>+}
> >
> >For the function above, I believe we can do better with the
> >idle_dt_init function to prevent to have to reparse the infos.
> >
> >I had the opportunity to discuss with Lorenzo privately and we found a
> >solution he will submit to solve that.
> >
> >>+static int qcom_cpuidle_init(void)
> >>+{
> >>+ struct cpuidle_driver *drv = &qcom_cpuidle_driver;
> >>+ int ret;
> >>+ int i;
> >>+
> >>+ drv->cpumask = kzalloc(cpumask_size(), GFP_KERNEL);
> >>+ if (!drv->cpumask)
> >>+ return -ENOMEM;
> >>+ cpumask_copy(drv->cpumask, cpu_possible_mask);
> >
> >You can get rid of this because the driver does not rely on the
> >multiple driver support. If the drv->cpumask is NULL it will be
> >automatically set to cpu_possible_mask by the cpuidle framework.
> >
> Lorenzo: Seems like the dt_init_idle_driver expects to see this mask
> set.
> May be you can check if its NULL and revert to cpu_possible_mask in that
> case. That would avoid drivers creating memory and copying
> cpu_possible_mask in their init functions.
I will do, thanks.
Lorenzo
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v4 8/8] arm: dts: qcom: Add idle states device nodes for 8974
2014-08-19 22:15 [PATCH v4 0/8] QCOM 8074 cpuidle driver Lina Iyer
` (6 preceding siblings ...)
2014-08-19 22:15 ` [PATCH v4 7/8] qcom: cpuidle: Add cpuidle driver for QCOM cpus Lina Iyer
@ 2014-08-19 22:15 ` Lina Iyer
7 siblings, 0 replies; 26+ messages in thread
From: Lina Iyer @ 2014-08-19 22:15 UTC (permalink / raw)
To: daniel.lezcano, khilman, sboyd, davidb, galak, linux-arm-msm,
lorenzo.pieralisi
Cc: msivasub, Lina Iyer
Add allowable C-States for each cpu using the cpu-idle-states node.
ARM spec dictates WFI as the default idle state at 0. Support standalone
power collapse (power down that does not affect any SoC idle states) for
each cpu.
Signed-off-by: Lina Iyer <lina.iyer@linaro.org>
---
arch/arm/boot/dts/qcom-msm8974.dtsi | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-msm8974.dtsi b/arch/arm/boot/dts/qcom-msm8974.dtsi
index 0580bc2..fd66afb 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>;
+ cpu-idle-states = <&CPU_SPC>;
};
CPU1: cpu@1 {
@@ -30,6 +31,7 @@
reg = <1>;
next-level-cache = <&L2>;
qcom,acc = <&acc1>;
+ cpu-idle-states = <&CPU_SPC>;
};
CPU2: cpu@2 {
@@ -39,6 +41,7 @@
reg = <2>;
next-level-cache = <&L2>;
qcom,acc = <&acc2>;
+ cpu-idle-states = <&CPU_SPC>;
};
CPU3: cpu@3 {
@@ -48,6 +51,7 @@
reg = <3>;
next-level-cache = <&L2>;
qcom,acc = <&acc3>;
+ cpu-idle-states = <&CPU_SPC>;
};
L2: l2-cache {
@@ -55,6 +59,16 @@
cache-level = <2>;
qcom,saw = <&saw_l2>;
};
+
+ idle-states {
+ CPU_SPC: cpu-sleep-0 {
+ compatible = "arm,idle-state";
+ entry-latency-us = <150>;
+ exit-latency-us = <200>;
+ min-residency-us = <2000>;
+ entry-method = "standalone-pc";
+ };
+ };
};
cpu-pmu {
--
1.9.1
^ permalink raw reply related [flat|nested] 26+ messages in thread