Re: [PATCH v2 1/2] firmware: psci: move cpu_suspend handling to generic code

From: Lorenzo Pieralisi
Date: Wed Jul 08 2015 - 13:32:01 EST

Next message: Greg Kroah-Hartman: "Re: [PATCH 4.1 54/56] fs/ufs: restore s_lock mutex"
Previous message: Andy Lutomirski: "Re: [PATCH] x86/kconfig/32: Mark CONFIG_VM86 as BROKEN"
In reply to: Jisheng Zhang: "[PATCH v2 1/2] firmware: psci: move cpu_suspend handling to generic code"
Next in thread: Jisheng Zhang: "[PATCH v2 2/2] arm: kernel: implement cpuidle_ops with psci backend"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Jisheng,

On Wed, Jul 08, 2015 at 01:51:07PM +0100, Jisheng Zhang wrote:
> Functions implemented on arm64 to suspend cpu and translate the idle
> state index passed by the cpu_suspend core call to a valid PSCI state
> are not arm64 specific and should be moved to generic code so that they
> can be reused on arm systems too.
>
> This patch moves these functions to generic PSCI firmware layer code.
>
> Signed-off-by: Jisheng Zhang <jszhang@xxxxxxxxxxx>
> ---
> arch/arm64/kernel/psci.c | 95 ------------------------------------------------
> drivers/firmware/psci.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/psci.h | 2 +
> 3 files changed, 97 insertions(+), 95 deletions(-)
>

Overall it seems fine, please rebase on top of the series I have
just posted (with a couple of change requests below):

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355098.html

> diff --git a/arch/arm64/kernel/psci.c b/arch/arm64/kernel/psci.c
> index 4be5972..06e5c9a 100644
> --- a/arch/arm64/kernel/psci.c
> +++ b/arch/arm64/kernel/psci.c
> @@ -20,7 +20,6 @@
> #include <linux/smp.h>
> #include <linux/delay.h>
> #include <linux/psci.h>
> -#include <linux/slab.h>
>
> #include <uapi/linux/psci.h>
>
> @@ -28,73 +27,6 @@
> #include <asm/cpu_ops.h>
> #include <asm/errno.h>
> #include <asm/smp_plat.h>
> -#include <asm/suspend.h>
> -
> -static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> -
> -static int __maybe_unused cpu_psci_cpu_init_idle(unsigned int cpu)
> -{
> - int i, ret, count = 0;
> - u32 *psci_states;
> - struct device_node *state_node, *cpu_node;
> -
> - cpu_node = of_get_cpu_node(cpu, NULL);
> - if (!cpu_node)
> - return -ENODEV;
> -
> - /*
> - * If the PSCI cpu_suspend function hook has not been initialized
> - * idle states must not be enabled, so bail out
> - */
> - if (!psci_ops.cpu_suspend)
> - return -EOPNOTSUPP;
> -
> - /* Count idle states */
> - while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
> - count))) {
> - count++;
> - of_node_put(state_node);
> - }
> -
> - if (!count)
> - return -ENODEV;
> -
> - psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> - if (!psci_states)
> - return -ENOMEM;
> -
> - for (i = 0; i < count; i++) {
> - u32 state;
> -
> - state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> -
> - ret = of_property_read_u32(state_node,
> - "arm,psci-suspend-param",
> - &state);
> - if (ret) {
> - pr_warn(" * %s missing arm,psci-suspend-param property\n",
> - state_node->full_name);
> - of_node_put(state_node);
> - goto free_mem;
> - }
> -
> - of_node_put(state_node);
> - pr_debug("psci-power-state %#x index %d\n", state, i);
> - if (!psci_power_state_is_valid(state)) {
> - pr_warn("Invalid PSCI power state %#x\n", state);
> - ret = -EINVAL;
> - goto free_mem;
> - }
> - psci_states[i] = state;
> - }
> - /* Idle states parsed correctly, initialize per-cpu pointer */
> - per_cpu(psci_power_state, cpu) = psci_states;
> - return 0;
> -
> -free_mem:
> - kfree(psci_states);
> - return ret;
> -}
>
> #ifdef CONFIG_SMP
>
> @@ -181,33 +113,6 @@ static int cpu_psci_cpu_kill(unsigned int cpu)
> #endif
> #endif
>
> -static int psci_suspend_finisher(unsigned long index)
> -{
> - u32 *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;
> - u32 *state = __this_cpu_read(psci_power_state);
> - /*
> - * idle state index 0 corresponds to wfi, should never be called
> - * from the cpu_suspend operations
> - */
> - if (WARN_ON_ONCE(!index))
> - return -EINVAL;
> -
> - if (!psci_power_state_loses_context(state[index - 1]))
> - ret = psci_ops.cpu_suspend(state[index - 1], 0);
> - else
> - ret = cpu_suspend(index, psci_suspend_finisher);
> -
> - return ret;
> -}
> -
> const struct cpu_operations cpu_psci_ops = {
> .name = "psci",
> #ifdef CONFIG_CPU_IDLE
> diff --git a/drivers/firmware/psci.c b/drivers/firmware/psci.c
> index 2f5d611..5da8aa2 100644
> --- a/drivers/firmware/psci.c
> +++ b/drivers/firmware/psci.c
> @@ -20,12 +20,14 @@
> #include <linux/printk.h>
> #include <linux/psci.h>
> #include <linux/reboot.h>
> +#include <linux/slab.h>
>
> #include <uapi/linux/psci.h>
>
> #include <asm/cputype.h>
> #include <asm/system_misc.h>
> #include <asm/smp_plat.h>
> +#include <asm/suspend.h>
>
> /*
> * While a 64-bit OS can make calls with SMC32 calling conventions, for some
> @@ -81,6 +83,8 @@ static u32 psci_function_id[PSCI_FN_MAX];
>
> static u32 psci_cpu_suspend_feature;
>
> +static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> +
> static inline bool psci_has_ext_power_state(void)
> {
> return psci_cpu_suspend_feature &
> @@ -217,6 +221,97 @@ static void psci_sys_poweroff(void)
> invoke_psci_fn(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> }
>
> +int cpu_psci_cpu_init_idle(unsigned int cpu)
> +{
> + int i, ret, count = 0;
> + u32 *psci_states;
> + struct device_node *state_node, *cpu_node;
> +
> + cpu_node = of_get_cpu_node(cpu, NULL);
> + if (!cpu_node)
> + return -ENODEV;
> +
> + /*
> + * If the PSCI cpu_suspend function hook has not been initialized
> + * idle states must not be enabled, so bail out
> + */
> + if (!psci_ops.cpu_suspend)
> + return -EOPNOTSUPP;
> +
> + /* Count idle states */
> + while ((state_node = of_parse_phandle(cpu_node, "cpu-idle-states",
> + count))) {
> + count++;
> + of_node_put(state_node);
> + }
> +
> + if (!count)
> + return -ENODEV;
> +
> + psci_states = kcalloc(count, sizeof(*psci_states), GFP_KERNEL);
> + if (!psci_states)
> + return -ENOMEM;
> +
> + for (i = 0; i < count; i++) {
> + u32 state;
> +
> + state_node = of_parse_phandle(cpu_node, "cpu-idle-states", i);
> +
> + ret = of_property_read_u32(state_node,
> + "arm,psci-suspend-param",
> + &state);
> + if (ret) {
> + pr_warn(" * %s missing arm,psci-suspend-param property\n",
> + state_node->full_name);
> + of_node_put(state_node);
> + goto free_mem;
> + }
> +
> + of_node_put(state_node);
> + pr_debug("psci-power-state %#x index %d\n", state, i);
> + if (!psci_power_state_is_valid(state)) {

psci_power_state_is_valid() can be made static and removed
from include/linux/psci.h

> + pr_warn("Invalid PSCI power state %#x\n", state);
> + ret = -EINVAL;
> + goto free_mem;
> + }
> + psci_states[i] = state;
> + }
> + /* Idle states parsed correctly, initialize per-cpu pointer */
> + per_cpu(psci_power_state, cpu) = psci_states;
> + return 0;
> +
> +free_mem:
> + kfree(psci_states);
> + return ret;
> +}
> +
> +static int psci_suspend_finisher(unsigned long index)
> +{
> + u32 *state = __this_cpu_read(psci_power_state);
> +
> + return psci_ops.cpu_suspend(state[index - 1],
> + virt_to_phys(cpu_resume));
> +}
> +
> +int cpu_psci_cpu_suspend(unsigned long index)
> +{
> + int ret;
> + u32 *state = __this_cpu_read(psci_power_state);
> + /*
> + * idle state index 0 corresponds to wfi, should never be called
> + * from the cpu_suspend operations
> + */
> + if (WARN_ON_ONCE(!index))
> + return -EINVAL;
> +
> + if (!psci_power_state_loses_context(state[index - 1]))

psci_power_state_loses_context() can be made static and removed
from include/linux/psci.h

Thanks,
Lorenzo

> + ret = psci_ops.cpu_suspend(state[index - 1], 0);
> + else
> + ret = cpu_suspend(index, psci_suspend_finisher);
> +
> + return ret;
> +}
> +
> static int __init psci_features(u32 psci_func_id)
> {
> return invoke_psci_fn(PSCI_1_0_FN_PSCI_FEATURES,
> diff --git a/include/linux/psci.h b/include/linux/psci.h
> index 12c4865..cbb49a3 100644
> --- a/include/linux/psci.h
> +++ b/include/linux/psci.h
> @@ -23,6 +23,8 @@
> bool psci_tos_resident_on(int cpu);
> bool psci_power_state_loses_context(u32 state);
> bool psci_power_state_is_valid(u32 state);
> +int cpu_psci_cpu_init_idle(unsigned int cpu);
> +int cpu_psci_cpu_suspend(unsigned long index);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Greg Kroah-Hartman: "Re: [PATCH 4.1 54/56] fs/ufs: restore s_lock mutex"
Previous message: Andy Lutomirski: "Re: [PATCH] x86/kconfig/32: Mark CONFIG_VM86 as BROKEN"
In reply to: Jisheng Zhang: "[PATCH v2 1/2] firmware: psci: move cpu_suspend handling to generic code"
Next in thread: Jisheng Zhang: "[PATCH v2 2/2] arm: kernel: implement cpuidle_ops with psci backend"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]