| Message ID | 1421840155-18990-3-git-send-email-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jan 21, 2015 at 11:35:55AM +0000, Sudeep Holla wrote: > PSCI specifications upto v0.2 and the related kernel back-end > implementation lack a method to enter system wide suspend state. > > This patch implements suspend to RAM support for all ARM64 systems > with PSCIv0.2 support using CPU SUSPEND and the new system state DT > bindings. > > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > arch/arm64/kernel/psci.c | 83 ++++++++++++++++++++++++++++++++++++++++++------ > 1 file changed, 74 insertions(+), 9 deletions(-) > > diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c > index f1dbca7d5c96..8be464747dd6 100644 > --- a/arch/arm64/kernel/psci.c > +++ b/arch/arm64/kernel/psci.c > @@ -22,6 +22,7 @@ > #include <linux/pm.h> > #include <linux/delay.h> > #include <linux/slab.h> > +#include <linux/suspend.h> > #include <uapi/linux/psci.h> > > #include <asm/compiler.h> > @@ -304,6 +305,75 @@ static void psci_sys_poweroff(void) > invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); > } > > +static int psci_suspend_finisher(unsigned long arg) > +{ > + return psci_ops.cpu_suspend(*(struct psci_power_state *)arg, > + virt_to_phys(cpu_resume)); > +} > + > +#ifdef CONFIG_SUSPEND > +static struct psci_power_state psci_system_suspend_state; > + > +static int psci_system_suspend_enter(suspend_state_t state) > +{ > + if (state != PM_SUSPEND_MEM) > + return 0; > + > + /* > + * TODO remove pack/unpacking of power_state to do away with > + * these ugly type conversions > + */ > + return __cpu_suspend((unsigned long)&psci_system_suspend_state, > + psci_suspend_finisher); > +} > + > +static const struct platform_suspend_ops psci_suspend_ops = { > + .valid = suspend_valid_only_mem, > + .enter = psci_system_suspend_enter, > +}; > + > +static void __init psci_0_2_system_suspend_init(void) > +{ > + int ret; > + u32 psci_power_state; > + const char *entry_method; > + struct device_node *node; > + > + if (!psci_ops.cpu_suspend) > + return; /* -EOPNOTSUPP */ > + > + node = of_find_compatible_node(NULL, NULL, "arm,system-suspend"); > + if (!node || !of_device_is_available(node)) > + return; /* -EOPNOTSUPP */ > + > + if (of_property_read_string(node, "entry-method", &entry_method)) { > + pr_warn(" * %s missing entry-method property\n", node->full_name); > + goto exit; > + } > + > + if (strcmp(entry_method, "arm,psci")) > + goto exit; /* out of PSCI scope ignore */ > + > + ret = of_property_read_u32(node, "arm,psci-suspend-param", > + &psci_power_state); > + if (ret) { > + pr_warn(" * %s missing arm,psci-suspend-param property\n", > + node->full_name); > + goto exit; > + } > + > + pr_debug("psci-power-state for system suspend %#x\n", psci_power_state); > + > + psci_power_state_unpack(psci_power_state, &psci_system_suspend_state); > + How about unify the power states passing for cpu idle and suspend? below is a example for dts which place all power state into psci entry, so that idle-states and system suspend both can reference the power state. psci { compatible = "arm,psci-0.2"; method = "smc"; power_state { CPU_POWER_OFF: cpu_power_off { state = <0x00010000>; }; CLUSTER_POWER_OFF: cluster_power_off { state = <0x01010000>; }; SOC_SUSPEND: soc_suspend { state = <0x01010001>; }; }; }; > + suspend_set_ops(&psci_suspend_ops); > +exit: > + of_node_put(node); > +} > +#else > +static void __init psci_0_2_system_suspend_init(void) { } > +#endif > + > /* > * PSCI Function IDs for v0.2+ are well defined so use > * standard values. > @@ -361,6 +431,8 @@ static int __init psci_0_2_init(struct device_node *np) > > pm_power_off = psci_sys_poweroff; > > + psci_0_2_system_suspend_init(); > + > out_put_node: > of_node_put(np); > return err; > @@ -509,14 +581,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu) > #endif > #endif > > -static int psci_suspend_finisher(unsigned long index) > -{ > - struct psci_power_state *state = __this_cpu_read(psci_power_state); > - > - return psci_ops.cpu_suspend(state[index - 1], > - virt_to_phys(cpu_resume)); > -} > - > static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index) > { > int ret; > @@ -531,7 +595,8 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index) > if (state[index - 1].type == PSCI_POWER_STATE_TYPE_STANDBY) > ret = psci_ops.cpu_suspend(state[index - 1], 0); > else > - ret = __cpu_suspend(index, psci_suspend_finisher); > + ret = __cpu_suspend((unsigned long)&state[index - 1], > + psci_suspend_finisher); > > return ret; > } > -- > 1.9.1 >
On Thu, Jan 22, 2015 at 06:18:12AM +0000, Leo Yan wrote: [...] > How about unify the power states passing for cpu idle and suspend? > > below is a example for dts which place all power state into psci > entry, so that idle-states and system suspend both can reference > the power state. > > psci { > compatible = "arm,psci-0.2"; > method = "smc"; > > power_state { > CPU_POWER_OFF: cpu_power_off { > state = <0x00010000>; > }; > > CLUSTER_POWER_OFF: cluster_power_off { > state = <0x01010000>; > }; > > SOC_SUSPEND: soc_suspend { > state = <0x01010001>; > }; > }; > }; I do not see why this would help. We would end up with phandles to the nodes above to get the parameter from idle-states, to me it looks like churn honestly, I do not see where the improvement is. Lorenzo
hi Lorenzo, On Thu, Jan 22, 2015 at 12:08:50PM +0000, Lorenzo Pieralisi wrote: > On Thu, Jan 22, 2015 at 06:18:12AM +0000, Leo Yan wrote: > > [...] > > > How about unify the power states passing for cpu idle and suspend? > > > > below is a example for dts which place all power state into psci > > entry, so that idle-states and system suspend both can reference > > the power state. > > > > psci { > > compatible = "arm,psci-0.2"; > > method = "smc"; > > > > power_state { > > CPU_POWER_OFF: cpu_power_off { > > state = <0x00010000>; > > }; > > > > CLUSTER_POWER_OFF: cluster_power_off { > > state = <0x01010000>; > > }; > > > > SOC_SUSPEND: soc_suspend { > > state = <0x01010001>; > > }; > > }; > > }; > > I do not see why this would help. We would end up with phandles to > the nodes above to get the parameter from idle-states, to me it > looks like churn honestly, I do not see where the improvement is. My original thought is these power states actually are mainly used by the arm trusted firmware; in kernel, it only need maintain such power state ids and pass these power state to firmware according to the requirement from cpuidle or suspend flows. For cpuidle and suspend, they only need to know the index which should use and simply pass this index to the function *cpu_suspend(idx)* will be enough. So finally we also can simplize the code to parse these power state. Following upper idea, the dts can be written like this way: psci { compatible = "arm,psci-0.2"; method = "smc"; power_state { CPU_POWER_OFF: cpu_power_off { state = <0x00010000>; }; CLUSTER_POWER_OFF: cluster_power_off { state = <0x01010000>; }; SOC_SUSPEND: soc_suspend { state = <0x01010001>; }; }; }; idle-states { entry-method = "arm,psci"; C1: cpu-power-down { compatible = "arm,idle-state"; arm,psci-state-idx = 0; entry-latency-us = <20>; exit-latency-us = <40>; min-residency-us = <80>; }; MP: cluster-power-down { compatible = "arm,idle-state"; local-timer-stop; arm,psci-state-idx = 1; entry-latency-us = <50>; exit-latency-us = <100>; min-residency-us = <250>; wakeup-latency-us = <130>; }; }; system-suspend { compatible = "arm,system-suspend"; entry-method = "arm,psci"; arm,psci-state-idx = 2; }; Before i have not followed up the discussion tightly, so if i miss something introduce noise for this topic, sorry about that. Thanks, Leo Yan
On Thu, Jan 22, 2015 at 02:34:43PM +0000, Leo Yan wrote: > hi Lorenzo, > > On Thu, Jan 22, 2015 at 12:08:50PM +0000, Lorenzo Pieralisi wrote: > > On Thu, Jan 22, 2015 at 06:18:12AM +0000, Leo Yan wrote: > > > > [...] > > > > > How about unify the power states passing for cpu idle and suspend? > > > > > > below is a example for dts which place all power state into psci > > > entry, so that idle-states and system suspend both can reference > > > the power state. > > > > > > psci { > > > compatible = "arm,psci-0.2"; > > > method = "smc"; > > > > > > power_state { > > > CPU_POWER_OFF: cpu_power_off { > > > state = <0x00010000>; > > > }; > > > > > > CLUSTER_POWER_OFF: cluster_power_off { > > > state = <0x01010000>; > > > }; > > > > > > SOC_SUSPEND: soc_suspend { > > > state = <0x01010001>; > > > }; > > > }; > > > }; > > > > I do not see why this would help. We would end up with phandles to > > the nodes above to get the parameter from idle-states, to me it > > looks like churn honestly, I do not see where the improvement is. > > My original thought is these power states actually are mainly used by > the arm trusted firmware; in kernel, it only need maintain such power > state ids and pass these power state to firmware according to the > requirement from cpuidle or suspend flows. > > For cpuidle and suspend, they only need to know the index which > should use and simply pass this index to the function *cpu_suspend(idx)* > will be enough. So finally we also can simplize the code to parse > these power state. > > Following upper idea, the dts can be written like this way: > > psci { > compatible = "arm,psci-0.2"; > method = "smc"; > > power_state { > CPU_POWER_OFF: cpu_power_off { > state = <0x00010000>; > }; > > CLUSTER_POWER_OFF: cluster_power_off { > state = <0x01010000>; > }; > > SOC_SUSPEND: soc_suspend { > state = <0x01010001>; > }; > }; > }; These are only glorified containers for single ID values, and all the information which could potentially have been shared (e.g. local-timer-stop) is not. > > idle-states { > entry-method = "arm,psci"; > > C1: cpu-power-down { > compatible = "arm,idle-state"; > arm,psci-state-idx = 0; This implicitly relies on the ordering of nodes above, which is not a very good idea. As Lorenzo mentioned, we would have to use phandles to explicitly refer to nodes in this matter. So all that we've achieved here is replacing the raw state ID with an indirection to the state ID, and haven't gained any uniformity across idle and suspend states anyway. I don't see the point in just indirecting in this manner unless some additional information is shared. Mark. > entry-latency-us = <20>; > exit-latency-us = <40>; > min-residency-us = <80>; > }; > > MP: cluster-power-down { > compatible = "arm,idle-state"; > local-timer-stop; > arm,psci-state-idx = 1; > entry-latency-us = <50>; > exit-latency-us = <100>; > min-residency-us = <250>; > wakeup-latency-us = <130>; > }; > }; > > system-suspend { > compatible = "arm,system-suspend"; > entry-method = "arm,psci"; > arm,psci-state-idx = 2; > }; > > Before i have not followed up the discussion tightly, so if i miss something > introduce noise for this topic, sorry about that. > > Thanks, > Leo Yan >
diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c index f1dbca7d5c96..8be464747dd6 100644 --- a/arch/arm64/kernel/psci.c +++ b/arch/arm64/kernel/psci.c @@ -22,6 +22,7 @@ #include <linux/pm.h> #include <linux/delay.h> #include <linux/slab.h> +#include <linux/suspend.h> #include <uapi/linux/psci.h> #include <asm/compiler.h> @@ -304,6 +305,75 @@ static void psci_sys_poweroff(void) invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0); } +static int psci_suspend_finisher(unsigned long arg) +{ + return psci_ops.cpu_suspend(*(struct psci_power_state *)arg, + virt_to_phys(cpu_resume)); +} + +#ifdef CONFIG_SUSPEND +static struct psci_power_state psci_system_suspend_state; + +static int psci_system_suspend_enter(suspend_state_t state) +{ + if (state != PM_SUSPEND_MEM) + return 0; + + /* + * TODO remove pack/unpacking of power_state to do away with + * these ugly type conversions + */ + return __cpu_suspend((unsigned long)&psci_system_suspend_state, + psci_suspend_finisher); +} + +static const struct platform_suspend_ops psci_suspend_ops = { + .valid = suspend_valid_only_mem, + .enter = psci_system_suspend_enter, +}; + +static void __init psci_0_2_system_suspend_init(void) +{ + int ret; + u32 psci_power_state; + const char *entry_method; + struct device_node *node; + + if (!psci_ops.cpu_suspend) + return; /* -EOPNOTSUPP */ + + node = of_find_compatible_node(NULL, NULL, "arm,system-suspend"); + if (!node || !of_device_is_available(node)) + return; /* -EOPNOTSUPP */ + + if (of_property_read_string(node, "entry-method", &entry_method)) { + pr_warn(" * %s missing entry-method property\n", node->full_name); + goto exit; + } + + if (strcmp(entry_method, "arm,psci")) + goto exit; /* out of PSCI scope ignore */ + + ret = of_property_read_u32(node, "arm,psci-suspend-param", + &psci_power_state); + if (ret) { + pr_warn(" * %s missing arm,psci-suspend-param property\n", + node->full_name); + goto exit; + } + + pr_debug("psci-power-state for system suspend %#x\n", psci_power_state); + + psci_power_state_unpack(psci_power_state, &psci_system_suspend_state); + + suspend_set_ops(&psci_suspend_ops); +exit: + of_node_put(node); +} +#else +static void __init psci_0_2_system_suspend_init(void) { } +#endif + /* * PSCI Function IDs for v0.2+ are well defined so use * standard values. @@ -361,6 +431,8 @@ static int __init psci_0_2_init(struct device_node *np) pm_power_off = psci_sys_poweroff; + psci_0_2_system_suspend_init(); + out_put_node: of_node_put(np); return err; @@ -509,14 +581,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu) #endif #endif -static int psci_suspend_finisher(unsigned long index) -{ - struct psci_power_state *state = __this_cpu_read(psci_power_state); - - return psci_ops.cpu_suspend(state[index - 1], - virt_to_phys(cpu_resume)); -} - static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index) { int ret; @@ -531,7 +595,8 @@ static int __maybe_unused cpu_psci_cpu_suspend(unsigned long index) if (state[index - 1].type == PSCI_POWER_STATE_TYPE_STANDBY) ret = psci_ops.cpu_suspend(state[index - 1], 0); else - ret = __cpu_suspend(index, psci_suspend_finisher); + ret = __cpu_suspend((unsigned long)&state[index - 1], + psci_suspend_finisher); return ret; }
PSCI specifications upto v0.2 and the related kernel back-end implementation lack a method to enter system wide suspend state. This patch implements suspend to RAM support for all ARM64 systems with PSCIv0.2 support using CPU SUSPEND and the new system state DT bindings. Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- arch/arm64/kernel/psci.c | 83 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 74 insertions(+), 9 deletions(-)