* [PATCH 0/5] Qualcomm Suspend to Idle Support
@ 2016-05-19 5:00 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-pm; +Cc: linux-arm-msm, linux-arm-kernel, stanimir.varbanov, Andy Gross
This set of patches adds suspend to idle support for the Qualcomm APQ8084
and MSM8916 platforms. The patches add a minimal set of suspend ops and
init functions to properly configure the enter_freeze function.
This patch set is based on the work done in the following:
[RFC v3 00/12] PM: SoC idle support using PM domains
http://comments.gmane.org/gmane.linux.ports.arm.msm/17966
The relevant prerequisites are the PSCI enablement DT patches. PSCI based
firmware is also required for the MSM8916 to work properly.
Andy Gross (5):
soc: qcom: Add suspend to idle support
arm: defconfig: Add Qualcomm Suspend options
arm64: dts: msm8916: Add spc compat tag
ARM: dts: qcom: Remove size elements from pmic reg
ARM: dts: qcom: pma8084: Add pwrkey entry
arch/arm/boot/dts/qcom-pma8084.dtsi | 20 ++++++---
arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
arch/arm64/configs/defconfig | 2 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/suspend.c | 77 +++++++++++++++++++++++++++++++++++
5 files changed, 95 insertions(+), 7 deletions(-)
create mode 100644 drivers/soc/qcom/suspend.c
--
1.9.1
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 0/5] Qualcomm Suspend to Idle Support
@ 2016-05-19 5:00 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-arm-kernel
This set of patches adds suspend to idle support for the Qualcomm APQ8084
and MSM8916 platforms. The patches add a minimal set of suspend ops and
init functions to properly configure the enter_freeze function.
This patch set is based on the work done in the following:
[RFC v3 00/12] PM: SoC idle support using PM domains
http://comments.gmane.org/gmane.linux.ports.arm.msm/17966
The relevant prerequisites are the PSCI enablement DT patches. PSCI based
firmware is also required for the MSM8916 to work properly.
Andy Gross (5):
soc: qcom: Add suspend to idle support
arm: defconfig: Add Qualcomm Suspend options
arm64: dts: msm8916: Add spc compat tag
ARM: dts: qcom: Remove size elements from pmic reg
ARM: dts: qcom: pma8084: Add pwrkey entry
arch/arm/boot/dts/qcom-pma8084.dtsi | 20 ++++++---
arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
arch/arm64/configs/defconfig | 2 +
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/suspend.c | 77 +++++++++++++++++++++++++++++++++++
5 files changed, 95 insertions(+), 7 deletions(-)
create mode 100644 drivers/soc/qcom/suspend.c
--
1.9.1
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 1/5] soc: qcom: Add suspend to idle support
2016-05-19 5:00 ` Andy Gross
@ 2016-05-19 5:00 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-pm; +Cc: linux-arm-msm, linux-arm-kernel, stanimir.varbanov, Andy Gross
This patch adds suspend to idle support for Qualcomm processors. While
suspend to memory will be a valid state, there won't be any special
handling or power savings over the suspend to idle.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/suspend.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+)
create mode 100644 drivers/soc/qcom/suspend.c
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index fdd664e..7c479d3 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
obj-$(CONFIG_QCOM_SMSM) += smsm.o
obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_SUSPEND) += suspend.o
diff --git a/drivers/soc/qcom/suspend.c b/drivers/soc/qcom/suspend.c
new file mode 100644
index 0000000..7d3f2dd
--- /dev/null
+++ b/drivers/soc/qcom/suspend.c
@@ -0,0 +1,77 @@
+/*
+ * (C) Copyright 2016 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/cpuidle.h>
+#include <linux/suspend.h>
+
+
+static void qcom_pm_enter_freeze(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ drv->states[index].enter(dev, drv, index);
+}
+
+static const struct of_device_id qcom_idle_state_match[] = {
+ { .compatible = "qcom,idle-state-spc", },
+ { },
+};
+
+static const struct platform_suspend_ops qcom_suspend_ops = {
+ .valid = suspend_valid_only_mem,
+};
+
+static int __init qcom_pm_init(void)
+{
+ struct cpuidle_device *cpu_dev;
+ struct cpuidle_driver *cpu_drv;
+ int state_count;
+ struct device_node *state_np, *cpu_np;
+ const struct of_device_id *match;
+ int i;
+
+ /* configure CPU enter_freeze if applicable */
+ for_each_present_cpu(i) {
+ cpu_np = of_get_cpu_node(i, NULL);
+ cpu_dev = per_cpu_ptr(cpuidle_devices, i);
+ cpu_drv = cpuidle_get_cpu_driver(cpu_dev);
+
+ if (!cpu_dev || !cpu_drv) {
+ of_node_put(cpu_np);
+ return -EPROBE_DEFER;
+ }
+
+ state_count = 0;
+ state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
+ state_count);
+
+ while (state_np) {
+ match = of_match_node(qcom_idle_state_match,
+ state_np);
+
+ state_count++;
+ if (match)
+ cpu_drv->states[state_count].enter_freeze =
+ &qcom_pm_enter_freeze;
+ of_node_put(state_np);
+
+ state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
+ state_count);
+ }
+
+ of_node_put(cpu_np);
+ }
+
+ suspend_set_ops(&qcom_suspend_ops);
+
+ return 0;
+}
+
+late_initcall(qcom_pm_init);
--
1.9.1
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH 1/5] soc: qcom: Add suspend to idle support
@ 2016-05-19 5:00 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds suspend to idle support for Qualcomm processors. While
suspend to memory will be a valid state, there won't be any special
handling or power savings over the suspend to idle.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
drivers/soc/qcom/Makefile | 1 +
drivers/soc/qcom/suspend.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+)
create mode 100644 drivers/soc/qcom/suspend.c
diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
index fdd664e..7c479d3 100644
--- a/drivers/soc/qcom/Makefile
+++ b/drivers/soc/qcom/Makefile
@@ -7,3 +7,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
obj-$(CONFIG_QCOM_SMSM) += smsm.o
obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
+obj-$(CONFIG_SUSPEND) += suspend.o
diff --git a/drivers/soc/qcom/suspend.c b/drivers/soc/qcom/suspend.c
new file mode 100644
index 0000000..7d3f2dd
--- /dev/null
+++ b/drivers/soc/qcom/suspend.c
@@ -0,0 +1,77 @@
+/*
+ * (C) Copyright 2016 Linaro Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/cpuidle.h>
+#include <linux/suspend.h>
+
+
+static void qcom_pm_enter_freeze(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ drv->states[index].enter(dev, drv, index);
+}
+
+static const struct of_device_id qcom_idle_state_match[] = {
+ { .compatible = "qcom,idle-state-spc", },
+ { },
+};
+
+static const struct platform_suspend_ops qcom_suspend_ops = {
+ .valid = suspend_valid_only_mem,
+};
+
+static int __init qcom_pm_init(void)
+{
+ struct cpuidle_device *cpu_dev;
+ struct cpuidle_driver *cpu_drv;
+ int state_count;
+ struct device_node *state_np, *cpu_np;
+ const struct of_device_id *match;
+ int i;
+
+ /* configure CPU enter_freeze if applicable */
+ for_each_present_cpu(i) {
+ cpu_np = of_get_cpu_node(i, NULL);
+ cpu_dev = per_cpu_ptr(cpuidle_devices, i);
+ cpu_drv = cpuidle_get_cpu_driver(cpu_dev);
+
+ if (!cpu_dev || !cpu_drv) {
+ of_node_put(cpu_np);
+ return -EPROBE_DEFER;
+ }
+
+ state_count = 0;
+ state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
+ state_count);
+
+ while (state_np) {
+ match = of_match_node(qcom_idle_state_match,
+ state_np);
+
+ state_count++;
+ if (match)
+ cpu_drv->states[state_count].enter_freeze =
+ &qcom_pm_enter_freeze;
+ of_node_put(state_np);
+
+ state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
+ state_count);
+ }
+
+ of_node_put(cpu_np);
+ }
+
+ suspend_set_ops(&qcom_suspend_ops);
+
+ return 0;
+}
+
+late_initcall(qcom_pm_init);
--
1.9.1
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH 2/5] arm: defconfig: Enable PM8941 pwr key
2016-05-19 5:00 ` Andy Gross
@ 2016-05-19 5:00 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-pm; +Cc: linux-arm-msm, linux-arm-kernel, stanimir.varbanov, Andy Gross
This patch enables the PM8941 pwr key driver.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
arch/arm64/configs/defconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 8917150..d4c539f 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -142,6 +142,8 @@ CONFIG_WL18XX=m
CONFIG_WLCORE_SDIO=m
CONFIG_INPUT_EVDEV=y
CONFIG_KEYBOARD_GPIO=y
+CONFIG_INPUT_MISC=y
+CONFIG_INPUT_PM8941_PWRKEY=y
# CONFIG_SERIO_SERPORT is not set
CONFIG_SERIO_AMBAKMI=y
CONFIG_LEGACY_PTY_COUNT=16
--
1.9.1
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH 2/5] arm: defconfig: Enable PM8941 pwr key
@ 2016-05-19 5:00 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-arm-kernel
This patch enables the PM8941 pwr key driver.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
arch/arm64/configs/defconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 8917150..d4c539f 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -142,6 +142,8 @@ CONFIG_WL18XX=m
CONFIG_WLCORE_SDIO=m
CONFIG_INPUT_EVDEV=y
CONFIG_KEYBOARD_GPIO=y
+CONFIG_INPUT_MISC=y
+CONFIG_INPUT_PM8941_PWRKEY=y
# CONFIG_SERIO_SERPORT is not set
CONFIG_SERIO_AMBAKMI=y
CONFIG_LEGACY_PTY_COUNT=16
--
1.9.1
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-05-19 5:00 ` Andy Gross
@ 2016-05-19 5:00 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-pm; +Cc: linux-arm-msm, linux-arm-kernel, stanimir.varbanov, Andy Gross
This patch adds the qcom,idle-state-spc compatible to the SPC idle
state. This compatible indicates that the state is one which supports
freeze.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 208af00..032e411 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -104,7 +104,7 @@
idle-states {
CPU_SPC: spc {
- compatible = "arm,idle-state";
+ compatible = "qcom,idle-state-spc", "arm,idle-state";
arm,psci-suspend-param = <0x40000002>;
entry-latency-us = <130>;
exit-latency-us = <150>;
--
1.9.1
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-05-19 5:00 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds the qcom,idle-state-spc compatible to the SPC idle
state. This compatible indicates that the state is one which supports
freeze.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
index 208af00..032e411 100644
--- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
@@ -104,7 +104,7 @@
idle-states {
CPU_SPC: spc {
- compatible = "arm,idle-state";
+ compatible = "qcom,idle-state-spc", "arm,idle-state";
arm,psci-suspend-param = <0x40000002>;
entry-latency-us = <130>;
exit-latency-us = <150>;
--
1.9.1
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH 4/5] ARM: dts: qcom: Remove size elements from pmic reg
2016-05-19 5:00 ` Andy Gross
@ 2016-05-19 5:00 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-pm; +Cc: linux-arm-msm, linux-arm-kernel, stanimir.varbanov, Andy Gross
The #size-cells for the pmics are 0, but we specify a size in the
reg property so that MPP and GPIO modules can figure out how many
pins there are. Now that we've done that by counting irqs, we can
remove the size elements in the reg properties and be DT
compliant.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
arch/arm/boot/dts/qcom-pma8084.dtsi | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arm/boot/dts/qcom-pma8084.dtsi b/arch/arm/boot/dts/qcom-pma8084.dtsi
index 4e9bd3f..ea7fce3 100644
--- a/arch/arm/boot/dts/qcom-pma8084.dtsi
+++ b/arch/arm/boot/dts/qcom-pma8084.dtsi
@@ -12,15 +12,15 @@
rtc@6000 {
compatible = "qcom,pm8941-rtc";
- reg = <0x6000 0x100>,
- <0x6100 0x100>;
+ reg = <0x6000>,
+ <0x6100>;
reg-names = "rtc", "alarm";
interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
};
pma8084_gpios: gpios@c000 {
compatible = "qcom,pma8084-gpio", "qcom,spmi-gpio";
- reg = <0xc000 0x1600>;
+ reg = <0xc000>;
gpio-controller;
#gpio-cells = <2>;
interrupts = <0 0xc0 0 IRQ_TYPE_NONE>,
@@ -49,7 +49,7 @@
pma8084_mpps: mpps@a000 {
compatible = "qcom,pma8084-mpp", "qcom,spmi-mpp";
- reg = <0xa000 0x800>;
+ reg = <0xa000>;
gpio-controller;
#gpio-cells = <2>;
interrupts = <0 0xa0 0 IRQ_TYPE_NONE>,
@@ -64,7 +64,7 @@
pma8084_temp: temp-alarm@2400 {
compatible = "qcom,spmi-temp-alarm";
- reg = <0x2400 0x100>;
+ reg = <0x2400>;
interrupts = <0 0x24 0 IRQ_TYPE_EDGE_RISING>;
#thermal-sensor-cells = <0>;
io-channels = <&pma8084_vadc VADC_DIE_TEMP>;
@@ -73,7 +73,7 @@
pma8084_vadc: vadc@3100 {
compatible = "qcom,spmi-vadc";
- reg = <0x3100 0x100>;
+ reg = <0x3100>;
interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
#address-cells = <1>;
#size-cells = <0>;
--
1.9.1
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH 4/5] ARM: dts: qcom: Remove size elements from pmic reg
@ 2016-05-19 5:00 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-arm-kernel
The #size-cells for the pmics are 0, but we specify a size in the
reg property so that MPP and GPIO modules can figure out how many
pins there are. Now that we've done that by counting irqs, we can
remove the size elements in the reg properties and be DT
compliant.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
arch/arm/boot/dts/qcom-pma8084.dtsi | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/arm/boot/dts/qcom-pma8084.dtsi b/arch/arm/boot/dts/qcom-pma8084.dtsi
index 4e9bd3f..ea7fce3 100644
--- a/arch/arm/boot/dts/qcom-pma8084.dtsi
+++ b/arch/arm/boot/dts/qcom-pma8084.dtsi
@@ -12,15 +12,15 @@
rtc at 6000 {
compatible = "qcom,pm8941-rtc";
- reg = <0x6000 0x100>,
- <0x6100 0x100>;
+ reg = <0x6000>,
+ <0x6100>;
reg-names = "rtc", "alarm";
interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
};
pma8084_gpios: gpios at c000 {
compatible = "qcom,pma8084-gpio", "qcom,spmi-gpio";
- reg = <0xc000 0x1600>;
+ reg = <0xc000>;
gpio-controller;
#gpio-cells = <2>;
interrupts = <0 0xc0 0 IRQ_TYPE_NONE>,
@@ -49,7 +49,7 @@
pma8084_mpps: mpps at a000 {
compatible = "qcom,pma8084-mpp", "qcom,spmi-mpp";
- reg = <0xa000 0x800>;
+ reg = <0xa000>;
gpio-controller;
#gpio-cells = <2>;
interrupts = <0 0xa0 0 IRQ_TYPE_NONE>,
@@ -64,7 +64,7 @@
pma8084_temp: temp-alarm at 2400 {
compatible = "qcom,spmi-temp-alarm";
- reg = <0x2400 0x100>;
+ reg = <0x2400>;
interrupts = <0 0x24 0 IRQ_TYPE_EDGE_RISING>;
#thermal-sensor-cells = <0>;
io-channels = <&pma8084_vadc VADC_DIE_TEMP>;
@@ -73,7 +73,7 @@
pma8084_vadc: vadc at 3100 {
compatible = "qcom,spmi-vadc";
- reg = <0x3100 0x100>;
+ reg = <0x3100>;
interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
#address-cells = <1>;
#size-cells = <0>;
--
1.9.1
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH 5/5] ARM: dts: qcom: pma8084: Add pwrkey entry
2016-05-19 5:00 ` Andy Gross
@ 2016-05-19 5:00 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-pm; +Cc: linux-arm-msm, linux-arm-kernel, stanimir.varbanov, Andy Gross
This patch adds the power key device tree node.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
arch/arm/boot/dts/qcom-pma8084.dtsi | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-pma8084.dtsi b/arch/arm/boot/dts/qcom-pma8084.dtsi
index ea7fce3..82d2580 100644
--- a/arch/arm/boot/dts/qcom-pma8084.dtsi
+++ b/arch/arm/boot/dts/qcom-pma8084.dtsi
@@ -18,6 +18,14 @@
interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
};
+ pwrkey@800 {
+ compatible = "qcom,pm8941-pwrkey";
+ reg = <0x800>;
+ interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
+ debounce = <15625>;
+ bias-pull-up;
+ };
+
pma8084_gpios: gpios@c000 {
compatible = "qcom,pma8084-gpio", "qcom,spmi-gpio";
reg = <0xc000>;
--
1.9.1
^ permalink raw reply related [flat|nested] 62+ messages in thread
* [PATCH 5/5] ARM: dts: qcom: pma8084: Add pwrkey entry
@ 2016-05-19 5:00 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 5:00 UTC (permalink / raw)
To: linux-arm-kernel
This patch adds the power key device tree node.
Signed-off-by: Andy Gross <andy.gross@linaro.org>
---
arch/arm/boot/dts/qcom-pma8084.dtsi | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm/boot/dts/qcom-pma8084.dtsi b/arch/arm/boot/dts/qcom-pma8084.dtsi
index ea7fce3..82d2580 100644
--- a/arch/arm/boot/dts/qcom-pma8084.dtsi
+++ b/arch/arm/boot/dts/qcom-pma8084.dtsi
@@ -18,6 +18,14 @@
interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
};
+ pwrkey at 800 {
+ compatible = "qcom,pm8941-pwrkey";
+ reg = <0x800>;
+ interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
+ debounce = <15625>;
+ bias-pull-up;
+ };
+
pma8084_gpios: gpios at c000 {
compatible = "qcom,pma8084-gpio", "qcom,spmi-gpio";
reg = <0xc000>;
--
1.9.1
^ permalink raw reply related [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-05-19 5:00 ` Andy Gross
@ 2016-05-19 19:52 ` Stanimir Varbanov
-1 siblings, 0 replies; 62+ messages in thread
From: Stanimir Varbanov @ 2016-05-19 19:52 UTC (permalink / raw)
To: Andy Gross, linux-pm; +Cc: linux-arm-msm, linux-arm-kernel, stanimir.varbanov
Hi Andy,
On 05/19/2016 08:00 AM, Andy Gross wrote:
> This patch adds the qcom,idle-state-spc compatible to the SPC idle
> state. This compatible indicates that the state is one which supports
> freeze.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 208af00..032e411 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -104,7 +104,7 @@
>
> idle-states {
Is this change depends on some another patchset, I cannot find
idle-states dt node on mainline nor linux-next ?
> CPU_SPC: spc {
> - compatible = "arm,idle-state";
> + compatible = "qcom,idle-state-spc", "arm,idle-state";
> arm,psci-suspend-param = <0x40000002>;
> entry-latency-us = <130>;
> exit-latency-us = <150>;
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-05-19 19:52 ` Stanimir Varbanov
0 siblings, 0 replies; 62+ messages in thread
From: Stanimir Varbanov @ 2016-05-19 19:52 UTC (permalink / raw)
To: linux-arm-kernel
Hi Andy,
On 05/19/2016 08:00 AM, Andy Gross wrote:
> This patch adds the qcom,idle-state-spc compatible to the SPC idle
> state. This compatible indicates that the state is one which supports
> freeze.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 208af00..032e411 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -104,7 +104,7 @@
>
> idle-states {
Is this change depends on some another patchset, I cannot find
idle-states dt node on mainline nor linux-next ?
> CPU_SPC: spc {
> - compatible = "arm,idle-state";
> + compatible = "qcom,idle-state-spc", "arm,idle-state";
> arm,psci-suspend-param = <0x40000002>;
> entry-latency-us = <130>;
> exit-latency-us = <150>;
>
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-05-19 19:52 ` Stanimir Varbanov
@ 2016-05-19 20:16 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 20:16 UTC (permalink / raw)
To: Stanimir Varbanov; +Cc: linux-pm, linux-arm-msm, linux-arm-kernel
On 19 May 2016 at 14:52, Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote:
> Hi Andy,
>
> On 05/19/2016 08:00 AM, Andy Gross wrote:
>> This patch adds the qcom,idle-state-spc compatible to the SPC idle
>> state. This compatible indicates that the state is one which supports
>> freeze.
>>
>> Signed-off-by: Andy Gross <andy.gross@linaro.org>
>> ---
>> arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
>> index 208af00..032e411 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
>> @@ -104,7 +104,7 @@
>>
>> idle-states {
>
> Is this change depends on some another patchset, I cannot find
> idle-states dt node on mainline nor linux-next ?
The cover letter gives a link to the patchset that contains the basis
for the changes. You can also check out my posted branch which merges
in a part of those changes.
The link to the specific patch is:
http://www.spinics.net/lists/linux-arm-msm/msg19675.html
or you can look at this:
https://git.linaro.org/people/andy.gross/linux-pm.git/shortlog/refs/heads/for-pramod
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-05-19 20:16 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-05-19 20:16 UTC (permalink / raw)
To: linux-arm-kernel
On 19 May 2016 at 14:52, Stanimir Varbanov <stanimir.varbanov@linaro.org> wrote:
> Hi Andy,
>
> On 05/19/2016 08:00 AM, Andy Gross wrote:
>> This patch adds the qcom,idle-state-spc compatible to the SPC idle
>> state. This compatible indicates that the state is one which supports
>> freeze.
>>
>> Signed-off-by: Andy Gross <andy.gross@linaro.org>
>> ---
>> arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
>> index 208af00..032e411 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
>> @@ -104,7 +104,7 @@
>>
>> idle-states {
>
> Is this change depends on some another patchset, I cannot find
> idle-states dt node on mainline nor linux-next ?
The cover letter gives a link to the patchset that contains the basis
for the changes. You can also check out my posted branch which merges
in a part of those changes.
The link to the specific patch is:
http://www.spinics.net/lists/linux-arm-msm/msg19675.html
or you can look at this:
https://git.linaro.org/people/andy.gross/linux-pm.git/shortlog/refs/heads/for-pramod
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 0/5] Qualcomm Suspend to Idle Support
2016-05-19 5:00 ` Andy Gross
@ 2016-05-20 6:09 ` Pramod Gurav
-1 siblings, 0 replies; 62+ messages in thread
From: Pramod Gurav @ 2016-05-20 6:09 UTC (permalink / raw)
To: Andy Gross; +Cc: linux-pm, linux-arm-msm, linux-arm-kernel, Stanimir Varbanov
On 19 May 2016 at 10:30, Andy Gross <andy.gross@linaro.org> wrote:
> This set of patches adds suspend to idle support for the Qualcomm APQ8084
> and MSM8916 platforms. The patches add a minimal set of suspend ops and
> init functions to properly configure the enter_freeze function.
>
> This patch set is based on the work done in the following:
> [RFC v3 00/12] PM: SoC idle support using PM domains
> http://comments.gmane.org/gmane.linux.ports.arm.msm/17966
>
> The relevant prerequisites are the PSCI enablement DT patches. PSCI based
> firmware is also required for the MSM8916 to work properly.
>
> Andy Gross (5):
> soc: qcom: Add suspend to idle support
> arm: defconfig: Add Qualcomm Suspend options
> arm64: dts: msm8916: Add spc compat tag
Tested this series on DB410C. So for these three patches:
Tested-by: Pramod Gurav <pramod.gurav@linaro.org>
> ARM: dts: qcom: Remove size elements from pmic reg
> ARM: dts: qcom: pma8084: Add pwrkey entry
>
> arch/arm/boot/dts/qcom-pma8084.dtsi | 20 ++++++---
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> arch/arm64/configs/defconfig | 2 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/suspend.c | 77 +++++++++++++++++++++++++++++++++++
> 5 files changed, 95 insertions(+), 7 deletions(-)
> create mode 100644 drivers/soc/qcom/suspend.c
>
> --
> 1.9.1
>
> --
> 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
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 0/5] Qualcomm Suspend to Idle Support
@ 2016-05-20 6:09 ` Pramod Gurav
0 siblings, 0 replies; 62+ messages in thread
From: Pramod Gurav @ 2016-05-20 6:09 UTC (permalink / raw)
To: linux-arm-kernel
On 19 May 2016 at 10:30, Andy Gross <andy.gross@linaro.org> wrote:
> This set of patches adds suspend to idle support for the Qualcomm APQ8084
> and MSM8916 platforms. The patches add a minimal set of suspend ops and
> init functions to properly configure the enter_freeze function.
>
> This patch set is based on the work done in the following:
> [RFC v3 00/12] PM: SoC idle support using PM domains
> http://comments.gmane.org/gmane.linux.ports.arm.msm/17966
>
> The relevant prerequisites are the PSCI enablement DT patches. PSCI based
> firmware is also required for the MSM8916 to work properly.
>
> Andy Gross (5):
> soc: qcom: Add suspend to idle support
> arm: defconfig: Add Qualcomm Suspend options
> arm64: dts: msm8916: Add spc compat tag
Tested this series on DB410C. So for these three patches:
Tested-by: Pramod Gurav <pramod.gurav@linaro.org>
> ARM: dts: qcom: Remove size elements from pmic reg
> ARM: dts: qcom: pma8084: Add pwrkey entry
>
> arch/arm/boot/dts/qcom-pma8084.dtsi | 20 ++++++---
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> arch/arm64/configs/defconfig | 2 +
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/suspend.c | 77 +++++++++++++++++++++++++++++++++++
> 5 files changed, 95 insertions(+), 7 deletions(-)
> create mode 100644 drivers/soc/qcom/suspend.c
>
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 1/5] soc: qcom: Add suspend to idle support
2016-05-19 5:00 ` Andy Gross
@ 2016-06-09 7:39 ` Ulf Hansson
-1 siblings, 0 replies; 62+ messages in thread
From: Ulf Hansson @ 2016-06-09 7:39 UTC (permalink / raw)
To: Andy Gross
Cc: linux-pm, linux-arm-msm, linux-arm-kernel, stanimir.varbanov,
Daniel Lezcano
+ Daniel
On 19 May 2016 at 07:00, Andy Gross <andy.gross@linaro.org> wrote:
> This patch adds suspend to idle support for Qualcomm processors. While
> suspend to memory will be a valid state, there won't be any special
> handling or power savings over the suspend to idle.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/suspend.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 78 insertions(+)
> create mode 100644 drivers/soc/qcom/suspend.c
>
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index fdd664e..7c479d3 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
> obj-$(CONFIG_QCOM_SMSM) += smsm.o
> obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> +obj-$(CONFIG_SUSPEND) += suspend.o
> diff --git a/drivers/soc/qcom/suspend.c b/drivers/soc/qcom/suspend.c
> new file mode 100644
> index 0000000..7d3f2dd
> --- /dev/null
> +++ b/drivers/soc/qcom/suspend.c
> @@ -0,0 +1,77 @@
> +/*
> + * (C) Copyright 2016 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/cpuidle.h>
> +#include <linux/suspend.h>
> +
> +
> +static void qcom_pm_enter_freeze(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv,
> + int index)
> +{
> + drv->states[index].enter(dev, drv, index);
> +}
> +
> +static const struct of_device_id qcom_idle_state_match[] = {
> + { .compatible = "qcom,idle-state-spc", },
> + { },
> +};
> +
> +static const struct platform_suspend_ops qcom_suspend_ops = {
> + .valid = suspend_valid_only_mem,
> +};
> +
> +static int __init qcom_pm_init(void)
> +{
> + struct cpuidle_device *cpu_dev;
> + struct cpuidle_driver *cpu_drv;
> + int state_count;
> + struct device_node *state_np, *cpu_np;
> + const struct of_device_id *match;
> + int i;
> +
> + /* configure CPU enter_freeze if applicable */
> + for_each_present_cpu(i) {
> + cpu_np = of_get_cpu_node(i, NULL);
> + cpu_dev = per_cpu_ptr(cpuidle_devices, i);
> + cpu_drv = cpuidle_get_cpu_driver(cpu_dev);
> +
> + if (!cpu_dev || !cpu_drv) {
> + of_node_put(cpu_np);
> + return -EPROBE_DEFER;
This isn't a driver, so returning -EPROBE_DEFER doesn't make sense.
> + }
> +
> + state_count = 0;
> + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
> + state_count);
> +
> + while (state_np) {
> + match = of_match_node(qcom_idle_state_match,
> + state_np);
> +
> + state_count++;
> + if (match)
> + cpu_drv->states[state_count].enter_freeze =
> + &qcom_pm_enter_freeze;
> + of_node_put(state_np);
> +
> + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
> + state_count);
> + }
> +
> + of_node_put(cpu_np);
> + }
> +
> + suspend_set_ops(&qcom_suspend_ops);
I don't think this will work!
When building a multi defconfig for ARM, you might overwrite the
suspend_ops (there's only one set) as here you don't know that it's
actually the QCOM platform that is running, right!?
Perhaps this code actually belongs closer to the cpuidle driver?
> +
> + return 0;
> +}
> +
> +late_initcall(qcom_pm_init);
> --
> 1.9.1
Kind regards
Uffe
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 1/5] soc: qcom: Add suspend to idle support
@ 2016-06-09 7:39 ` Ulf Hansson
0 siblings, 0 replies; 62+ messages in thread
From: Ulf Hansson @ 2016-06-09 7:39 UTC (permalink / raw)
To: linux-arm-kernel
+ Daniel
On 19 May 2016 at 07:00, Andy Gross <andy.gross@linaro.org> wrote:
> This patch adds suspend to idle support for Qualcomm processors. While
> suspend to memory will be a valid state, there won't be any special
> handling or power savings over the suspend to idle.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
> drivers/soc/qcom/Makefile | 1 +
> drivers/soc/qcom/suspend.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 78 insertions(+)
> create mode 100644 drivers/soc/qcom/suspend.c
>
> diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> index fdd664e..7c479d3 100644
> --- a/drivers/soc/qcom/Makefile
> +++ b/drivers/soc/qcom/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
> obj-$(CONFIG_QCOM_SMSM) += smsm.o
> obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> +obj-$(CONFIG_SUSPEND) += suspend.o
> diff --git a/drivers/soc/qcom/suspend.c b/drivers/soc/qcom/suspend.c
> new file mode 100644
> index 0000000..7d3f2dd
> --- /dev/null
> +++ b/drivers/soc/qcom/suspend.c
> @@ -0,0 +1,77 @@
> +/*
> + * (C) Copyright 2016 Linaro Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; version 2
> + * of the License.
> + */
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/cpuidle.h>
> +#include <linux/suspend.h>
> +
> +
> +static void qcom_pm_enter_freeze(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv,
> + int index)
> +{
> + drv->states[index].enter(dev, drv, index);
> +}
> +
> +static const struct of_device_id qcom_idle_state_match[] = {
> + { .compatible = "qcom,idle-state-spc", },
> + { },
> +};
> +
> +static const struct platform_suspend_ops qcom_suspend_ops = {
> + .valid = suspend_valid_only_mem,
> +};
> +
> +static int __init qcom_pm_init(void)
> +{
> + struct cpuidle_device *cpu_dev;
> + struct cpuidle_driver *cpu_drv;
> + int state_count;
> + struct device_node *state_np, *cpu_np;
> + const struct of_device_id *match;
> + int i;
> +
> + /* configure CPU enter_freeze if applicable */
> + for_each_present_cpu(i) {
> + cpu_np = of_get_cpu_node(i, NULL);
> + cpu_dev = per_cpu_ptr(cpuidle_devices, i);
> + cpu_drv = cpuidle_get_cpu_driver(cpu_dev);
> +
> + if (!cpu_dev || !cpu_drv) {
> + of_node_put(cpu_np);
> + return -EPROBE_DEFER;
This isn't a driver, so returning -EPROBE_DEFER doesn't make sense.
> + }
> +
> + state_count = 0;
> + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
> + state_count);
> +
> + while (state_np) {
> + match = of_match_node(qcom_idle_state_match,
> + state_np);
> +
> + state_count++;
> + if (match)
> + cpu_drv->states[state_count].enter_freeze =
> + &qcom_pm_enter_freeze;
> + of_node_put(state_np);
> +
> + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
> + state_count);
> + }
> +
> + of_node_put(cpu_np);
> + }
> +
> + suspend_set_ops(&qcom_suspend_ops);
I don't think this will work!
When building a multi defconfig for ARM, you might overwrite the
suspend_ops (there's only one set) as here you don't know that it's
actually the QCOM platform that is running, right!?
Perhaps this code actually belongs closer to the cpuidle driver?
> +
> + return 0;
> +}
> +
> +late_initcall(qcom_pm_init);
> --
> 1.9.1
Kind regards
Uffe
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 1/5] soc: qcom: Add suspend to idle support
2016-06-09 7:39 ` Ulf Hansson
@ 2016-06-09 18:09 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-09 18:09 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-pm, linux-arm-msm, linux-arm-kernel, stanimir.varbanov,
Daniel Lezcano
On Thu, Jun 09, 2016 at 09:39:34AM +0200, Ulf Hansson wrote:
> + Daniel
>
> On 19 May 2016 at 07:00, Andy Gross <andy.gross@linaro.org> wrote:
> > This patch adds suspend to idle support for Qualcomm processors. While
> > suspend to memory will be a valid state, there won't be any special
> > handling or power savings over the suspend to idle.
> >
> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > ---
> > drivers/soc/qcom/Makefile | 1 +
> > drivers/soc/qcom/suspend.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 78 insertions(+)
> > create mode 100644 drivers/soc/qcom/suspend.c
> >
> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> > index fdd664e..7c479d3 100644
> > --- a/drivers/soc/qcom/Makefile
> > +++ b/drivers/soc/qcom/Makefile
> > @@ -7,3 +7,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> > obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
> > obj-$(CONFIG_QCOM_SMSM) += smsm.o
> > obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> > +obj-$(CONFIG_SUSPEND) += suspend.o
> > diff --git a/drivers/soc/qcom/suspend.c b/drivers/soc/qcom/suspend.c
> > new file mode 100644
> > index 0000000..7d3f2dd
> > --- /dev/null
> > +++ b/drivers/soc/qcom/suspend.c
> > @@ -0,0 +1,77 @@
> > +/*
> > + * (C) Copyright 2016 Linaro Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; version 2
> > + * of the License.
> > + */
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/cpuidle.h>
> > +#include <linux/suspend.h>
> > +
> > +
> > +static void qcom_pm_enter_freeze(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv,
> > + int index)
> > +{
> > + drv->states[index].enter(dev, drv, index);
> > +}
> > +
> > +static const struct of_device_id qcom_idle_state_match[] = {
> > + { .compatible = "qcom,idle-state-spc", },
> > + { },
> > +};
> > +
> > +static const struct platform_suspend_ops qcom_suspend_ops = {
> > + .valid = suspend_valid_only_mem,
> > +};
> > +
> > +static int __init qcom_pm_init(void)
> > +{
> > + struct cpuidle_device *cpu_dev;
> > + struct cpuidle_driver *cpu_drv;
> > + int state_count;
> > + struct device_node *state_np, *cpu_np;
> > + const struct of_device_id *match;
> > + int i;
> > +
> > + /* configure CPU enter_freeze if applicable */
> > + for_each_present_cpu(i) {
> > + cpu_np = of_get_cpu_node(i, NULL);
> > + cpu_dev = per_cpu_ptr(cpuidle_devices, i);
> > + cpu_drv = cpuidle_get_cpu_driver(cpu_dev);
> > +
> > + if (!cpu_dev || !cpu_drv) {
> > + of_node_put(cpu_np);
> > + return -EPROBE_DEFER;
>
> This isn't a driver, so returning -EPROBE_DEFER doesn't make sense.
Right. It's not like it'll get retried.
> > + }
> > +
> > + state_count = 0;
> > + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
> > + state_count);
> > +
> > + while (state_np) {
> > + match = of_match_node(qcom_idle_state_match,
> > + state_np);
> > +
> > + state_count++;
> > + if (match)
> > + cpu_drv->states[state_count].enter_freeze =
> > + &qcom_pm_enter_freeze;
> > + of_node_put(state_np);
> > +
> > + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
> > + state_count);
> > + }
> > +
> > + of_node_put(cpu_np);
> > + }
> > +
> > + suspend_set_ops(&qcom_suspend_ops);
>
> I don't think this will work!
>
> When building a multi defconfig for ARM, you might overwrite the
> suspend_ops (there's only one set) as here you don't know that it's
> actually the QCOM platform that is running, right!?
>
> Perhaps this code actually belongs closer to the cpuidle driver?
Hmmmm, I might have to get creative. I originally had a DT entry for the pm,
but that doesn't make sense as this is purely a software construct. The db410c
uses the arm cpuidle driver so I can't really hook it in there. I'll have to
come up with something else.
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 1/5] soc: qcom: Add suspend to idle support
@ 2016-06-09 18:09 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-09 18:09 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 09, 2016 at 09:39:34AM +0200, Ulf Hansson wrote:
> + Daniel
>
> On 19 May 2016 at 07:00, Andy Gross <andy.gross@linaro.org> wrote:
> > This patch adds suspend to idle support for Qualcomm processors. While
> > suspend to memory will be a valid state, there won't be any special
> > handling or power savings over the suspend to idle.
> >
> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > ---
> > drivers/soc/qcom/Makefile | 1 +
> > drivers/soc/qcom/suspend.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 78 insertions(+)
> > create mode 100644 drivers/soc/qcom/suspend.c
> >
> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
> > index fdd664e..7c479d3 100644
> > --- a/drivers/soc/qcom/Makefile
> > +++ b/drivers/soc/qcom/Makefile
> > @@ -7,3 +7,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
> > obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
> > obj-$(CONFIG_QCOM_SMSM) += smsm.o
> > obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
> > +obj-$(CONFIG_SUSPEND) += suspend.o
> > diff --git a/drivers/soc/qcom/suspend.c b/drivers/soc/qcom/suspend.c
> > new file mode 100644
> > index 0000000..7d3f2dd
> > --- /dev/null
> > +++ b/drivers/soc/qcom/suspend.c
> > @@ -0,0 +1,77 @@
> > +/*
> > + * (C) Copyright 2016 Linaro Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * as published by the Free Software Foundation; version 2
> > + * of the License.
> > + */
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/cpuidle.h>
> > +#include <linux/suspend.h>
> > +
> > +
> > +static void qcom_pm_enter_freeze(struct cpuidle_device *dev,
> > + struct cpuidle_driver *drv,
> > + int index)
> > +{
> > + drv->states[index].enter(dev, drv, index);
> > +}
> > +
> > +static const struct of_device_id qcom_idle_state_match[] = {
> > + { .compatible = "qcom,idle-state-spc", },
> > + { },
> > +};
> > +
> > +static const struct platform_suspend_ops qcom_suspend_ops = {
> > + .valid = suspend_valid_only_mem,
> > +};
> > +
> > +static int __init qcom_pm_init(void)
> > +{
> > + struct cpuidle_device *cpu_dev;
> > + struct cpuidle_driver *cpu_drv;
> > + int state_count;
> > + struct device_node *state_np, *cpu_np;
> > + const struct of_device_id *match;
> > + int i;
> > +
> > + /* configure CPU enter_freeze if applicable */
> > + for_each_present_cpu(i) {
> > + cpu_np = of_get_cpu_node(i, NULL);
> > + cpu_dev = per_cpu_ptr(cpuidle_devices, i);
> > + cpu_drv = cpuidle_get_cpu_driver(cpu_dev);
> > +
> > + if (!cpu_dev || !cpu_drv) {
> > + of_node_put(cpu_np);
> > + return -EPROBE_DEFER;
>
> This isn't a driver, so returning -EPROBE_DEFER doesn't make sense.
Right. It's not like it'll get retried.
> > + }
> > +
> > + state_count = 0;
> > + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
> > + state_count);
> > +
> > + while (state_np) {
> > + match = of_match_node(qcom_idle_state_match,
> > + state_np);
> > +
> > + state_count++;
> > + if (match)
> > + cpu_drv->states[state_count].enter_freeze =
> > + &qcom_pm_enter_freeze;
> > + of_node_put(state_np);
> > +
> > + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
> > + state_count);
> > + }
> > +
> > + of_node_put(cpu_np);
> > + }
> > +
> > + suspend_set_ops(&qcom_suspend_ops);
>
> I don't think this will work!
>
> When building a multi defconfig for ARM, you might overwrite the
> suspend_ops (there's only one set) as here you don't know that it's
> actually the QCOM platform that is running, right!?
>
> Perhaps this code actually belongs closer to the cpuidle driver?
Hmmmm, I might have to get creative. I originally had a DT entry for the pm,
but that doesn't make sense as this is purely a software construct. The db410c
uses the arm cpuidle driver so I can't really hook it in there. I'll have to
come up with something else.
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 1/5] soc: qcom: Add suspend to idle support
2016-06-09 18:09 ` Andy Gross
@ 2016-06-10 8:47 ` Ulf Hansson
-1 siblings, 0 replies; 62+ messages in thread
From: Ulf Hansson @ 2016-06-10 8:47 UTC (permalink / raw)
To: Andy Gross
Cc: linux-pm, linux-arm-msm, linux-arm-kernel, Stanimir Varbanov,
Daniel Lezcano
On 9 June 2016 at 20:09, Andy Gross <andy.gross@linaro.org> wrote:
> On Thu, Jun 09, 2016 at 09:39:34AM +0200, Ulf Hansson wrote:
>> + Daniel
>>
>> On 19 May 2016 at 07:00, Andy Gross <andy.gross@linaro.org> wrote:
>> > This patch adds suspend to idle support for Qualcomm processors. While
>> > suspend to memory will be a valid state, there won't be any special
>> > handling or power savings over the suspend to idle.
>> >
>> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
>> > ---
>> > drivers/soc/qcom/Makefile | 1 +
>> > drivers/soc/qcom/suspend.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>> > 2 files changed, 78 insertions(+)
>> > create mode 100644 drivers/soc/qcom/suspend.c
>> >
>> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> > index fdd664e..7c479d3 100644
>> > --- a/drivers/soc/qcom/Makefile
>> > +++ b/drivers/soc/qcom/Makefile
>> > @@ -7,3 +7,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
>> > obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
>> > obj-$(CONFIG_QCOM_SMSM) += smsm.o
>> > obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>> > +obj-$(CONFIG_SUSPEND) += suspend.o
>> > diff --git a/drivers/soc/qcom/suspend.c b/drivers/soc/qcom/suspend.c
>> > new file mode 100644
>> > index 0000000..7d3f2dd
>> > --- /dev/null
>> > +++ b/drivers/soc/qcom/suspend.c
>> > @@ -0,0 +1,77 @@
>> > +/*
>> > + * (C) Copyright 2016 Linaro Ltd.
>> > + *
>> > + * This program is free software; you can redistribute it and/or
>> > + * modify it under the terms of the GNU General Public License
>> > + * as published by the Free Software Foundation; version 2
>> > + * of the License.
>> > + */
>> > +#include <linux/module.h>
>> > +#include <linux/of.h>
>> > +#include <linux/cpuidle.h>
>> > +#include <linux/suspend.h>
>> > +
>> > +
>> > +static void qcom_pm_enter_freeze(struct cpuidle_device *dev,
>> > + struct cpuidle_driver *drv,
>> > + int index)
>> > +{
>> > + drv->states[index].enter(dev, drv, index);
>> > +}
>> > +
>> > +static const struct of_device_id qcom_idle_state_match[] = {
>> > + { .compatible = "qcom,idle-state-spc", },
>> > + { },
>> > +};
>> > +
>> > +static const struct platform_suspend_ops qcom_suspend_ops = {
>> > + .valid = suspend_valid_only_mem,
>> > +};
>> > +
>> > +static int __init qcom_pm_init(void)
>> > +{
>> > + struct cpuidle_device *cpu_dev;
>> > + struct cpuidle_driver *cpu_drv;
>> > + int state_count;
>> > + struct device_node *state_np, *cpu_np;
>> > + const struct of_device_id *match;
>> > + int i;
>> > +
>> > + /* configure CPU enter_freeze if applicable */
>> > + for_each_present_cpu(i) {
>> > + cpu_np = of_get_cpu_node(i, NULL);
>> > + cpu_dev = per_cpu_ptr(cpuidle_devices, i);
>> > + cpu_drv = cpuidle_get_cpu_driver(cpu_dev);
>> > +
>> > + if (!cpu_dev || !cpu_drv) {
>> > + of_node_put(cpu_np);
>> > + return -EPROBE_DEFER;
>>
>> This isn't a driver, so returning -EPROBE_DEFER doesn't make sense.
>
> Right. It's not like it'll get retried.
>
>> > + }
>> > +
>> > + state_count = 0;
>> > + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
>> > + state_count);
>> > +
>> > + while (state_np) {
>> > + match = of_match_node(qcom_idle_state_match,
>> > + state_np);
>> > +
>> > + state_count++;
>> > + if (match)
>> > + cpu_drv->states[state_count].enter_freeze =
>> > + &qcom_pm_enter_freeze;
>> > + of_node_put(state_np);
>> > +
>> > + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
>> > + state_count);
>> > + }
>> > +
>> > + of_node_put(cpu_np);
>> > + }
>> > +
>> > + suspend_set_ops(&qcom_suspend_ops);
>>
>> I don't think this will work!
>>
>> When building a multi defconfig for ARM, you might overwrite the
>> suspend_ops (there's only one set) as here you don't know that it's
>> actually the QCOM platform that is running, right!?
>>
>> Perhaps this code actually belongs closer to the cpuidle driver?
>
> Hmmmm, I might have to get creative. I originally had a DT entry for the pm,
> but that doesn't make sense as this is purely a software construct. The db410c
> uses the arm cpuidle driver so I can't really hook it in there. I'll have to
> come up with something else.
I did a little research.
I think you should be able to use the struct cpuidle_ops->init()
callback. This is being invoked when the arm cpuidle driver is
initialized.
arm_idle_init()
->arm_cpuidle_init()
->cpuidle_ops->init()
In the QCOM case, it's the spm driver that registers these cpuidle_ops
(drivers/soc/qcom/spm.c). If you fold in the code from $subject patch
in qcom_cpuidle_init(), that should work I think.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 1/5] soc: qcom: Add suspend to idle support
@ 2016-06-10 8:47 ` Ulf Hansson
0 siblings, 0 replies; 62+ messages in thread
From: Ulf Hansson @ 2016-06-10 8:47 UTC (permalink / raw)
To: linux-arm-kernel
On 9 June 2016 at 20:09, Andy Gross <andy.gross@linaro.org> wrote:
> On Thu, Jun 09, 2016 at 09:39:34AM +0200, Ulf Hansson wrote:
>> + Daniel
>>
>> On 19 May 2016 at 07:00, Andy Gross <andy.gross@linaro.org> wrote:
>> > This patch adds suspend to idle support for Qualcomm processors. While
>> > suspend to memory will be a valid state, there won't be any special
>> > handling or power savings over the suspend to idle.
>> >
>> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
>> > ---
>> > drivers/soc/qcom/Makefile | 1 +
>> > drivers/soc/qcom/suspend.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++
>> > 2 files changed, 78 insertions(+)
>> > create mode 100644 drivers/soc/qcom/suspend.c
>> >
>> > diff --git a/drivers/soc/qcom/Makefile b/drivers/soc/qcom/Makefile
>> > index fdd664e..7c479d3 100644
>> > --- a/drivers/soc/qcom/Makefile
>> > +++ b/drivers/soc/qcom/Makefile
>> > @@ -7,3 +7,4 @@ obj-$(CONFIG_QCOM_SMEM_STATE) += smem_state.o
>> > obj-$(CONFIG_QCOM_SMP2P) += smp2p.o
>> > obj-$(CONFIG_QCOM_SMSM) += smsm.o
>> > obj-$(CONFIG_QCOM_WCNSS_CTRL) += wcnss_ctrl.o
>> > +obj-$(CONFIG_SUSPEND) += suspend.o
>> > diff --git a/drivers/soc/qcom/suspend.c b/drivers/soc/qcom/suspend.c
>> > new file mode 100644
>> > index 0000000..7d3f2dd
>> > --- /dev/null
>> > +++ b/drivers/soc/qcom/suspend.c
>> > @@ -0,0 +1,77 @@
>> > +/*
>> > + * (C) Copyright 2016 Linaro Ltd.
>> > + *
>> > + * This program is free software; you can redistribute it and/or
>> > + * modify it under the terms of the GNU General Public License
>> > + * as published by the Free Software Foundation; version 2
>> > + * of the License.
>> > + */
>> > +#include <linux/module.h>
>> > +#include <linux/of.h>
>> > +#include <linux/cpuidle.h>
>> > +#include <linux/suspend.h>
>> > +
>> > +
>> > +static void qcom_pm_enter_freeze(struct cpuidle_device *dev,
>> > + struct cpuidle_driver *drv,
>> > + int index)
>> > +{
>> > + drv->states[index].enter(dev, drv, index);
>> > +}
>> > +
>> > +static const struct of_device_id qcom_idle_state_match[] = {
>> > + { .compatible = "qcom,idle-state-spc", },
>> > + { },
>> > +};
>> > +
>> > +static const struct platform_suspend_ops qcom_suspend_ops = {
>> > + .valid = suspend_valid_only_mem,
>> > +};
>> > +
>> > +static int __init qcom_pm_init(void)
>> > +{
>> > + struct cpuidle_device *cpu_dev;
>> > + struct cpuidle_driver *cpu_drv;
>> > + int state_count;
>> > + struct device_node *state_np, *cpu_np;
>> > + const struct of_device_id *match;
>> > + int i;
>> > +
>> > + /* configure CPU enter_freeze if applicable */
>> > + for_each_present_cpu(i) {
>> > + cpu_np = of_get_cpu_node(i, NULL);
>> > + cpu_dev = per_cpu_ptr(cpuidle_devices, i);
>> > + cpu_drv = cpuidle_get_cpu_driver(cpu_dev);
>> > +
>> > + if (!cpu_dev || !cpu_drv) {
>> > + of_node_put(cpu_np);
>> > + return -EPROBE_DEFER;
>>
>> This isn't a driver, so returning -EPROBE_DEFER doesn't make sense.
>
> Right. It's not like it'll get retried.
>
>> > + }
>> > +
>> > + state_count = 0;
>> > + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
>> > + state_count);
>> > +
>> > + while (state_np) {
>> > + match = of_match_node(qcom_idle_state_match,
>> > + state_np);
>> > +
>> > + state_count++;
>> > + if (match)
>> > + cpu_drv->states[state_count].enter_freeze =
>> > + &qcom_pm_enter_freeze;
>> > + of_node_put(state_np);
>> > +
>> > + state_np = of_parse_phandle(cpu_np, "cpu-idle-states",
>> > + state_count);
>> > + }
>> > +
>> > + of_node_put(cpu_np);
>> > + }
>> > +
>> > + suspend_set_ops(&qcom_suspend_ops);
>>
>> I don't think this will work!
>>
>> When building a multi defconfig for ARM, you might overwrite the
>> suspend_ops (there's only one set) as here you don't know that it's
>> actually the QCOM platform that is running, right!?
>>
>> Perhaps this code actually belongs closer to the cpuidle driver?
>
> Hmmmm, I might have to get creative. I originally had a DT entry for the pm,
> but that doesn't make sense as this is purely a software construct. The db410c
> uses the arm cpuidle driver so I can't really hook it in there. I'll have to
> come up with something else.
I did a little research.
I think you should be able to use the struct cpuidle_ops->init()
callback. This is being invoked when the arm cpuidle driver is
initialized.
arm_idle_init()
->arm_cpuidle_init()
->cpuidle_ops->init()
In the QCOM case, it's the spm driver that registers these cpuidle_ops
(drivers/soc/qcom/spm.c). If you fold in the code from $subject patch
in qcom_cpuidle_init(), that should work I think.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 1/5] soc: qcom: Add suspend to idle support
2016-06-10 8:47 ` Ulf Hansson
@ 2016-06-10 15:26 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 15:26 UTC (permalink / raw)
To: Ulf Hansson
Cc: linux-pm, linux-arm-msm, linux-arm-kernel, Stanimir Varbanov,
Daniel Lezcano
On Fri, Jun 10, 2016 at 10:47:35AM +0200, Ulf Hansson wrote:
<snip>
> >>
> >> I don't think this will work!
> >>
> >> When building a multi defconfig for ARM, you might overwrite the
> >> suspend_ops (there's only one set) as here you don't know that it's
> >> actually the QCOM platform that is running, right!?
> >>
> >> Perhaps this code actually belongs closer to the cpuidle driver?
> >
> > Hmmmm, I might have to get creative. I originally had a DT entry for the pm,
> > but that doesn't make sense as this is purely a software construct. The db410c
> > uses the arm cpuidle driver so I can't really hook it in there. I'll have to
> > come up with something else.
>
> I did a little research.
>
> I think you should be able to use the struct cpuidle_ops->init()
> callback. This is being invoked when the arm cpuidle driver is
> initialized.
>
> arm_idle_init()
> ->arm_cpuidle_init()
> ->cpuidle_ops->init()
Ah I'll see if I can get that to work. Thanks for the pointer.
>
> In the QCOM case, it's the spm driver that registers these cpuidle_ops
> (drivers/soc/qcom/spm.c). If you fold in the code from $subject patch
> in qcom_cpuidle_init(), that should work I think.
Only for the 32 bit processor case. The SPM is not used for the 64 bit procs.
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 1/5] soc: qcom: Add suspend to idle support
@ 2016-06-10 15:26 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 15:26 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10, 2016 at 10:47:35AM +0200, Ulf Hansson wrote:
<snip>
> >>
> >> I don't think this will work!
> >>
> >> When building a multi defconfig for ARM, you might overwrite the
> >> suspend_ops (there's only one set) as here you don't know that it's
> >> actually the QCOM platform that is running, right!?
> >>
> >> Perhaps this code actually belongs closer to the cpuidle driver?
> >
> > Hmmmm, I might have to get creative. I originally had a DT entry for the pm,
> > but that doesn't make sense as this is purely a software construct. The db410c
> > uses the arm cpuidle driver so I can't really hook it in there. I'll have to
> > come up with something else.
>
> I did a little research.
>
> I think you should be able to use the struct cpuidle_ops->init()
> callback. This is being invoked when the arm cpuidle driver is
> initialized.
>
> arm_idle_init()
> ->arm_cpuidle_init()
> ->cpuidle_ops->init()
Ah I'll see if I can get that to work. Thanks for the pointer.
>
> In the QCOM case, it's the spm driver that registers these cpuidle_ops
> (drivers/soc/qcom/spm.c). If you fold in the code from $subject patch
> in qcom_cpuidle_init(), that should work I think.
Only for the 32 bit processor case. The SPM is not used for the 64 bit procs.
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-05-19 5:00 ` Andy Gross
@ 2016-06-10 15:48 ` Mark Rutland
-1 siblings, 0 replies; 62+ messages in thread
From: Mark Rutland @ 2016-06-10 15:48 UTC (permalink / raw)
To: Andy Gross
Cc: linux-pm, linux-arm-msm, linux-arm-kernel, stanimir.varbanov,
lorenzo.pieralisi
[+ Lorenzo]
On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> This patch adds the qcom,idle-state-spc compatible to the SPC idle
> state. This compatible indicates that the state is one which supports
> freeze.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 208af00..032e411 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -104,7 +104,7 @@
>
> idle-states {
> CPU_SPC: spc {
> - compatible = "arm,idle-state";
> + compatible = "qcom,idle-state-spc", "arm,idle-state";
> arm,psci-suspend-param = <0x40000002>;
> entry-latency-us = <130>;
> exit-latency-us = <150>;
This looks suspicious.
This is a PSCI idle state, and we have a PSCI driver driven by the
generic ARM cpuidle driver.
Why do we need a qcom-specific compatible here?
Surely we should be able to use the idle code in a generic fashion to
driver suspend-to-idle?
Thank,
Mark.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 15:48 ` Mark Rutland
0 siblings, 0 replies; 62+ messages in thread
From: Mark Rutland @ 2016-06-10 15:48 UTC (permalink / raw)
To: linux-arm-kernel
[+ Lorenzo]
On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> This patch adds the qcom,idle-state-spc compatible to the SPC idle
> state. This compatible indicates that the state is one which supports
> freeze.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
> ---
> arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> index 208af00..032e411 100644
> --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> @@ -104,7 +104,7 @@
>
> idle-states {
> CPU_SPC: spc {
> - compatible = "arm,idle-state";
> + compatible = "qcom,idle-state-spc", "arm,idle-state";
> arm,psci-suspend-param = <0x40000002>;
> entry-latency-us = <130>;
> exit-latency-us = <150>;
This looks suspicious.
This is a PSCI idle state, and we have a PSCI driver driven by the
generic ARM cpuidle driver.
Why do we need a qcom-specific compatible here?
Surely we should be able to use the idle code in a generic fashion to
driver suspend-to-idle?
Thank,
Mark.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 15:48 ` Mark Rutland
@ 2016-06-10 16:12 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 16:12 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-pm, linux-arm-msm, linux-arm-kernel, stanimir.varbanov,
lorenzo.pieralisi
On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> [+ Lorenzo]
>
> On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > state. This compatible indicates that the state is one which supports
> > freeze.
> >
> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > ---
> > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > index 208af00..032e411 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > @@ -104,7 +104,7 @@
> >
> > idle-states {
> > CPU_SPC: spc {
> > - compatible = "arm,idle-state";
> > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > arm,psci-suspend-param = <0x40000002>;
> > entry-latency-us = <130>;
> > exit-latency-us = <150>;
>
> This looks suspicious.
>
> This is a PSCI idle state, and we have a PSCI driver driven by the
> generic ARM cpuidle driver.
>
> Why do we need a qcom-specific compatible here?
>
> Surely we should be able to use the idle code in a generic fashion to
> driver suspend-to-idle?
We need a way to identify specific idle states that support suspend-to-idle. In
addition, when we have identified the states, we may have to configure the
enter_freeze() function.
I chose to do this outside of the arm cpuidle driver because I didn't want to
add any more DT information aside from the compatible, and I needed a separate
place for the Qualcomm specific suspend code. With the compatible, this makes
my 32 and 64 bit processor suspend code identical, as we have our own cpuidle
driver for the 32 bit procs.
An alternative would be to add some facilities to communicate this to the arm
cpuidle driver and configure the enter_freeze() function at some later point.
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 16:12 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 16:12 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> [+ Lorenzo]
>
> On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > state. This compatible indicates that the state is one which supports
> > freeze.
> >
> > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > ---
> > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > index 208af00..032e411 100644
> > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > @@ -104,7 +104,7 @@
> >
> > idle-states {
> > CPU_SPC: spc {
> > - compatible = "arm,idle-state";
> > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > arm,psci-suspend-param = <0x40000002>;
> > entry-latency-us = <130>;
> > exit-latency-us = <150>;
>
> This looks suspicious.
>
> This is a PSCI idle state, and we have a PSCI driver driven by the
> generic ARM cpuidle driver.
>
> Why do we need a qcom-specific compatible here?
>
> Surely we should be able to use the idle code in a generic fashion to
> driver suspend-to-idle?
We need a way to identify specific idle states that support suspend-to-idle. In
addition, when we have identified the states, we may have to configure the
enter_freeze() function.
I chose to do this outside of the arm cpuidle driver because I didn't want to
add any more DT information aside from the compatible, and I needed a separate
place for the Qualcomm specific suspend code. With the compatible, this makes
my 32 and 64 bit processor suspend code identical, as we have our own cpuidle
driver for the 32 bit procs.
An alternative would be to add some facilities to communicate this to the arm
cpuidle driver and configure the enter_freeze() function at some later point.
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 16:12 ` Andy Gross
@ 2016-06-10 16:25 ` Mark Rutland
-1 siblings, 0 replies; 62+ messages in thread
From: Mark Rutland @ 2016-06-10 16:25 UTC (permalink / raw)
To: Andy Gross
Cc: linux-pm, linux-arm-msm, linux-arm-kernel, stanimir.varbanov,
lorenzo.pieralisi
On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > [+ Lorenzo]
> >
> > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > state. This compatible indicates that the state is one which supports
> > > freeze.
> > >
> > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > ---
> > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > index 208af00..032e411 100644
> > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > @@ -104,7 +104,7 @@
> > >
> > > idle-states {
> > > CPU_SPC: spc {
> > > - compatible = "arm,idle-state";
> > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > arm,psci-suspend-param = <0x40000002>;
> > > entry-latency-us = <130>;
> > > exit-latency-us = <150>;
> >
> > This looks suspicious.
> >
> > This is a PSCI idle state, and we have a PSCI driver driven by the
> > generic ARM cpuidle driver.
> >
> > Why do we need a qcom-specific compatible here?
> >
> > Surely we should be able to use the idle code in a generic fashion to
> > driver suspend-to-idle?
>
> We need a way to identify specific idle states that support suspend-to-idle. In
> addition, when we have identified the states, we may have to configure the
> enter_freeze() function.
Could you elaborate on what you mean by a state supporting
suspend-to-idle? It was my understanding that any idle state should
function for suspend-to-idle (and the choice of state is potentially
subjective).
> I chose to do this outside of the arm cpuidle driver because I didn't want to
> add any more DT information aside from the compatible, and I needed a separate
> place for the Qualcomm specific suspend code.
Which suspend code is Qualcomm-specific? There shouldn't be anything on
the PSCI side, so I can only imagine that device management is left.
What am I missing?
> With the compatible, this makes
> my 32 and 64 bit processor suspend code identical, as we have our own cpuidle
> driver for the 32 bit procs.
>
> An alternative would be to add some facilities to communicate this to the arm
> cpuidle driver and configure the enter_freeze() function at some later point.
I would prefer that suspend-to-idle using PSCI occurred via the generic
PSCI and cpuidle code. I am happy to extend that code if there is
something lacking, and I would prefer to not have platform-specific
hooks.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 16:25 ` Mark Rutland
0 siblings, 0 replies; 62+ messages in thread
From: Mark Rutland @ 2016-06-10 16:25 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > [+ Lorenzo]
> >
> > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > state. This compatible indicates that the state is one which supports
> > > freeze.
> > >
> > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > ---
> > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > index 208af00..032e411 100644
> > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > @@ -104,7 +104,7 @@
> > >
> > > idle-states {
> > > CPU_SPC: spc {
> > > - compatible = "arm,idle-state";
> > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > arm,psci-suspend-param = <0x40000002>;
> > > entry-latency-us = <130>;
> > > exit-latency-us = <150>;
> >
> > This looks suspicious.
> >
> > This is a PSCI idle state, and we have a PSCI driver driven by the
> > generic ARM cpuidle driver.
> >
> > Why do we need a qcom-specific compatible here?
> >
> > Surely we should be able to use the idle code in a generic fashion to
> > driver suspend-to-idle?
>
> We need a way to identify specific idle states that support suspend-to-idle. In
> addition, when we have identified the states, we may have to configure the
> enter_freeze() function.
Could you elaborate on what you mean by a state supporting
suspend-to-idle? It was my understanding that any idle state should
function for suspend-to-idle (and the choice of state is potentially
subjective).
> I chose to do this outside of the arm cpuidle driver because I didn't want to
> add any more DT information aside from the compatible, and I needed a separate
> place for the Qualcomm specific suspend code.
Which suspend code is Qualcomm-specific? There shouldn't be anything on
the PSCI side, so I can only imagine that device management is left.
What am I missing?
> With the compatible, this makes
> my 32 and 64 bit processor suspend code identical, as we have our own cpuidle
> driver for the 32 bit procs.
>
> An alternative would be to add some facilities to communicate this to the arm
> cpuidle driver and configure the enter_freeze() function at some later point.
I would prefer that suspend-to-idle using PSCI occurred via the generic
PSCI and cpuidle code. I am happy to extend that code if there is
something lacking, and I would prefer to not have platform-specific
hooks.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 16:12 ` Andy Gross
@ 2016-06-10 16:31 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 62+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-10 16:31 UTC (permalink / raw)
To: Andy Gross
Cc: Mark Rutland, linux-pm, linux-arm-msm, linux-arm-kernel,
stanimir.varbanov
On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > [+ Lorenzo]
> >
> > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > state. This compatible indicates that the state is one which supports
> > > freeze.
> > >
> > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > ---
> > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > index 208af00..032e411 100644
> > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > @@ -104,7 +104,7 @@
> > >
> > > idle-states {
> > > CPU_SPC: spc {
> > > - compatible = "arm,idle-state";
> > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > arm,psci-suspend-param = <0x40000002>;
> > > entry-latency-us = <130>;
> > > exit-latency-us = <150>;
> >
> > This looks suspicious.
> >
> > This is a PSCI idle state, and we have a PSCI driver driven by the
> > generic ARM cpuidle driver.
> >
> > Why do we need a qcom-specific compatible here?
> >
> > Surely we should be able to use the idle code in a generic fashion to
> > driver suspend-to-idle?
>
> We need a way to identify specific idle states that support
> suspend-to-idle. In addition, when we have identified the states, we
> may have to configure the enter_freeze() function.
>
> I chose to do this outside of the arm cpuidle driver because I didn't
> want to add any more DT information aside from the compatible, and I
> needed a separate place for the Qualcomm specific suspend code. With
> the compatible, this makes my 32 and 64 bit processor suspend code
> identical, as we have our own cpuidle driver for the 32 bit procs.
>
> An alternative would be to add some facilities to communicate this to
> the arm cpuidle driver and configure the enter_freeze() function at
> some later point.
(1) enter_freeze() hooks are not strictly necessary to enable
suspend-to-idle (they are if we want the tick to be frozen
on suspend-to-idle, which is different)
(2) If I understand your code correctly you have to set the suspend
ops hook to make sure suspend-to-idle is enabled. This is a core
code issue rather than anything else, given that suspend-to-idle
(hey it is based on CPUidle !) does NOT rely on suspend ops to
function.
So the gist is: as far as I am concerned you do not need any of this
code to enable (yes you need PSCI idle states but no
qcom,idle-state-spc compatible string whatsoever) suspend-to-idle
on ARM64 on top of PSCI, let me know what I am missing.
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 16:31 ` Lorenzo Pieralisi
0 siblings, 0 replies; 62+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-10 16:31 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > [+ Lorenzo]
> >
> > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > state. This compatible indicates that the state is one which supports
> > > freeze.
> > >
> > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > ---
> > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > index 208af00..032e411 100644
> > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > @@ -104,7 +104,7 @@
> > >
> > > idle-states {
> > > CPU_SPC: spc {
> > > - compatible = "arm,idle-state";
> > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > arm,psci-suspend-param = <0x40000002>;
> > > entry-latency-us = <130>;
> > > exit-latency-us = <150>;
> >
> > This looks suspicious.
> >
> > This is a PSCI idle state, and we have a PSCI driver driven by the
> > generic ARM cpuidle driver.
> >
> > Why do we need a qcom-specific compatible here?
> >
> > Surely we should be able to use the idle code in a generic fashion to
> > driver suspend-to-idle?
>
> We need a way to identify specific idle states that support
> suspend-to-idle. In addition, when we have identified the states, we
> may have to configure the enter_freeze() function.
>
> I chose to do this outside of the arm cpuidle driver because I didn't
> want to add any more DT information aside from the compatible, and I
> needed a separate place for the Qualcomm specific suspend code. With
> the compatible, this makes my 32 and 64 bit processor suspend code
> identical, as we have our own cpuidle driver for the 32 bit procs.
>
> An alternative would be to add some facilities to communicate this to
> the arm cpuidle driver and configure the enter_freeze() function at
> some later point.
(1) enter_freeze() hooks are not strictly necessary to enable
suspend-to-idle (they are if we want the tick to be frozen
on suspend-to-idle, which is different)
(2) If I understand your code correctly you have to set the suspend
ops hook to make sure suspend-to-idle is enabled. This is a core
code issue rather than anything else, given that suspend-to-idle
(hey it is based on CPUidle !) does NOT rely on suspend ops to
function.
So the gist is: as far as I am concerned you do not need any of this
code to enable (yes you need PSCI idle states but no
qcom,idle-state-spc compatible string whatsoever) suspend-to-idle
on ARM64 on top of PSCI, let me know what I am missing.
Thanks,
Lorenzo
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 16:25 ` Mark Rutland
@ 2016-06-10 16:47 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 16:47 UTC (permalink / raw)
To: Mark Rutland
Cc: linux-pm, linux-arm-msm, linux-arm-kernel, stanimir.varbanov,
lorenzo.pieralisi
On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > > [+ Lorenzo]
> > >
> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > > state. This compatible indicates that the state is one which supports
> > > > freeze.
> > > >
> > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > > ---
> > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > index 208af00..032e411 100644
> > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > @@ -104,7 +104,7 @@
> > > >
> > > > idle-states {
> > > > CPU_SPC: spc {
> > > > - compatible = "arm,idle-state";
> > > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > > arm,psci-suspend-param = <0x40000002>;
> > > > entry-latency-us = <130>;
> > > > exit-latency-us = <150>;
> > >
> > > This looks suspicious.
> > >
> > > This is a PSCI idle state, and we have a PSCI driver driven by the
> > > generic ARM cpuidle driver.
> > >
> > > Why do we need a qcom-specific compatible here?
> > >
> > > Surely we should be able to use the idle code in a generic fashion to
> > > driver suspend-to-idle?
> >
> > We need a way to identify specific idle states that support suspend-to-idle. In
> > addition, when we have identified the states, we may have to configure the
> > enter_freeze() function.
>
> Could you elaborate on what you mean by a state supporting
> suspend-to-idle? It was my understanding that any idle state should
> function for suspend-to-idle (and the choice of state is potentially
> subjective).
when you freeze the system, cpuidle will try to find the deepest state which
supports freeze (by checking if a enter_freeze() exists). If it does exist,
then the tick is frozen and the enter_freeze is called as each cpu goes idle.
If no enter_freeze exists, the system will suspend, but will continue to have
interrupts occur due to the tick and other bookkeeping stuff.
>
> > I chose to do this outside of the arm cpuidle driver because I didn't want to
> > add any more DT information aside from the compatible, and I needed a separate
> > place for the Qualcomm specific suspend code.
>
> Which suspend code is Qualcomm-specific? There shouldn't be anything on
> the PSCI side, so I can only imagine that device management is left.
> What am I missing?
Qualcomm won't be supporting the psci suspend feature and will be doing their
own thing. As such, they need their own suspend ops. In addition, we have
platforms that don't use psci (32 bit).
> > With the compatible, this makes
> > my 32 and 64 bit processor suspend code identical, as we have our own cpuidle
> > driver for the 32 bit procs.
> >
> > An alternative would be to add some facilities to communicate this to the arm
> > cpuidle driver and configure the enter_freeze() function at some later point.
>
> I would prefer that suspend-to-idle using PSCI occurred via the generic
> PSCI and cpuidle code. I am happy to extend that code if there is
> something lacking, and I would prefer to not have platform-specific
> hooks.
Right, I briefly looked at that in the beginning and decided against it. But if
we can at least add some way to identify freezeable idle states and configure
the freeze function, that'd cover all the requirements.
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 16:47 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 16:47 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > > [+ Lorenzo]
> > >
> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > > state. This compatible indicates that the state is one which supports
> > > > freeze.
> > > >
> > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > > ---
> > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > index 208af00..032e411 100644
> > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > @@ -104,7 +104,7 @@
> > > >
> > > > idle-states {
> > > > CPU_SPC: spc {
> > > > - compatible = "arm,idle-state";
> > > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > > arm,psci-suspend-param = <0x40000002>;
> > > > entry-latency-us = <130>;
> > > > exit-latency-us = <150>;
> > >
> > > This looks suspicious.
> > >
> > > This is a PSCI idle state, and we have a PSCI driver driven by the
> > > generic ARM cpuidle driver.
> > >
> > > Why do we need a qcom-specific compatible here?
> > >
> > > Surely we should be able to use the idle code in a generic fashion to
> > > driver suspend-to-idle?
> >
> > We need a way to identify specific idle states that support suspend-to-idle. In
> > addition, when we have identified the states, we may have to configure the
> > enter_freeze() function.
>
> Could you elaborate on what you mean by a state supporting
> suspend-to-idle? It was my understanding that any idle state should
> function for suspend-to-idle (and the choice of state is potentially
> subjective).
when you freeze the system, cpuidle will try to find the deepest state which
supports freeze (by checking if a enter_freeze() exists). If it does exist,
then the tick is frozen and the enter_freeze is called as each cpu goes idle.
If no enter_freeze exists, the system will suspend, but will continue to have
interrupts occur due to the tick and other bookkeeping stuff.
>
> > I chose to do this outside of the arm cpuidle driver because I didn't want to
> > add any more DT information aside from the compatible, and I needed a separate
> > place for the Qualcomm specific suspend code.
>
> Which suspend code is Qualcomm-specific? There shouldn't be anything on
> the PSCI side, so I can only imagine that device management is left.
> What am I missing?
Qualcomm won't be supporting the psci suspend feature and will be doing their
own thing. As such, they need their own suspend ops. In addition, we have
platforms that don't use psci (32 bit).
> > With the compatible, this makes
> > my 32 and 64 bit processor suspend code identical, as we have our own cpuidle
> > driver for the 32 bit procs.
> >
> > An alternative would be to add some facilities to communicate this to the arm
> > cpuidle driver and configure the enter_freeze() function at some later point.
>
> I would prefer that suspend-to-idle using PSCI occurred via the generic
> PSCI and cpuidle code. I am happy to extend that code if there is
> something lacking, and I would prefer to not have platform-specific
> hooks.
Right, I briefly looked at that in the beginning and decided against it. But if
we can at least add some way to identify freezeable idle states and configure
the freeze function, that'd cover all the requirements.
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 16:31 ` Lorenzo Pieralisi
@ 2016-06-10 16:52 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 16:52 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Mark Rutland, linux-pm, linux-arm-msm, linux-arm-kernel,
stanimir.varbanov
On Fri, Jun 10, 2016 at 05:31:53PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > > [+ Lorenzo]
> > >
> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > > state. This compatible indicates that the state is one which supports
> > > > freeze.
> > > >
> > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > > ---
> > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > index 208af00..032e411 100644
> > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > @@ -104,7 +104,7 @@
> > > >
> > > > idle-states {
> > > > CPU_SPC: spc {
> > > > - compatible = "arm,idle-state";
> > > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > > arm,psci-suspend-param = <0x40000002>;
> > > > entry-latency-us = <130>;
> > > > exit-latency-us = <150>;
> > >
> > > This looks suspicious.
> > >
> > > This is a PSCI idle state, and we have a PSCI driver driven by the
> > > generic ARM cpuidle driver.
> > >
> > > Why do we need a qcom-specific compatible here?
> > >
> > > Surely we should be able to use the idle code in a generic fashion to
> > > driver suspend-to-idle?
> >
> > We need a way to identify specific idle states that support
> > suspend-to-idle. In addition, when we have identified the states, we
> > may have to configure the enter_freeze() function.
> >
> > I chose to do this outside of the arm cpuidle driver because I didn't
> > want to add any more DT information aside from the compatible, and I
> > needed a separate place for the Qualcomm specific suspend code. With
> > the compatible, this makes my 32 and 64 bit processor suspend code
> > identical, as we have our own cpuidle driver for the 32 bit procs.
> >
> > An alternative would be to add some facilities to communicate this to
> > the arm cpuidle driver and configure the enter_freeze() function at
> > some later point.
>
> (1) enter_freeze() hooks are not strictly necessary to enable
> suspend-to-idle (they are if we want the tick to be frozen
> on suspend-to-idle, which is different)
I'd think that you'd want the tick frozen. Even if you are going to just call
the deepest freezable idle state in your freeze_function, you don't want to keep
getting woken up as this costs some power usage
> (2) If I understand your code correctly you have to set the suspend
> ops hook to make sure suspend-to-idle is enabled. This is a core
> code issue rather than anything else, given that suspend-to-idle
> (hey it is based on CPUidle !) does NOT rely on suspend ops to
> function.
It only requires having suspend_ops and a valid function. Otherwise you can
never suspend and never exercise the freeze portion of the cpuidle code.
>
> So the gist is: as far as I am concerned you do not need any of this
> code to enable (yes you need PSCI idle states but no
> qcom,idle-state-spc compatible string whatsoever) suspend-to-idle
> on ARM64 on top of PSCI, let me know what I am missing.
If we had the facilities in the arm cpuidle driver then for 64 bit processors, I
wouldn't have to do anything except provision my suspend_ops + valid function.
For 32 bit, we actually already use this compat tag and I just have to add code
in the spm driver (qcom cpuidle) to init the suspend_ops.
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 16:52 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 16:52 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10, 2016 at 05:31:53PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > > [+ Lorenzo]
> > >
> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > > state. This compatible indicates that the state is one which supports
> > > > freeze.
> > > >
> > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > > ---
> > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > index 208af00..032e411 100644
> > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > @@ -104,7 +104,7 @@
> > > >
> > > > idle-states {
> > > > CPU_SPC: spc {
> > > > - compatible = "arm,idle-state";
> > > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > > arm,psci-suspend-param = <0x40000002>;
> > > > entry-latency-us = <130>;
> > > > exit-latency-us = <150>;
> > >
> > > This looks suspicious.
> > >
> > > This is a PSCI idle state, and we have a PSCI driver driven by the
> > > generic ARM cpuidle driver.
> > >
> > > Why do we need a qcom-specific compatible here?
> > >
> > > Surely we should be able to use the idle code in a generic fashion to
> > > driver suspend-to-idle?
> >
> > We need a way to identify specific idle states that support
> > suspend-to-idle. In addition, when we have identified the states, we
> > may have to configure the enter_freeze() function.
> >
> > I chose to do this outside of the arm cpuidle driver because I didn't
> > want to add any more DT information aside from the compatible, and I
> > needed a separate place for the Qualcomm specific suspend code. With
> > the compatible, this makes my 32 and 64 bit processor suspend code
> > identical, as we have our own cpuidle driver for the 32 bit procs.
> >
> > An alternative would be to add some facilities to communicate this to
> > the arm cpuidle driver and configure the enter_freeze() function at
> > some later point.
>
> (1) enter_freeze() hooks are not strictly necessary to enable
> suspend-to-idle (they are if we want the tick to be frozen
> on suspend-to-idle, which is different)
I'd think that you'd want the tick frozen. Even if you are going to just call
the deepest freezable idle state in your freeze_function, you don't want to keep
getting woken up as this costs some power usage
> (2) If I understand your code correctly you have to set the suspend
> ops hook to make sure suspend-to-idle is enabled. This is a core
> code issue rather than anything else, given that suspend-to-idle
> (hey it is based on CPUidle !) does NOT rely on suspend ops to
> function.
It only requires having suspend_ops and a valid function. Otherwise you can
never suspend and never exercise the freeze portion of the cpuidle code.
>
> So the gist is: as far as I am concerned you do not need any of this
> code to enable (yes you need PSCI idle states but no
> qcom,idle-state-spc compatible string whatsoever) suspend-to-idle
> on ARM64 on top of PSCI, let me know what I am missing.
If we had the facilities in the arm cpuidle driver then for 64 bit processors, I
wouldn't have to do anything except provision my suspend_ops + valid function.
For 32 bit, we actually already use this compat tag and I just have to add code
in the spm driver (qcom cpuidle) to init the suspend_ops.
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 16:52 ` Andy Gross
@ 2016-06-10 17:06 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 62+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-10 17:06 UTC (permalink / raw)
To: Andy Gross
Cc: Mark Rutland, linux-pm, linux-arm-msm, linux-arm-kernel,
stanimir.varbanov
On Fri, Jun 10, 2016 at 11:52:48AM -0500, Andy Gross wrote:
[...]
> > (1) enter_freeze() hooks are not strictly necessary to enable
> > suspend-to-idle (they are if we want the tick to be frozen
> > on suspend-to-idle, which is different)
>
> I'd think that you'd want the tick frozen. Even if you are going to
> just call the deepest freezable idle state in your freeze_function,
> you don't want to keep getting woken up as this costs some power usage
As I said, that's a separate issue from these bindings.
> > (2) If I understand your code correctly you have to set the suspend
> > ops hook to make sure suspend-to-idle is enabled. This is a core
> > code issue rather than anything else, given that suspend-to-idle
> > (hey it is based on CPUidle !) does NOT rely on suspend ops to
> > function.
>
> It only requires having suspend_ops and a valid function. Otherwise
> you can never suspend and never exercise the freeze portion of the
> cpuidle code.
As I said, *currently* you have to call suspend_set_ops() to set
up the pm_states label for PM_SUSPEND_FREEZE, suspend_ops are
irrelevant to make suspend-to-idle work and as I said that's
related to core code rather than platform specific suspend ops.
We should solve issues, not work around them.
> > So the gist is: as far as I am concerned you do not need any of this
> > code to enable (yes you need PSCI idle states but no
> > qcom,idle-state-spc compatible string whatsoever) suspend-to-idle
> > on ARM64 on top of PSCI, let me know what I am missing.
>
> If we had the facilities in the arm cpuidle driver then for 64 bit
> processors, I wouldn't have to do anything except provision my
> suspend_ops + valid function. For 32 bit, we actually already use
> this compat tag and I just have to add code in the spm driver (qcom
> cpuidle) to init the suspend_ops.
For 64-bit you do not have to have add any facility.
1) we should change core code to make PM_SUSPEND_FREEZE independent
of suspend_set_ops()
2) we should define which idle states are freezable (99% of them are
minus coupled idle states), through generic bindings
Lorenzo
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 17:06 ` Lorenzo Pieralisi
0 siblings, 0 replies; 62+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-10 17:06 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10, 2016 at 11:52:48AM -0500, Andy Gross wrote:
[...]
> > (1) enter_freeze() hooks are not strictly necessary to enable
> > suspend-to-idle (they are if we want the tick to be frozen
> > on suspend-to-idle, which is different)
>
> I'd think that you'd want the tick frozen. Even if you are going to
> just call the deepest freezable idle state in your freeze_function,
> you don't want to keep getting woken up as this costs some power usage
As I said, that's a separate issue from these bindings.
> > (2) If I understand your code correctly you have to set the suspend
> > ops hook to make sure suspend-to-idle is enabled. This is a core
> > code issue rather than anything else, given that suspend-to-idle
> > (hey it is based on CPUidle !) does NOT rely on suspend ops to
> > function.
>
> It only requires having suspend_ops and a valid function. Otherwise
> you can never suspend and never exercise the freeze portion of the
> cpuidle code.
As I said, *currently* you have to call suspend_set_ops() to set
up the pm_states label for PM_SUSPEND_FREEZE, suspend_ops are
irrelevant to make suspend-to-idle work and as I said that's
related to core code rather than platform specific suspend ops.
We should solve issues, not work around them.
> > So the gist is: as far as I am concerned you do not need any of this
> > code to enable (yes you need PSCI idle states but no
> > qcom,idle-state-spc compatible string whatsoever) suspend-to-idle
> > on ARM64 on top of PSCI, let me know what I am missing.
>
> If we had the facilities in the arm cpuidle driver then for 64 bit
> processors, I wouldn't have to do anything except provision my
> suspend_ops + valid function. For 32 bit, we actually already use
> this compat tag and I just have to add code in the spm driver (qcom
> cpuidle) to init the suspend_ops.
For 64-bit you do not have to have add any facility.
1) we should change core code to make PM_SUSPEND_FREEZE independent
of suspend_set_ops()
2) we should define which idle states are freezable (99% of them are
minus coupled idle states), through generic bindings
Lorenzo
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 17:06 ` Lorenzo Pieralisi
@ 2016-06-10 17:27 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 17:27 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Mark Rutland, linux-pm, linux-arm-msm, linux-arm-kernel,
stanimir.varbanov
On Fri, Jun 10, 2016 at 06:06:56PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Jun 10, 2016 at 11:52:48AM -0500, Andy Gross wrote:
>
> [...]
>
> > > (1) enter_freeze() hooks are not strictly necessary to enable
> > > suspend-to-idle (they are if we want the tick to be frozen
> > > on suspend-to-idle, which is different)
> >
> > I'd think that you'd want the tick frozen. Even if you are going to
> > just call the deepest freezable idle state in your freeze_function,
> > you don't want to keep getting woken up as this costs some power usage
>
> As I said, that's a separate issue from these bindings.
I kind of see that coupled with the determination that a idle state supports
freeze. Or are you wanting to have something else that decides whether or not
to configure the enter_freeze?
> > > (2) If I understand your code correctly you have to set the suspend
> > > ops hook to make sure suspend-to-idle is enabled. This is a core
> > > code issue rather than anything else, given that suspend-to-idle
> > > (hey it is based on CPUidle !) does NOT rely on suspend ops to
> > > function.
> >
> > It only requires having suspend_ops and a valid function. Otherwise
> > you can never suspend and never exercise the freeze portion of the
> > cpuidle code.
>
> As I said, *currently* you have to call suspend_set_ops() to set
> up the pm_states label for PM_SUSPEND_FREEZE, suspend_ops are
> irrelevant to make suspend-to-idle work and as I said that's
> related to core code rather than platform specific suspend ops.
>
> We should solve issues, not work around them.
I wholeheartedly agree. I was just being more precise in my answer.
>
> > > So the gist is: as far as I am concerned you do not need any of this
> > > code to enable (yes you need PSCI idle states but no
> > > qcom,idle-state-spc compatible string whatsoever) suspend-to-idle
> > > on ARM64 on top of PSCI, let me know what I am missing.
> >
> > If we had the facilities in the arm cpuidle driver then for 64 bit
> > processors, I wouldn't have to do anything except provision my
> > suspend_ops + valid function. For 32 bit, we actually already use
> > this compat tag and I just have to add code in the spm driver (qcom
> > cpuidle) to init the suspend_ops.
>
> For 64-bit you do not have to have add any facility.
>
> 1) we should change core code to make PM_SUSPEND_FREEZE independent
> of suspend_set_ops()
So then, if cpuidle is active and you have idle states supporting freeze, then
you can always implicitly freeze the system. This would infer then that the
system will always indicate it can do freeze. I am good with that.
For now, we can get away with just implementing the changes you are suggesting.
> 2) we should define which idle states are freezable (99% of them are
> minus coupled idle states), through generic bindings
This would be in the form of a DT property? And given this property then we'd
just assign a enter_freeze() function that calls the enter() for that state. Or
would we have something else that we need to key off of to decide to configure
the enter_freeze?
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 17:27 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 17:27 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10, 2016 at 06:06:56PM +0100, Lorenzo Pieralisi wrote:
> On Fri, Jun 10, 2016 at 11:52:48AM -0500, Andy Gross wrote:
>
> [...]
>
> > > (1) enter_freeze() hooks are not strictly necessary to enable
> > > suspend-to-idle (they are if we want the tick to be frozen
> > > on suspend-to-idle, which is different)
> >
> > I'd think that you'd want the tick frozen. Even if you are going to
> > just call the deepest freezable idle state in your freeze_function,
> > you don't want to keep getting woken up as this costs some power usage
>
> As I said, that's a separate issue from these bindings.
I kind of see that coupled with the determination that a idle state supports
freeze. Or are you wanting to have something else that decides whether or not
to configure the enter_freeze?
> > > (2) If I understand your code correctly you have to set the suspend
> > > ops hook to make sure suspend-to-idle is enabled. This is a core
> > > code issue rather than anything else, given that suspend-to-idle
> > > (hey it is based on CPUidle !) does NOT rely on suspend ops to
> > > function.
> >
> > It only requires having suspend_ops and a valid function. Otherwise
> > you can never suspend and never exercise the freeze portion of the
> > cpuidle code.
>
> As I said, *currently* you have to call suspend_set_ops() to set
> up the pm_states label for PM_SUSPEND_FREEZE, suspend_ops are
> irrelevant to make suspend-to-idle work and as I said that's
> related to core code rather than platform specific suspend ops.
>
> We should solve issues, not work around them.
I wholeheartedly agree. I was just being more precise in my answer.
>
> > > So the gist is: as far as I am concerned you do not need any of this
> > > code to enable (yes you need PSCI idle states but no
> > > qcom,idle-state-spc compatible string whatsoever) suspend-to-idle
> > > on ARM64 on top of PSCI, let me know what I am missing.
> >
> > If we had the facilities in the arm cpuidle driver then for 64 bit
> > processors, I wouldn't have to do anything except provision my
> > suspend_ops + valid function. For 32 bit, we actually already use
> > this compat tag and I just have to add code in the spm driver (qcom
> > cpuidle) to init the suspend_ops.
>
> For 64-bit you do not have to have add any facility.
>
> 1) we should change core code to make PM_SUSPEND_FREEZE independent
> of suspend_set_ops()
So then, if cpuidle is active and you have idle states supporting freeze, then
you can always implicitly freeze the system. This would infer then that the
system will always indicate it can do freeze. I am good with that.
For now, we can get away with just implementing the changes you are suggesting.
> 2) we should define which idle states are freezable (99% of them are
> minus coupled idle states), through generic bindings
This would be in the form of a DT property? And given this property then we'd
just assign a enter_freeze() function that calls the enter() for that state. Or
would we have something else that we need to key off of to decide to configure
the enter_freeze?
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 2/5] arm: defconfig: Enable PM8941 pwr key
2016-05-19 5:00 ` Andy Gross
@ 2016-06-10 20:25 ` Bjorn Andersson
-1 siblings, 0 replies; 62+ messages in thread
From: Bjorn Andersson @ 2016-06-10 20:25 UTC (permalink / raw)
To: Andy Gross; +Cc: linux-pm, linux-arm-msm, stanimir.varbanov, linux-arm-kernel
On Wed 18 May 22:00 PDT 2016, Andy Gross wrote:
> This patch enables the PM8941 pwr key driver.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> arch/arm64/configs/defconfig | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 8917150..d4c539f 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -142,6 +142,8 @@ CONFIG_WL18XX=m
> CONFIG_WLCORE_SDIO=m
> CONFIG_INPUT_EVDEV=y
> CONFIG_KEYBOARD_GPIO=y
> +CONFIG_INPUT_MISC=y
> +CONFIG_INPUT_PM8941_PWRKEY=y
> # CONFIG_SERIO_SERPORT is not set
> CONFIG_SERIO_AMBAKMI=y
> CONFIG_LEGACY_PTY_COUNT=16
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 2/5] arm: defconfig: Enable PM8941 pwr key
@ 2016-06-10 20:25 ` Bjorn Andersson
0 siblings, 0 replies; 62+ messages in thread
From: Bjorn Andersson @ 2016-06-10 20:25 UTC (permalink / raw)
To: linux-arm-kernel
On Wed 18 May 22:00 PDT 2016, Andy Gross wrote:
> This patch enables the PM8941 pwr key driver.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> arch/arm64/configs/defconfig | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 8917150..d4c539f 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -142,6 +142,8 @@ CONFIG_WL18XX=m
> CONFIG_WLCORE_SDIO=m
> CONFIG_INPUT_EVDEV=y
> CONFIG_KEYBOARD_GPIO=y
> +CONFIG_INPUT_MISC=y
> +CONFIG_INPUT_PM8941_PWRKEY=y
> # CONFIG_SERIO_SERPORT is not set
> CONFIG_SERIO_AMBAKMI=y
> CONFIG_LEGACY_PTY_COUNT=16
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 4/5] ARM: dts: qcom: Remove size elements from pmic reg
2016-05-19 5:00 ` Andy Gross
@ 2016-06-10 20:26 ` Bjorn Andersson
-1 siblings, 0 replies; 62+ messages in thread
From: Bjorn Andersson @ 2016-06-10 20:26 UTC (permalink / raw)
To: Andy Gross; +Cc: linux-pm, linux-arm-msm, stanimir.varbanov, linux-arm-kernel
On Wed 18 May 22:00 PDT 2016, Andy Gross wrote:
> The #size-cells for the pmics are 0, but we specify a size in the
> reg property so that MPP and GPIO modules can figure out how many
> pins there are. Now that we've done that by counting irqs, we can
> remove the size elements in the reg properties and be DT
> compliant.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> arch/arm/boot/dts/qcom-pma8084.dtsi | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/boot/dts/qcom-pma8084.dtsi b/arch/arm/boot/dts/qcom-pma8084.dtsi
> index 4e9bd3f..ea7fce3 100644
> --- a/arch/arm/boot/dts/qcom-pma8084.dtsi
> +++ b/arch/arm/boot/dts/qcom-pma8084.dtsi
> @@ -12,15 +12,15 @@
>
> rtc@6000 {
> compatible = "qcom,pm8941-rtc";
> - reg = <0x6000 0x100>,
> - <0x6100 0x100>;
> + reg = <0x6000>,
> + <0x6100>;
> reg-names = "rtc", "alarm";
> interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
> };
>
> pma8084_gpios: gpios@c000 {
> compatible = "qcom,pma8084-gpio", "qcom,spmi-gpio";
> - reg = <0xc000 0x1600>;
> + reg = <0xc000>;
> gpio-controller;
> #gpio-cells = <2>;
> interrupts = <0 0xc0 0 IRQ_TYPE_NONE>,
> @@ -49,7 +49,7 @@
>
> pma8084_mpps: mpps@a000 {
> compatible = "qcom,pma8084-mpp", "qcom,spmi-mpp";
> - reg = <0xa000 0x800>;
> + reg = <0xa000>;
> gpio-controller;
> #gpio-cells = <2>;
> interrupts = <0 0xa0 0 IRQ_TYPE_NONE>,
> @@ -64,7 +64,7 @@
>
> pma8084_temp: temp-alarm@2400 {
> compatible = "qcom,spmi-temp-alarm";
> - reg = <0x2400 0x100>;
> + reg = <0x2400>;
> interrupts = <0 0x24 0 IRQ_TYPE_EDGE_RISING>;
> #thermal-sensor-cells = <0>;
> io-channels = <&pma8084_vadc VADC_DIE_TEMP>;
> @@ -73,7 +73,7 @@
>
> pma8084_vadc: vadc@3100 {
> compatible = "qcom,spmi-vadc";
> - reg = <0x3100 0x100>;
> + reg = <0x3100>;
> interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
> #address-cells = <1>;
> #size-cells = <0>;
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 4/5] ARM: dts: qcom: Remove size elements from pmic reg
@ 2016-06-10 20:26 ` Bjorn Andersson
0 siblings, 0 replies; 62+ messages in thread
From: Bjorn Andersson @ 2016-06-10 20:26 UTC (permalink / raw)
To: linux-arm-kernel
On Wed 18 May 22:00 PDT 2016, Andy Gross wrote:
> The #size-cells for the pmics are 0, but we specify a size in the
> reg property so that MPP and GPIO modules can figure out how many
> pins there are. Now that we've done that by counting irqs, we can
> remove the size elements in the reg properties and be DT
> compliant.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> arch/arm/boot/dts/qcom-pma8084.dtsi | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/boot/dts/qcom-pma8084.dtsi b/arch/arm/boot/dts/qcom-pma8084.dtsi
> index 4e9bd3f..ea7fce3 100644
> --- a/arch/arm/boot/dts/qcom-pma8084.dtsi
> +++ b/arch/arm/boot/dts/qcom-pma8084.dtsi
> @@ -12,15 +12,15 @@
>
> rtc at 6000 {
> compatible = "qcom,pm8941-rtc";
> - reg = <0x6000 0x100>,
> - <0x6100 0x100>;
> + reg = <0x6000>,
> + <0x6100>;
> reg-names = "rtc", "alarm";
> interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
> };
>
> pma8084_gpios: gpios at c000 {
> compatible = "qcom,pma8084-gpio", "qcom,spmi-gpio";
> - reg = <0xc000 0x1600>;
> + reg = <0xc000>;
> gpio-controller;
> #gpio-cells = <2>;
> interrupts = <0 0xc0 0 IRQ_TYPE_NONE>,
> @@ -49,7 +49,7 @@
>
> pma8084_mpps: mpps at a000 {
> compatible = "qcom,pma8084-mpp", "qcom,spmi-mpp";
> - reg = <0xa000 0x800>;
> + reg = <0xa000>;
> gpio-controller;
> #gpio-cells = <2>;
> interrupts = <0 0xa0 0 IRQ_TYPE_NONE>,
> @@ -64,7 +64,7 @@
>
> pma8084_temp: temp-alarm at 2400 {
> compatible = "qcom,spmi-temp-alarm";
> - reg = <0x2400 0x100>;
> + reg = <0x2400>;
> interrupts = <0 0x24 0 IRQ_TYPE_EDGE_RISING>;
> #thermal-sensor-cells = <0>;
> io-channels = <&pma8084_vadc VADC_DIE_TEMP>;
> @@ -73,7 +73,7 @@
>
> pma8084_vadc: vadc at 3100 {
> compatible = "qcom,spmi-vadc";
> - reg = <0x3100 0x100>;
> + reg = <0x3100>;
> interrupts = <0x0 0x31 0x0 IRQ_TYPE_EDGE_RISING>;
> #address-cells = <1>;
> #size-cells = <0>;
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 5/5] ARM: dts: qcom: pma8084: Add pwrkey entry
2016-05-19 5:00 ` Andy Gross
@ 2016-06-10 20:29 ` Bjorn Andersson
-1 siblings, 0 replies; 62+ messages in thread
From: Bjorn Andersson @ 2016-06-10 20:29 UTC (permalink / raw)
To: Andy Gross; +Cc: linux-pm, linux-arm-msm, stanimir.varbanov, linux-arm-kernel
On Wed 18 May 22:00 PDT 2016, Andy Gross wrote:
> This patch adds the power key device tree node.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> arch/arm/boot/dts/qcom-pma8084.dtsi | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm/boot/dts/qcom-pma8084.dtsi b/arch/arm/boot/dts/qcom-pma8084.dtsi
> index ea7fce3..82d2580 100644
> --- a/arch/arm/boot/dts/qcom-pma8084.dtsi
> +++ b/arch/arm/boot/dts/qcom-pma8084.dtsi
> @@ -18,6 +18,14 @@
> interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
> };
>
> + pwrkey@800 {
> + compatible = "qcom,pm8941-pwrkey";
> + reg = <0x800>;
> + interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
> + debounce = <15625>;
> + bias-pull-up;
> + };
> +
> pma8084_gpios: gpios@c000 {
> compatible = "qcom,pma8084-gpio", "qcom,spmi-gpio";
> reg = <0xc000>;
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 5/5] ARM: dts: qcom: pma8084: Add pwrkey entry
@ 2016-06-10 20:29 ` Bjorn Andersson
0 siblings, 0 replies; 62+ messages in thread
From: Bjorn Andersson @ 2016-06-10 20:29 UTC (permalink / raw)
To: linux-arm-kernel
On Wed 18 May 22:00 PDT 2016, Andy Gross wrote:
> This patch adds the power key device tree node.
>
> Signed-off-by: Andy Gross <andy.gross@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> arch/arm/boot/dts/qcom-pma8084.dtsi | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm/boot/dts/qcom-pma8084.dtsi b/arch/arm/boot/dts/qcom-pma8084.dtsi
> index ea7fce3..82d2580 100644
> --- a/arch/arm/boot/dts/qcom-pma8084.dtsi
> +++ b/arch/arm/boot/dts/qcom-pma8084.dtsi
> @@ -18,6 +18,14 @@
> interrupts = <0x0 0x61 0x1 IRQ_TYPE_EDGE_RISING>;
> };
>
> + pwrkey at 800 {
> + compatible = "qcom,pm8941-pwrkey";
> + reg = <0x800>;
> + interrupts = <0x0 0x8 0 IRQ_TYPE_EDGE_BOTH>;
> + debounce = <15625>;
> + bias-pull-up;
> + };
> +
> pma8084_gpios: gpios at c000 {
> compatible = "qcom,pma8084-gpio", "qcom,spmi-gpio";
> reg = <0xc000>;
> --
> 1.9.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 17:27 ` Andy Gross
@ 2016-06-10 20:59 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 62+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-10 20:59 UTC (permalink / raw)
To: Andy Gross
Cc: Mark Rutland, linux-pm, linux-arm-msm, linux-arm-kernel,
stanimir.varbanov
On Fri, Jun 10, 2016 at 12:27:16PM -0500, Andy Gross wrote:
> On Fri, Jun 10, 2016 at 06:06:56PM +0100, Lorenzo Pieralisi wrote:
> > On Fri, Jun 10, 2016 at 11:52:48AM -0500, Andy Gross wrote:
> >
> > [...]
> >
> > > > (1) enter_freeze() hooks are not strictly necessary to enable
> > > > suspend-to-idle (they are if we want the tick to be frozen
> > > > on suspend-to-idle, which is different)
> > >
> > > I'd think that you'd want the tick frozen. Even if you are going to
> > > just call the deepest freezable idle state in your freeze_function,
> > > you don't want to keep getting woken up as this costs some power usage
> >
> > As I said, that's a separate issue from these bindings.
>
> I kind of see that coupled with the determination that a idle state
> supports freeze. Or are you wanting to have something else that
> decides whether or not to configure the enter_freeze?
All PSCI based idle state support freeze, as long as most of the
other idle routines for ARM 32-bit, I do not even think we should
bother with defining DT bindings for it.
[...]
> > For 64-bit you do not have to have add any facility.
> >
> > 1) we should change core code to make PM_SUSPEND_FREEZE independent
> > of suspend_set_ops()
>
> So then, if cpuidle is active and you have idle states supporting
> freeze, then you can always implicitly freeze the system. This would
> infer then that the system will always indicate it can do freeze. I
> am good with that.
>
> For now, we can get away with just implementing the changes you are
> suggesting.
Cracking.
> > 2) we should define which idle states are freezable (99% of them are
> > minus coupled idle states), through generic bindings
>
> This would be in the form of a DT property? And given this property
> then we'd just assign a enter_freeze() function that calls the enter()
> for that state. Or would we have something else that we need to key
> off of to decide to configure the enter_freeze?
See above, I will give it more thought. I take an action to change
core code as per discussion above.
Thanks for raising the point.
Lorenzo
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 20:59 ` Lorenzo Pieralisi
0 siblings, 0 replies; 62+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-10 20:59 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10, 2016 at 12:27:16PM -0500, Andy Gross wrote:
> On Fri, Jun 10, 2016 at 06:06:56PM +0100, Lorenzo Pieralisi wrote:
> > On Fri, Jun 10, 2016 at 11:52:48AM -0500, Andy Gross wrote:
> >
> > [...]
> >
> > > > (1) enter_freeze() hooks are not strictly necessary to enable
> > > > suspend-to-idle (they are if we want the tick to be frozen
> > > > on suspend-to-idle, which is different)
> > >
> > > I'd think that you'd want the tick frozen. Even if you are going to
> > > just call the deepest freezable idle state in your freeze_function,
> > > you don't want to keep getting woken up as this costs some power usage
> >
> > As I said, that's a separate issue from these bindings.
>
> I kind of see that coupled with the determination that a idle state
> supports freeze. Or are you wanting to have something else that
> decides whether or not to configure the enter_freeze?
All PSCI based idle state support freeze, as long as most of the
other idle routines for ARM 32-bit, I do not even think we should
bother with defining DT bindings for it.
[...]
> > For 64-bit you do not have to have add any facility.
> >
> > 1) we should change core code to make PM_SUSPEND_FREEZE independent
> > of suspend_set_ops()
>
> So then, if cpuidle is active and you have idle states supporting
> freeze, then you can always implicitly freeze the system. This would
> infer then that the system will always indicate it can do freeze. I
> am good with that.
>
> For now, we can get away with just implementing the changes you are
> suggesting.
Cracking.
> > 2) we should define which idle states are freezable (99% of them are
> > minus coupled idle states), through generic bindings
>
> This would be in the form of a DT property? And given this property
> then we'd just assign a enter_freeze() function that calls the enter()
> for that state. Or would we have something else that we need to key
> off of to decide to configure the enter_freeze?
See above, I will give it more thought. I take an action to change
core code as per discussion above.
Thanks for raising the point.
Lorenzo
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 16:47 ` Andy Gross
@ 2016-06-10 21:16 ` Lina Iyer
-1 siblings, 0 replies; 62+ messages in thread
From: Lina Iyer @ 2016-06-10 21:16 UTC (permalink / raw)
To: Andy Gross
Cc: Mark Rutland, linux-pm, linux-arm-msm, linux-arm-kernel,
stanimir.varbanov, lorenzo.pieralisi
On Fri, Jun 10 2016 at 10:47 -0600, Andy Gross wrote:
>On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
>> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
>> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
>> > > [+ Lorenzo]
>> > >
>> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
<...>
>>
>> > I chose to do this outside of the arm cpuidle driver because I didn't want to
>> > add any more DT information aside from the compatible, and I needed a separate
>> > place for the Qualcomm specific suspend code.
>>
>> Which suspend code is Qualcomm-specific? There shouldn't be anything on
>> the PSCI side, so I can only imagine that device management is left.
>> What am I missing?
>
>Qualcomm won't be supporting the psci suspend feature and will be doing their
>own thing. As such, they need their own suspend ops. In addition, we have
>platforms that don't use psci (32 bit).
>
Andy, which PSCI suspend feature are you referring to, here? On 8916, we
do support CPU_SUSPEND in PSCI.
You don't want to call SYSTEM_SUSPEND, as it requires the other CPUs to
be CPU_OFF (hotplugged off) before the last core calls this PSCI
function.
Lina
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 21:16 ` Lina Iyer
0 siblings, 0 replies; 62+ messages in thread
From: Lina Iyer @ 2016-06-10 21:16 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10 2016 at 10:47 -0600, Andy Gross wrote:
>On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
>> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
>> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
>> > > [+ Lorenzo]
>> > >
>> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
<...>
>>
>> > I chose to do this outside of the arm cpuidle driver because I didn't want to
>> > add any more DT information aside from the compatible, and I needed a separate
>> > place for the Qualcomm specific suspend code.
>>
>> Which suspend code is Qualcomm-specific? There shouldn't be anything on
>> the PSCI side, so I can only imagine that device management is left.
>> What am I missing?
>
>Qualcomm won't be supporting the psci suspend feature and will be doing their
>own thing. As such, they need their own suspend ops. In addition, we have
>platforms that don't use psci (32 bit).
>
Andy, which PSCI suspend feature are you referring to, here? On 8916, we
do support CPU_SUSPEND in PSCI.
You don't want to call SYSTEM_SUSPEND, as it requires the other CPUs to
be CPU_OFF (hotplugged off) before the last core calls this PSCI
function.
Lina
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 21:16 ` Lina Iyer
@ 2016-06-10 21:52 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 21:52 UTC (permalink / raw)
To: Lina Iyer
Cc: Mark Rutland, Lorenzo Pieralisi, linux-pm, linux-arm-msm,
Stanimir Varbanov, linux-arm-kernel
On 10 June 2016 at 16:16, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Fri, Jun 10 2016 at 10:47 -0600, Andy Gross wrote:
>>
>> On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
>>>
>>> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
>>> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
>>> > > [+ Lorenzo]
>>> > >
>>> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
>
> <...>
>>>
>>>
>>> > I chose to do this outside of the arm cpuidle driver because I didn't
>>> > want to
>>> > add any more DT information aside from the compatible, and I needed a
>>> > separate
>>> > place for the Qualcomm specific suspend code.
>>>
>>> Which suspend code is Qualcomm-specific? There shouldn't be anything on
>>> the PSCI side, so I can only imagine that device management is left.
>>> What am I missing?
>>
>>
>
>> Qualcomm won't be supporting the psci suspend feature and will be doing
>> their
>> own thing. As such, they need their own suspend ops. In addition, we
>> have
>> platforms that don't use psci (32 bit).
>>
> Andy, which PSCI suspend feature are you referring to, here? On 8916, we
> do support CPU_SUSPEND in PSCI.
>
> You don't want to call SYSTEM_SUSPEND, as it requires the other CPUs to
> be CPU_OFF (hotplugged off) before the last core calls this PSCI
> function.
I was referring to the SYSTEM_SUSPEND psci feature. It was my
understanding that wouldn't be supported on the Qualcomm platforms.
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-10 21:52 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-10 21:52 UTC (permalink / raw)
To: linux-arm-kernel
On 10 June 2016 at 16:16, Lina Iyer <lina.iyer@linaro.org> wrote:
> On Fri, Jun 10 2016 at 10:47 -0600, Andy Gross wrote:
>>
>> On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
>>>
>>> On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
>>> > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
>>> > > [+ Lorenzo]
>>> > >
>>> > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
>
> <...>
>>>
>>>
>>> > I chose to do this outside of the arm cpuidle driver because I didn't
>>> > want to
>>> > add any more DT information aside from the compatible, and I needed a
>>> > separate
>>> > place for the Qualcomm specific suspend code.
>>>
>>> Which suspend code is Qualcomm-specific? There shouldn't be anything on
>>> the PSCI side, so I can only imagine that device management is left.
>>> What am I missing?
>>
>>
>
>> Qualcomm won't be supporting the psci suspend feature and will be doing
>> their
>> own thing. As such, they need their own suspend ops. In addition, we
>> have
>> platforms that don't use psci (32 bit).
>>
> Andy, which PSCI suspend feature are you referring to, here? On 8916, we
> do support CPU_SUSPEND in PSCI.
>
> You don't want to call SYSTEM_SUSPEND, as it requires the other CPUs to
> be CPU_OFF (hotplugged off) before the last core calls this PSCI
> function.
I was referring to the SYSTEM_SUSPEND psci feature. It was my
understanding that wouldn't be supported on the Qualcomm platforms.
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-10 16:47 ` Andy Gross
@ 2016-06-13 11:00 ` Mark Rutland
-1 siblings, 0 replies; 62+ messages in thread
From: Mark Rutland @ 2016-06-13 11:00 UTC (permalink / raw)
To: Andy Gross
Cc: linux-pm, linux-arm-msm, linux-arm-kernel, stanimir.varbanov,
lorenzo.pieralisi
On Fri, Jun 10, 2016 at 11:47:21AM -0500, Andy Gross wrote:
> On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
> > On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> > > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > > > [+ Lorenzo]
> > > >
> > > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > > > state. This compatible indicates that the state is one which supports
> > > > > freeze.
> > > > >
> > > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > > > ---
> > > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > index 208af00..032e411 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > @@ -104,7 +104,7 @@
> > > > >
> > > > > idle-states {
> > > > > CPU_SPC: spc {
> > > > > - compatible = "arm,idle-state";
> > > > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > > > arm,psci-suspend-param = <0x40000002>;
> > > > > entry-latency-us = <130>;
> > > > > exit-latency-us = <150>;
> > > >
> > > > This looks suspicious.
> > > >
> > > > This is a PSCI idle state, and we have a PSCI driver driven by the
> > > > generic ARM cpuidle driver.
> > > >
> > > > Why do we need a qcom-specific compatible here?
> > > >
> > > > Surely we should be able to use the idle code in a generic fashion to
> > > > driver suspend-to-idle?
> > >
> > > We need a way to identify specific idle states that support suspend-to-idle. In
> > > addition, when we have identified the states, we may have to configure the
> > > enter_freeze() function.
> >
> > Could you elaborate on what you mean by a state supporting
> > suspend-to-idle? It was my understanding that any idle state should
> > function for suspend-to-idle (and the choice of state is potentially
> > subjective).
>
> when you freeze the system, cpuidle will try to find the deepest state which
> supports freeze (by checking if a enter_freeze() exists). If it does exist,
> then the tick is frozen and the enter_freeze is called as each cpu goes idle.
Per Lorenzo's replies in another thread, it sounds like this is a
generic issue, and not specific to Qualcomm. My understanding is that
the only issue is coupled idle states, and further, that issue is really
an implementation detail within Linux w.r.t. IRQ management.
So it sounds like we need to rework things to be robust in the case of
coupled idle states, and we can wire up enter_freeze for all states
generically.
If there is some peroblem with making things robust, I assume we can
identify coupled idle states today in some generic manner. I don't
currently see the need for any DT binding.
Lorenzo, does the above make sense to you?
> > > I chose to do this outside of the arm cpuidle driver because I didn't want to
> > > add any more DT information aside from the compatible, and I needed a separate
> > > place for the Qualcomm specific suspend code.
> >
> > Which suspend code is Qualcomm-specific? There shouldn't be anything on
> > the PSCI side, so I can only imagine that device management is left.
> > What am I missing?
>
> Qualcomm won't be supporting the psci suspend feature and will be doing their
> own thing. As such, they need their own suspend ops. In addition, we have
> platforms that don't use psci (32 bit).
That's orthogonal to suspend-to-idle, which can be handled in a generic
fashion. It's also orthogonal to 32-bit !PSCI platforms, as my
complaint was regarding the use of this with PSCI.
> > > With the compatible, this makes
> > > my 32 and 64 bit processor suspend code identical, as we have our own cpuidle
> > > driver for the 32 bit procs.
> > >
> > > An alternative would be to add some facilities to communicate this to the arm
> > > cpuidle driver and configure the enter_freeze() function at some later point.
> >
> > I would prefer that suspend-to-idle using PSCI occurred via the generic
> > PSCI and cpuidle code. I am happy to extend that code if there is
> > something lacking, and I would prefer to not have platform-specific
> > hooks.
>
> Right, I briefly looked at that in the beginning and decided against it. But if
> we can at least add some way to identify freezeable idle states and configure
> the freeze function, that'd cover all the requirements.
Ok, per the above I believe that is covered.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-13 11:00 ` Mark Rutland
0 siblings, 0 replies; 62+ messages in thread
From: Mark Rutland @ 2016-06-13 11:00 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jun 10, 2016 at 11:47:21AM -0500, Andy Gross wrote:
> On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
> > On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> > > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > > > [+ Lorenzo]
> > > >
> > > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > > > state. This compatible indicates that the state is one which supports
> > > > > freeze.
> > > > >
> > > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > > > ---
> > > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > index 208af00..032e411 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > @@ -104,7 +104,7 @@
> > > > >
> > > > > idle-states {
> > > > > CPU_SPC: spc {
> > > > > - compatible = "arm,idle-state";
> > > > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > > > arm,psci-suspend-param = <0x40000002>;
> > > > > entry-latency-us = <130>;
> > > > > exit-latency-us = <150>;
> > > >
> > > > This looks suspicious.
> > > >
> > > > This is a PSCI idle state, and we have a PSCI driver driven by the
> > > > generic ARM cpuidle driver.
> > > >
> > > > Why do we need a qcom-specific compatible here?
> > > >
> > > > Surely we should be able to use the idle code in a generic fashion to
> > > > driver suspend-to-idle?
> > >
> > > We need a way to identify specific idle states that support suspend-to-idle. In
> > > addition, when we have identified the states, we may have to configure the
> > > enter_freeze() function.
> >
> > Could you elaborate on what you mean by a state supporting
> > suspend-to-idle? It was my understanding that any idle state should
> > function for suspend-to-idle (and the choice of state is potentially
> > subjective).
>
> when you freeze the system, cpuidle will try to find the deepest state which
> supports freeze (by checking if a enter_freeze() exists). If it does exist,
> then the tick is frozen and the enter_freeze is called as each cpu goes idle.
Per Lorenzo's replies in another thread, it sounds like this is a
generic issue, and not specific to Qualcomm. My understanding is that
the only issue is coupled idle states, and further, that issue is really
an implementation detail within Linux w.r.t. IRQ management.
So it sounds like we need to rework things to be robust in the case of
coupled idle states, and we can wire up enter_freeze for all states
generically.
If there is some peroblem with making things robust, I assume we can
identify coupled idle states today in some generic manner. I don't
currently see the need for any DT binding.
Lorenzo, does the above make sense to you?
> > > I chose to do this outside of the arm cpuidle driver because I didn't want to
> > > add any more DT information aside from the compatible, and I needed a separate
> > > place for the Qualcomm specific suspend code.
> >
> > Which suspend code is Qualcomm-specific? There shouldn't be anything on
> > the PSCI side, so I can only imagine that device management is left.
> > What am I missing?
>
> Qualcomm won't be supporting the psci suspend feature and will be doing their
> own thing. As such, they need their own suspend ops. In addition, we have
> platforms that don't use psci (32 bit).
That's orthogonal to suspend-to-idle, which can be handled in a generic
fashion. It's also orthogonal to 32-bit !PSCI platforms, as my
complaint was regarding the use of this with PSCI.
> > > With the compatible, this makes
> > > my 32 and 64 bit processor suspend code identical, as we have our own cpuidle
> > > driver for the 32 bit procs.
> > >
> > > An alternative would be to add some facilities to communicate this to the arm
> > > cpuidle driver and configure the enter_freeze() function at some later point.
> >
> > I would prefer that suspend-to-idle using PSCI occurred via the generic
> > PSCI and cpuidle code. I am happy to extend that code if there is
> > something lacking, and I would prefer to not have platform-specific
> > hooks.
>
> Right, I briefly looked at that in the beginning and decided against it. But if
> we can at least add some way to identify freezeable idle states and configure
> the freeze function, that'd cover all the requirements.
Ok, per the above I believe that is covered.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-13 11:00 ` Mark Rutland
@ 2016-06-13 13:48 ` Lorenzo Pieralisi
-1 siblings, 0 replies; 62+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-13 13:48 UTC (permalink / raw)
To: Mark Rutland
Cc: Andy Gross, linux-pm, linux-arm-msm, linux-arm-kernel, stanimir.varbanov
On Mon, Jun 13, 2016 at 12:00:43PM +0100, Mark Rutland wrote:
> On Fri, Jun 10, 2016 at 11:47:21AM -0500, Andy Gross wrote:
> > On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
> > > On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> > > > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > > > > [+ Lorenzo]
> > > > >
> > > > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > > > > state. This compatible indicates that the state is one which supports
> > > > > > freeze.
> > > > > >
> > > > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > > > > ---
> > > > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > index 208af00..032e411 100644
> > > > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > @@ -104,7 +104,7 @@
> > > > > >
> > > > > > idle-states {
> > > > > > CPU_SPC: spc {
> > > > > > - compatible = "arm,idle-state";
> > > > > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > > > > arm,psci-suspend-param = <0x40000002>;
> > > > > > entry-latency-us = <130>;
> > > > > > exit-latency-us = <150>;
> > > > >
> > > > > This looks suspicious.
> > > > >
> > > > > This is a PSCI idle state, and we have a PSCI driver driven by the
> > > > > generic ARM cpuidle driver.
> > > > >
> > > > > Why do we need a qcom-specific compatible here?
> > > > >
> > > > > Surely we should be able to use the idle code in a generic fashion to
> > > > > driver suspend-to-idle?
> > > >
> > > > We need a way to identify specific idle states that support suspend-to-idle. In
> > > > addition, when we have identified the states, we may have to configure the
> > > > enter_freeze() function.
> > >
> > > Could you elaborate on what you mean by a state supporting
> > > suspend-to-idle? It was my understanding that any idle state should
> > > function for suspend-to-idle (and the choice of state is potentially
> > > subjective).
> >
> > when you freeze the system, cpuidle will try to find the deepest state which
> > supports freeze (by checking if a enter_freeze() exists). If it does exist,
> > then the tick is frozen and the enter_freeze is called as each cpu goes idle.
>
> Per Lorenzo's replies in another thread, it sounds like this is a
> generic issue, and not specific to Qualcomm. My understanding is that
> the only issue is coupled idle states, and further, that issue is really
> an implementation detail within Linux w.r.t. IRQ management.
>
> So it sounds like we need to rework things to be robust in the case of
> coupled idle states, and we can wire up enter_freeze for all states
> generically.
>
> If there is some peroblem with making things robust, I assume we can
> identify coupled idle states today in some generic manner. I don't
> currently see the need for any DT binding.
>
> Lorenzo, does the above make sense to you?
It does but I have a concern, let me summarize the problem here.
An idle state is freezeable if it can be entered with a function
that does not enable IRQs and that's a kernel implementation
detail; that function is used to initialize its enter_freeze()
hook.
That's the case for all ARM platforms I am aware of, apart
from the ones relying on coupled C-states that (by construction,
in core CPUidle code) rely on IRQ enabling in their enter
function body to work (NB: I do not handle coupled idle states
in generic ARM CPUidle code, and I will never do so that's a
non-existing problem).
The point is: while initializing the idle states in:
drivers/cpuidle/dt_idle_states.c (init_state_node())
we do not have all necessary information to know if we can
safely initialize the enter_freeze() hook (because
that's a property that depends on the CPUidle back-end).
We could add a DT property to report an idle state as
"freezeable", but that's ugly and ill-defined, it is a kernel
implementation detail, not sure a DT property makes sense
here.
So, either:
(1) We consider all idle states to be "freezeable" and so
I initialize their enter_freeze() pointer == enter()
(and assume the idle back-end does NOT enable IRQs, I
could add some documentation for that)
(2) I have to add code that probes the idle back-end (ie
the arm_cpuidle_suspend() implementation) to detect
which states require IRQs to be enabled.
(2) is ugly/convoluted (and solving a problem that does not exist
at present). I am quite tempted to go for (1) and if we ever
have some issues with that we blacklist the respective platforms
enable-methods in the DT idle parsing code.
Thoughts appreciated.
Lorenzo
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-13 13:48 ` Lorenzo Pieralisi
0 siblings, 0 replies; 62+ messages in thread
From: Lorenzo Pieralisi @ 2016-06-13 13:48 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 13, 2016 at 12:00:43PM +0100, Mark Rutland wrote:
> On Fri, Jun 10, 2016 at 11:47:21AM -0500, Andy Gross wrote:
> > On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
> > > On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> > > > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > > > > [+ Lorenzo]
> > > > >
> > > > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > > > > state. This compatible indicates that the state is one which supports
> > > > > > freeze.
> > > > > >
> > > > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > > > > ---
> > > > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > index 208af00..032e411 100644
> > > > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > @@ -104,7 +104,7 @@
> > > > > >
> > > > > > idle-states {
> > > > > > CPU_SPC: spc {
> > > > > > - compatible = "arm,idle-state";
> > > > > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > > > > arm,psci-suspend-param = <0x40000002>;
> > > > > > entry-latency-us = <130>;
> > > > > > exit-latency-us = <150>;
> > > > >
> > > > > This looks suspicious.
> > > > >
> > > > > This is a PSCI idle state, and we have a PSCI driver driven by the
> > > > > generic ARM cpuidle driver.
> > > > >
> > > > > Why do we need a qcom-specific compatible here?
> > > > >
> > > > > Surely we should be able to use the idle code in a generic fashion to
> > > > > driver suspend-to-idle?
> > > >
> > > > We need a way to identify specific idle states that support suspend-to-idle. In
> > > > addition, when we have identified the states, we may have to configure the
> > > > enter_freeze() function.
> > >
> > > Could you elaborate on what you mean by a state supporting
> > > suspend-to-idle? It was my understanding that any idle state should
> > > function for suspend-to-idle (and the choice of state is potentially
> > > subjective).
> >
> > when you freeze the system, cpuidle will try to find the deepest state which
> > supports freeze (by checking if a enter_freeze() exists). If it does exist,
> > then the tick is frozen and the enter_freeze is called as each cpu goes idle.
>
> Per Lorenzo's replies in another thread, it sounds like this is a
> generic issue, and not specific to Qualcomm. My understanding is that
> the only issue is coupled idle states, and further, that issue is really
> an implementation detail within Linux w.r.t. IRQ management.
>
> So it sounds like we need to rework things to be robust in the case of
> coupled idle states, and we can wire up enter_freeze for all states
> generically.
>
> If there is some peroblem with making things robust, I assume we can
> identify coupled idle states today in some generic manner. I don't
> currently see the need for any DT binding.
>
> Lorenzo, does the above make sense to you?
It does but I have a concern, let me summarize the problem here.
An idle state is freezeable if it can be entered with a function
that does not enable IRQs and that's a kernel implementation
detail; that function is used to initialize its enter_freeze()
hook.
That's the case for all ARM platforms I am aware of, apart
from the ones relying on coupled C-states that (by construction,
in core CPUidle code) rely on IRQ enabling in their enter
function body to work (NB: I do not handle coupled idle states
in generic ARM CPUidle code, and I will never do so that's a
non-existing problem).
The point is: while initializing the idle states in:
drivers/cpuidle/dt_idle_states.c (init_state_node())
we do not have all necessary information to know if we can
safely initialize the enter_freeze() hook (because
that's a property that depends on the CPUidle back-end).
We could add a DT property to report an idle state as
"freezeable", but that's ugly and ill-defined, it is a kernel
implementation detail, not sure a DT property makes sense
here.
So, either:
(1) We consider all idle states to be "freezeable" and so
I initialize their enter_freeze() pointer == enter()
(and assume the idle back-end does NOT enable IRQs, I
could add some documentation for that)
(2) I have to add code that probes the idle back-end (ie
the arm_cpuidle_suspend() implementation) to detect
which states require IRQs to be enabled.
(2) is ugly/convoluted (and solving a problem that does not exist
at present). I am quite tempted to go for (1) and if we ever
have some issues with that we blacklist the respective platforms
enable-methods in the DT idle parsing code.
Thoughts appreciated.
Lorenzo
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 1/5] soc: qcom: Add suspend to idle support
2016-06-09 18:09 ` Andy Gross
@ 2016-06-13 16:12 ` Daniel Lezcano
-1 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2016-06-13 16:12 UTC (permalink / raw)
To: Andy Gross
Cc: Ulf Hansson, linux-pm, linux-arm-msm, linux-arm-kernel,
stanimir.varbanov
On Thu, Jun 09, 2016 at 01:09:02PM -0500, Andy Gross wrote:
> On Thu, Jun 09, 2016 at 09:39:34AM +0200, Ulf Hansson wrote:
> > + Daniel
> >
> > On 19 May 2016 at 07:00, Andy Gross <andy.gross@linaro.org> wrote:
[ ... ]
> > I don't think this will work!
> >
> > When building a multi defconfig for ARM, you might overwrite the
> > suspend_ops (there's only one set) as here you don't know that it's
> > actually the QCOM platform that is running, right!?
> >
> > Perhaps this code actually belongs closer to the cpuidle driver?
>
> Hmmmm, I might have to get creative. I originally had a DT entry for the pm,
> but that doesn't make sense as this is purely a software construct. The db410c
> uses the arm cpuidle driver so I can't really hook it in there. I'll have to
> come up with something else.
IMO, the generic ARM driver should be revisited to add support for suspend
to idle.
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 1/5] soc: qcom: Add suspend to idle support
@ 2016-06-13 16:12 ` Daniel Lezcano
0 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2016-06-13 16:12 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jun 09, 2016 at 01:09:02PM -0500, Andy Gross wrote:
> On Thu, Jun 09, 2016 at 09:39:34AM +0200, Ulf Hansson wrote:
> > + Daniel
> >
> > On 19 May 2016 at 07:00, Andy Gross <andy.gross@linaro.org> wrote:
[ ... ]
> > I don't think this will work!
> >
> > When building a multi defconfig for ARM, you might overwrite the
> > suspend_ops (there's only one set) as here you don't know that it's
> > actually the QCOM platform that is running, right!?
> >
> > Perhaps this code actually belongs closer to the cpuidle driver?
>
> Hmmmm, I might have to get creative. I originally had a DT entry for the pm,
> but that doesn't make sense as this is purely a software construct. The db410c
> uses the arm cpuidle driver so I can't really hook it in there. I'll have to
> come up with something else.
IMO, the generic ARM driver should be revisited to add support for suspend
to idle.
^ permalink raw reply [flat|nested] 62+ messages in thread
* Re: [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
2016-06-13 13:48 ` Lorenzo Pieralisi
@ 2016-06-16 8:12 ` Andy Gross
-1 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-16 8:12 UTC (permalink / raw)
To: Lorenzo Pieralisi
Cc: Mark Rutland, linux-pm, linux-arm-msm, linux-arm-kernel,
stanimir.varbanov
On Mon, Jun 13, 2016 at 02:48:33PM +0100, Lorenzo Pieralisi wrote:
> On Mon, Jun 13, 2016 at 12:00:43PM +0100, Mark Rutland wrote:
> > On Fri, Jun 10, 2016 at 11:47:21AM -0500, Andy Gross wrote:
> > > On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
> > > > On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> > > > > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > > > > > [+ Lorenzo]
> > > > > >
> > > > > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > > > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > > > > > state. This compatible indicates that the state is one which supports
> > > > > > > freeze.
> > > > > > >
> > > > > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > > > > > ---
> > > > > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > > index 208af00..032e411 100644
> > > > > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > > @@ -104,7 +104,7 @@
> > > > > > >
> > > > > > > idle-states {
> > > > > > > CPU_SPC: spc {
> > > > > > > - compatible = "arm,idle-state";
> > > > > > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > > > > > arm,psci-suspend-param = <0x40000002>;
> > > > > > > entry-latency-us = <130>;
> > > > > > > exit-latency-us = <150>;
> > > > > >
> > > > > > This looks suspicious.
> > > > > >
> > > > > > This is a PSCI idle state, and we have a PSCI driver driven by the
> > > > > > generic ARM cpuidle driver.
> > > > > >
> > > > > > Why do we need a qcom-specific compatible here?
> > > > > >
> > > > > > Surely we should be able to use the idle code in a generic fashion to
> > > > > > driver suspend-to-idle?
> > > > >
> > > > > We need a way to identify specific idle states that support suspend-to-idle. In
> > > > > addition, when we have identified the states, we may have to configure the
> > > > > enter_freeze() function.
> > > >
> > > > Could you elaborate on what you mean by a state supporting
> > > > suspend-to-idle? It was my understanding that any idle state should
> > > > function for suspend-to-idle (and the choice of state is potentially
> > > > subjective).
> > >
> > > when you freeze the system, cpuidle will try to find the deepest state which
> > > supports freeze (by checking if a enter_freeze() exists). If it does exist,
> > > then the tick is frozen and the enter_freeze is called as each cpu goes idle.
> >
> > Per Lorenzo's replies in another thread, it sounds like this is a
> > generic issue, and not specific to Qualcomm. My understanding is that
> > the only issue is coupled idle states, and further, that issue is really
> > an implementation detail within Linux w.r.t. IRQ management.
> >
> > So it sounds like we need to rework things to be robust in the case of
> > coupled idle states, and we can wire up enter_freeze for all states
> > generically.
> >
> > If there is some peroblem with making things robust, I assume we can
> > identify coupled idle states today in some generic manner. I don't
> > currently see the need for any DT binding.
> >
> > Lorenzo, does the above make sense to you?
>
> It does but I have a concern, let me summarize the problem here.
>
> An idle state is freezeable if it can be entered with a function
> that does not enable IRQs and that's a kernel implementation
> detail; that function is used to initialize its enter_freeze()
> hook.
>
> That's the case for all ARM platforms I am aware of, apart
> from the ones relying on coupled C-states that (by construction,
> in core CPUidle code) rely on IRQ enabling in their enter
> function body to work (NB: I do not handle coupled idle states
> in generic ARM CPUidle code, and I will never do so that's a
> non-existing problem).
>
> The point is: while initializing the idle states in:
>
> drivers/cpuidle/dt_idle_states.c (init_state_node())
>
> we do not have all necessary information to know if we can
> safely initialize the enter_freeze() hook (because
> that's a property that depends on the CPUidle back-end).
And this is why I was modifying the enter_freeze after the cpuidle driver was
loaded. And also why i was keying off the compat tag.
>
> We could add a DT property to report an idle state as
> "freezeable", but that's ugly and ill-defined, it is a kernel
> implementation detail, not sure a DT property makes sense
> here.
>
> So, either:
>
> (1) We consider all idle states to be "freezeable" and so
> I initialize their enter_freeze() pointer == enter()
> (and assume the idle back-end does NOT enable IRQs, I
> could add some documentation for that)
The only issue at the moment with this is that the function prototypes are
different. That is why I call the .enter function from withing my enter_freeze.
Otherwise I think this would work pretty well.
>
> (2) I have to add code that probes the idle back-end (ie
> the arm_cpuidle_suspend() implementation) to detect
> which states require IRQs to be enabled.
>
> (2) is ugly/convoluted (and solving a problem that does not exist
> at present). I am quite tempted to go for (1) and if we ever
> have some issues with that we blacklist the respective platforms
> enable-methods in the DT idle parsing code.
>
> Thoughts appreciated.
>
> Lorenzo
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
* [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag
@ 2016-06-16 8:12 ` Andy Gross
0 siblings, 0 replies; 62+ messages in thread
From: Andy Gross @ 2016-06-16 8:12 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Jun 13, 2016 at 02:48:33PM +0100, Lorenzo Pieralisi wrote:
> On Mon, Jun 13, 2016 at 12:00:43PM +0100, Mark Rutland wrote:
> > On Fri, Jun 10, 2016 at 11:47:21AM -0500, Andy Gross wrote:
> > > On Fri, Jun 10, 2016 at 05:25:55PM +0100, Mark Rutland wrote:
> > > > On Fri, Jun 10, 2016 at 11:12:34AM -0500, Andy Gross wrote:
> > > > > On Fri, Jun 10, 2016 at 04:48:57PM +0100, Mark Rutland wrote:
> > > > > > [+ Lorenzo]
> > > > > >
> > > > > > On Thu, May 19, 2016 at 12:00:18AM -0500, Andy Gross wrote:
> > > > > > > This patch adds the qcom,idle-state-spc compatible to the SPC idle
> > > > > > > state. This compatible indicates that the state is one which supports
> > > > > > > freeze.
> > > > > > >
> > > > > > > Signed-off-by: Andy Gross <andy.gross@linaro.org>
> > > > > > > ---
> > > > > > > arch/arm64/boot/dts/qcom/msm8916.dtsi | 2 +-
> > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/arch/arm64/boot/dts/qcom/msm8916.dtsi b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > > index 208af00..032e411 100644
> > > > > > > --- a/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > > +++ b/arch/arm64/boot/dts/qcom/msm8916.dtsi
> > > > > > > @@ -104,7 +104,7 @@
> > > > > > >
> > > > > > > idle-states {
> > > > > > > CPU_SPC: spc {
> > > > > > > - compatible = "arm,idle-state";
> > > > > > > + compatible = "qcom,idle-state-spc", "arm,idle-state";
> > > > > > > arm,psci-suspend-param = <0x40000002>;
> > > > > > > entry-latency-us = <130>;
> > > > > > > exit-latency-us = <150>;
> > > > > >
> > > > > > This looks suspicious.
> > > > > >
> > > > > > This is a PSCI idle state, and we have a PSCI driver driven by the
> > > > > > generic ARM cpuidle driver.
> > > > > >
> > > > > > Why do we need a qcom-specific compatible here?
> > > > > >
> > > > > > Surely we should be able to use the idle code in a generic fashion to
> > > > > > driver suspend-to-idle?
> > > > >
> > > > > We need a way to identify specific idle states that support suspend-to-idle. In
> > > > > addition, when we have identified the states, we may have to configure the
> > > > > enter_freeze() function.
> > > >
> > > > Could you elaborate on what you mean by a state supporting
> > > > suspend-to-idle? It was my understanding that any idle state should
> > > > function for suspend-to-idle (and the choice of state is potentially
> > > > subjective).
> > >
> > > when you freeze the system, cpuidle will try to find the deepest state which
> > > supports freeze (by checking if a enter_freeze() exists). If it does exist,
> > > then the tick is frozen and the enter_freeze is called as each cpu goes idle.
> >
> > Per Lorenzo's replies in another thread, it sounds like this is a
> > generic issue, and not specific to Qualcomm. My understanding is that
> > the only issue is coupled idle states, and further, that issue is really
> > an implementation detail within Linux w.r.t. IRQ management.
> >
> > So it sounds like we need to rework things to be robust in the case of
> > coupled idle states, and we can wire up enter_freeze for all states
> > generically.
> >
> > If there is some peroblem with making things robust, I assume we can
> > identify coupled idle states today in some generic manner. I don't
> > currently see the need for any DT binding.
> >
> > Lorenzo, does the above make sense to you?
>
> It does but I have a concern, let me summarize the problem here.
>
> An idle state is freezeable if it can be entered with a function
> that does not enable IRQs and that's a kernel implementation
> detail; that function is used to initialize its enter_freeze()
> hook.
>
> That's the case for all ARM platforms I am aware of, apart
> from the ones relying on coupled C-states that (by construction,
> in core CPUidle code) rely on IRQ enabling in their enter
> function body to work (NB: I do not handle coupled idle states
> in generic ARM CPUidle code, and I will never do so that's a
> non-existing problem).
>
> The point is: while initializing the idle states in:
>
> drivers/cpuidle/dt_idle_states.c (init_state_node())
>
> we do not have all necessary information to know if we can
> safely initialize the enter_freeze() hook (because
> that's a property that depends on the CPUidle back-end).
And this is why I was modifying the enter_freeze after the cpuidle driver was
loaded. And also why i was keying off the compat tag.
>
> We could add a DT property to report an idle state as
> "freezeable", but that's ugly and ill-defined, it is a kernel
> implementation detail, not sure a DT property makes sense
> here.
>
> So, either:
>
> (1) We consider all idle states to be "freezeable" and so
> I initialize their enter_freeze() pointer == enter()
> (and assume the idle back-end does NOT enable IRQs, I
> could add some documentation for that)
The only issue at the moment with this is that the function prototypes are
different. That is why I call the .enter function from withing my enter_freeze.
Otherwise I think this would work pretty well.
>
> (2) I have to add code that probes the idle back-end (ie
> the arm_cpuidle_suspend() implementation) to detect
> which states require IRQs to be enabled.
>
> (2) is ugly/convoluted (and solving a problem that does not exist
> at present). I am quite tempted to go for (1) and if we ever
> have some issues with that we blacklist the respective platforms
> enable-methods in the DT idle parsing code.
>
> Thoughts appreciated.
>
> Lorenzo
Regards,
Andy
^ permalink raw reply [flat|nested] 62+ messages in thread
end of thread, other threads:[~2016-06-16 8:12 UTC | newest]
Thread overview: 62+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-19 5:00 [PATCH 0/5] Qualcomm Suspend to Idle Support Andy Gross
2016-05-19 5:00 ` Andy Gross
2016-05-19 5:00 ` [PATCH 1/5] soc: qcom: Add suspend to idle support Andy Gross
2016-05-19 5:00 ` Andy Gross
2016-06-09 7:39 ` Ulf Hansson
2016-06-09 7:39 ` Ulf Hansson
2016-06-09 18:09 ` Andy Gross
2016-06-09 18:09 ` Andy Gross
2016-06-10 8:47 ` Ulf Hansson
2016-06-10 8:47 ` Ulf Hansson
2016-06-10 15:26 ` Andy Gross
2016-06-10 15:26 ` Andy Gross
2016-06-13 16:12 ` Daniel Lezcano
2016-06-13 16:12 ` Daniel Lezcano
2016-05-19 5:00 ` [PATCH 2/5] arm: defconfig: Enable PM8941 pwr key Andy Gross
2016-05-19 5:00 ` Andy Gross
2016-06-10 20:25 ` Bjorn Andersson
2016-06-10 20:25 ` Bjorn Andersson
2016-05-19 5:00 ` [PATCH 3/5] arm64: dts: msm8916: Add spc compat tag Andy Gross
2016-05-19 5:00 ` Andy Gross
2016-05-19 19:52 ` Stanimir Varbanov
2016-05-19 19:52 ` Stanimir Varbanov
2016-05-19 20:16 ` Andy Gross
2016-05-19 20:16 ` Andy Gross
2016-06-10 15:48 ` Mark Rutland
2016-06-10 15:48 ` Mark Rutland
2016-06-10 16:12 ` Andy Gross
2016-06-10 16:12 ` Andy Gross
2016-06-10 16:25 ` Mark Rutland
2016-06-10 16:25 ` Mark Rutland
2016-06-10 16:47 ` Andy Gross
2016-06-10 16:47 ` Andy Gross
2016-06-10 21:16 ` Lina Iyer
2016-06-10 21:16 ` Lina Iyer
2016-06-10 21:52 ` Andy Gross
2016-06-10 21:52 ` Andy Gross
2016-06-13 11:00 ` Mark Rutland
2016-06-13 11:00 ` Mark Rutland
2016-06-13 13:48 ` Lorenzo Pieralisi
2016-06-13 13:48 ` Lorenzo Pieralisi
2016-06-16 8:12 ` Andy Gross
2016-06-16 8:12 ` Andy Gross
2016-06-10 16:31 ` Lorenzo Pieralisi
2016-06-10 16:31 ` Lorenzo Pieralisi
2016-06-10 16:52 ` Andy Gross
2016-06-10 16:52 ` Andy Gross
2016-06-10 17:06 ` Lorenzo Pieralisi
2016-06-10 17:06 ` Lorenzo Pieralisi
2016-06-10 17:27 ` Andy Gross
2016-06-10 17:27 ` Andy Gross
2016-06-10 20:59 ` Lorenzo Pieralisi
2016-06-10 20:59 ` Lorenzo Pieralisi
2016-05-19 5:00 ` [PATCH 4/5] ARM: dts: qcom: Remove size elements from pmic reg Andy Gross
2016-05-19 5:00 ` Andy Gross
2016-06-10 20:26 ` Bjorn Andersson
2016-06-10 20:26 ` Bjorn Andersson
2016-05-19 5:00 ` [PATCH 5/5] ARM: dts: qcom: pma8084: Add pwrkey entry Andy Gross
2016-05-19 5:00 ` Andy Gross
2016-06-10 20:29 ` Bjorn Andersson
2016-06-10 20:29 ` Bjorn Andersson
2016-05-20 6:09 ` [PATCH 0/5] Qualcomm Suspend to Idle Support Pramod Gurav
2016-05-20 6:09 ` Pramod Gurav
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.