[U-Boot] [PATCH v3 07/11] ARMv7: PSCI: add PSCI v1.0 functions skeleton

[Andre Przywara <andre.przywara@arm.com>]

Hi,

On 18/05/16 10:10, macro.wave.z at gmail.com wrote:
> From: Hongbo Zhang <hongbo.zhang@nxp.com>
> 
> This patch adds all the PSCI v1.0 functions in to the common framework, with
> all the functions returning "not sopported" by default, as a common framework
> all the functions are added here, it is up to every platform developer to
> decide which version of PSCI and which functions in it to be implemented.
> 
> Signed-off-by: Hongbo Zhang <hongbo.zhang@nxp.com>
> Signed-off-by: Wang Dongsheng <dongsheng.wang@nxp.com>
> ---
>  arch/arm/cpu/armv7/psci.S    | 70 ++++++++++++++++++++++++++++++++++++++++++++
>  arch/arm/cpu/armv7/virt-dt.c | 45 +++++++++++++++++++++-------
>  arch/arm/include/asm/psci.h  | 21 +++++++++++++
>  3 files changed, 125 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S
> index 28579d7..7d27300 100644
> --- a/arch/arm/cpu/armv7/psci.S
> +++ b/arch/arm/cpu/armv7/psci.S
> @@ -46,30 +46,100 @@ ENTRY(default_psci_vector)
>  ENDPROC(default_psci_vector)
>  .weak default_psci_vector
>  
> +ENTRY(psci_version)
>  ENTRY(psci_cpu_suspend)
>  ENTRY(psci_cpu_off)
>  ENTRY(psci_cpu_on)
> +ENTRY(psci_affinity_info)
>  ENTRY(psci_migrate)
> +ENTRY(psci_migrate_info_type)
> +ENTRY(psci_migrate_info_up_cpu)
> +ENTRY(psci_system_off)
> +ENTRY(psci_system_reset)
> +ENTRY(psci_features)
> +ENTRY(psci_cpu_freeze)
> +ENTRY(psci_cpu_default_suspend)
> +ENTRY(psci_node_hw_state)
> +ENTRY(psci_system_suspend)
> +ENTRY(psci_set_suspend_mode)
> +ENTRY(psi_stat_residency)
> +ENTRY(psci_stat_count)
>  	mov	r0, #PSCI_RET_NOT_SUPPORTED	@ Return -1 (Not Supported)
>  	mov	pc, lr
> +ENDPROC(psci_stat_count)
> +ENDPROC(psi_stat_residency)
> +ENDPROC(psci_set_suspend_mode)
> +ENDPROC(psci_system_suspend)
> +ENDPROC(psci_node_hw_state)
> +ENDPROC(psci_cpu_default_suspend)
> +ENDPROC(psci_cpu_freeze)
> +ENDPROC(psci_features)
> +ENDPROC(psci_system_reset)
> +ENDPROC(psci_system_off)
> +ENDPROC(psci_migrate_info_up_cpu)
> +ENDPROC(psci_migrate_info_type)
>  ENDPROC(psci_migrate)
> +ENDPROC(psci_affinity_info)
>  ENDPROC(psci_cpu_on)
>  ENDPROC(psci_cpu_off)
>  ENDPROC(psci_cpu_suspend)
> +ENDPROC(psci_version)
> +.weak psci_version
>  .weak psci_cpu_suspend
>  .weak psci_cpu_off
>  .weak psci_cpu_on
> +.weak psci_affinity_info
>  .weak psci_migrate
> +.weak psci_migrate_info_type
> +.weak psci_migrate_info_up_cpu
> +.weak psci_system_off
> +.weak psci_system_reset
> +.weak psci_features
> +.weak psci_cpu_freeze
> +.weak psci_cpu_default_suspend
> +.weak psci_node_hw_state
> +.weak psci_system_suspend
> +.weak psci_set_suspend_mode
> +.weak psi_stat_residency
> +.weak psci_stat_count

I wonder if we can have something more clever here to handle
unimplemented functions. For the PSCI_FEATURES call we would need this
very same list of functions again, so is it worthwhile to have some
_code_ instead of a table which does the dispatching?
So this code could check against a list of unimplemented functions,
returning #PSCI_RET_NOT_SUPPORTED if there is a hit.
Or we just have a list of _implemented_ functions and for every miss we
return #PSCI_RET_NOT_SUPPORTED.
The PSCI_FEATURES call could then just use the very same list and this
could be a generic implementation then.

>  
>  _psci_table:
> +	.word	PSCI_FN_PSCI_VERSION
> +	.word	psci_version
>  	.word	PSCI_FN_CPU_SUSPEND
>  	.word	psci_cpu_suspend
>  	.word	PSCI_FN_CPU_OFF
>  	.word	psci_cpu_off
>  	.word	PSCI_FN_CPU_ON
>  	.word	psci_cpu_on
> +	.word	PSCI_FN_AFFINITY_INFO
> +	.word	psci_affinity_info
>  	.word	PSCI_FN_MIGRATE
>  	.word	psci_migrate
> +	.word	PSCI_FN_MIGRATE_INFO_TYPE
> +	.word	psci_migrate_info_type
> +	.word	PSCI_FN_MIGRATE_INFO_UP_CPU
> +	.word	psci_migrate_info_up_cpu
> +	.word	PSCI_FN_SYSTEM_OFF
> +	.word	psci_system_off
> +	.word	PSCI_FN_SYSTEM_RESET
> +	.word	psci_system_reset
> +	.word	PSCI_FN_PSCI_FEATURES
> +	.word	psci_features
> +	.word	PSCI_FN_CPU_FREEZE
> +	.word	psci_cpu_freeze
> +	.word	PSCI_FN_CPU_DEFAULT_SUSPEND
> +	.word	psci_cpu_default_suspend
> +	.word	PSCI_FN_NODE_HW_STATE
> +	.word	psci_node_hw_state
> +	.word	PSCI_FN_SYSTEM_SUSPEND
> +	.word	psci_system_suspend
> +	.word	PSCI_FN_SET_SUSPEND_MODE
> +	.word	psci_set_suspend_mode
> +	.word	PSCI_FN_STAT_RESIDENCY
> +	.word	psi_stat_residency
> +	.word	PSCI_FN_STAT_COUNT
> +	.word	psci_stat_count
>  	.word	0
>  	.word	0

So those function IDs are now contigious and also much more, can we just
use the lower bits of the function ID as an index and get rid of the
redundant pairing of (function ID, address) here?

>  
> diff --git a/arch/arm/cpu/armv7/virt-dt.c b/arch/arm/cpu/armv7/virt-dt.c
> index 4953f27..08258a0 100644
> --- a/arch/arm/cpu/armv7/virt-dt.c
> +++ b/arch/arm/cpu/armv7/virt-dt.c
> @@ -26,6 +26,35 @@
>  #include <asm/armv7.h>
>  #include <asm/psci.h>
>  
> +#ifdef CONFIG_ARMV7_PSCI
> +#ifdef CONFIG_ARMV7_PSCI_1_0
> +static int fdt_psci_1_0_fixup(void *fdt, int nodeoff)
> +{
> +	return fdt_setprop_string(fdt, nodeoff, "compatible", "arm,psci-1.0");
> +}
> +#endif
> +
> +static int fdt_psci_0_1_fixup(void *fdt, int nodeoff)
> +{
> +	int ret;
> +
> +	ret = fdt_appendprop_string(fdt, nodeoff, "compatible", "arm,psci");
> +	if (ret)
> +		return ret;
> +	ret = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend", PSCI_FN_CPU_SUSPEND);
> +	if (ret)
> +		return ret;
> +	ret = fdt_setprop_u32(fdt, nodeoff, "cpu_off", PSCI_FN_CPU_OFF);
> +	if (ret)
> +		return ret;
> +	ret = fdt_setprop_u32(fdt, nodeoff, "cpu_on", PSCI_FN_CPU_ON);
> +	if (ret)
> +		return ret;
> +
> +	return fdt_setprop_u32(fdt, nodeoff, "migrate", PSCI_FN_MIGRATE);
> +}
> +#endif
> +

I think it would be worth to use the compatibility features of the PSCI
DT bindings here, that would allow to boot OSes which don't support PSCI
0.2 and help to ease migration of boards to v1.0 PSCI without regressions.
So basically:
- The compatible string in the 1_0_fixup function becomes:
"arm,psci-1.0", "arm,psci-0,2", "arm,psci"
- This 0_1_fixup function above just sets the compatible string.
...

>  static int fdt_psci(void *fdt)
>  {
>  #ifdef CONFIG_ARMV7_PSCI
> @@ -67,22 +96,16 @@ static int fdt_psci(void *fdt)
>  			return nodeoff;
>  	}
>  
> -	tmp = fdt_setprop_string(fdt, nodeoff, "compatible", "arm,psci");
> -	if (tmp)
> -		return tmp;
>  	tmp = fdt_setprop_string(fdt, nodeoff, "method", "smc");
>  	if (tmp)
>  		return tmp;
> -	tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_suspend", PSCI_FN_CPU_SUSPEND);
> -	if (tmp)
> -		return tmp;
> -	tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_off", PSCI_FN_CPU_OFF);
> -	if (tmp)
> -		return tmp;
> -	tmp = fdt_setprop_u32(fdt, nodeoff, "cpu_on", PSCI_FN_CPU_ON);

We keep those function ID specifiers in the node for compatibility
reasons. For PSCI 0.2 and higher they are ignored, but older OSes can
just use them as before.

> +
> +#ifdef CONFIG_ARMV7_PSCI_1_0
> +	tmp = fdt_psci_1_0_fixup(fdt, nodeoff);
>  	if (tmp)
>  		return tmp;
> -	tmp = fdt_setprop_u32(fdt, nodeoff, "migrate", PSCI_FN_MIGRATE);

We keep this also.

So eventually the nodes look very similar, it's just the list of
compatible strings that differs.

> +#endif
> +	tmp = fdt_psci_0_1_fixup(fdt, nodeoff);
>  	if (tmp)
>  		return tmp;
>  #endif
> diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h
> index d703aeb..32ae359 100644
> --- a/arch/arm/include/asm/psci.h
> +++ b/arch/arm/include/asm/psci.h
> @@ -27,16 +27,37 @@
>  #define PSCI_FN_BASE			0x84000000
>  #define PSCI_FN_ID(n)			(PSCI_FN_BASE + (n))
>  
> +#define PSCI_FN_PSCI_VERSION		PSCI_FN_ID(0)
>  #define PSCI_FN_CPU_SUSPEND		PSCI_FN_ID(1)
>  #define PSCI_FN_CPU_OFF			PSCI_FN_ID(2)
>  #define PSCI_FN_CPU_ON			PSCI_FN_ID(3)
> +#define PSCI_FN_AFFINITY_INFO		PSCI_FN_ID(4)
>  #define PSCI_FN_MIGRATE			PSCI_FN_ID(5)
> +#define PSCI_FN_MIGRATE_INFO_TYPE	PSCI_FN_ID(6)
> +#define PSCI_FN_MIGRATE_INFO_UP_CPU	PSCI_FN_ID(7)
> +#define PSCI_FN_SYSTEM_OFF		PSCI_FN_ID(8)
> +#define PSCI_FN_SYSTEM_RESET		PSCI_FN_ID(9)
> +#define PSCI_FN_PSCI_FEATURES		PSCI_FN_ID(10)
> +#define PSCI_FN_CPU_FREEZE		PSCI_FN_ID(11)
> +#define PSCI_FN_CPU_DEFAULT_SUSPEND	PSCI_FN_ID(12)
> +#define PSCI_FN_NODE_HW_STATE		PSCI_FN_ID(13)
> +#define PSCI_FN_SYSTEM_SUSPEND		PSCI_FN_ID(14)
> +#define PSCI_FN_SET_SUSPEND_MODE	PSCI_FN_ID(15)
> +#define PSCI_FN_STAT_RESIDENCY		PSCI_FN_ID(16)
> +#define PSCI_FN_STAT_COUNT		PSCI_FN_ID(17)
> +
>  
>  /* PSCI return values */
>  #define PSCI_RET_SUCCESS		0
>  #define PSCI_RET_NOT_SUPPORTED		(-1)
>  #define PSCI_RET_INVALID_PARAMS		(-2)
>  #define PSCI_RET_DENIED			(-3)
> +#define PSCI_RET_ALREADY_ON		(-4)
> +#define PSCI_RET_ON_PENDING		(-5)
> +#define PSCI_RET_INTERNAL_FAILURE	(-6)
> +#define PSCI_RET_NOT_PRESENT		(-7)
> +#define PSCI_RET_DISABLED		(-8)
> +#define PSCI_RET_INVALID_ADDRESS	(-9)

I checked the function IDs and the return codes against the spec, they
are correct.

Cheers,
Andre.