Re: [RFC PATCH 3/3] arm: psci: add cpuidle_ops support

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Sat, Jul 04, 2015 at 02:01:50PM +0100, Jisheng Zhang wrote:
> This patch implement cpuidle_ops using psci, the code is stolen from
> arm64. Now we can use cpuidle-arm.c for both arm and arm64.

You mean cpuidle-arm.c with PSCI back-end. You will have to rewrite
the commit log anyway since this patch will be significantly trimmed,
see below.

> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
>  arch/arm/kernel/psci.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
> 
> diff --git a/arch/arm/kernel/psci.c b/arch/arm/kernel/psci.c
> index 7f6ff02..7bf744d 100644
> --- a/arch/arm/kernel/psci.c
> +++ b/arch/arm/kernel/psci.c
> @@ -13,16 +13,132 @@
>   * Author: Will Deacon <will.deacon@arm.com>
>   */
>  
> +#include <linux/cpuidle.h>
>  #include <linux/init.h>
>  #include <linux/smp.h>
>  #include <linux/of.h>
>  #include <linux/delay.h>
>  #include <linux/psci.h>
> +#include <linux/slab.h>
>  
>  #include <uapi/linux/psci.h>
>  
> +#include <asm/cpuidle.h>
>  #include <asm/psci.h>
>  #include <asm/smp_plat.h>
> +#include <asm/suspend.h>
> +
> +#ifdef CONFIG_CPU_IDLE
> +static bool psci_power_state_loses_context(u32 state)
> +{
> +	return state & PSCI_0_2_POWER_STATE_TYPE_MASK;
> +}
> +
> +static bool psci_power_state_is_valid(u32 state)
> +{
> +	const u32 valid_mask = PSCI_0_2_POWER_STATE_ID_MASK |
> +			       PSCI_0_2_POWER_STATE_TYPE_MASK |
> +			       PSCI_0_2_POWER_STATE_AFFL_MASK;
> +
> +	return !(state & ~valid_mask);
> +}

Already moved these helpers to drivers/firmware:

http://lists.infradead.org/pipermail/linux-arm-kernel/2015-May/347355.html

> +
> +static DEFINE_PER_CPU_READ_MOSTLY(u32 *, psci_power_state);
> +
> +static int __init cpu_psci_cpu_init_idle(struct device_node *cpu_node,
> +					 int cpu)
> +{
> +	int i, ret, count = 0;
> +	u32 *psci_states;
> +	struct device_node *state_node;
> +
> +	/*
> +	 * 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_warn("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;
> +}
> +
> +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 cpu_psci_cpu_suspend(int cpu, 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;
> +}

Code above is arch agnostic and should be moved out of arch/arm (and
arm64).

Can you put together a patch to do it or you want me to do it ?

> +static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> +	.suspend = cpu_psci_cpu_suspend,
> +	.init = cpu_psci_cpu_init_idle,
> +};
> +CPUIDLE_METHOD_OF_DECLARE(psci_idle, "psci", &psci_cpuidle_ops);
> +#endif

This is fine and should stay in arch/arm.

Thanks,
Lorenzo