[U-Boot] [PATCH v3 07/11] ARMv7: PSCI: add PSCI v1.0 functions skeleton
Andre Przywara
andre.przywara at arm.com
Wed May 18 12:39:06 CEST 2016
Hi,
On 18/05/16 10:10, macro.wave.z at gmail.com wrote:
> From: Hongbo Zhang <hongbo.zhang at 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 at nxp.com>
> Signed-off-by: Wang Dongsheng <dongsheng.wang at 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.
More information about the U-Boot
mailing list