[U-Boot] [PATCH v3 07/11] ARMv7: PSCI: add PSCI v1.0 functions skeleton
Hongbo Zhang
macro.wave.z at gmail.com
Thu May 19 10:23:38 CEST 2016
On Wed, May 18, 2016 at 6:39 PM, Andre Przywara <andre.przywara at arm.com> wrote:
> 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.
>
I don't think so, because this is just a dummy and common framework,
we cannot expect which functions will be implemented in each specific
platforms, it is up to each platform's developer to decide which
functions they need.
We offer default NOT_SUPPORTED implementation here, then each platform
is only responsible to their own implementations, so when a
non-implemented function is called, the PSCI code won't crush.
>>
>> _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?
>
This a for compatible of the v0.1, because in v0.1 the function IDs
are implementation decided and may not be continuous.
>>
>> 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"
I use fdt_appendprop_string, it *append* compatible string instead of
*set*, so the final string would be:
"arm,psci-1.0", "arm,psci"
The "arm,psci-0,2", was missed, our internal opinion was that since
we've had latest v1.0 and the v0.2 isn't implement at all, so we could
skip v0.2 here.
But now your suggestion sounds reasonable, since v1.0 is compatible
with v0.2, we can add these three strings in the 1_0_fixup.
> - 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.
>
Yes, good suggestion, thanks.
>> +
>> +#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.
>
Yes.
>> +#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.
>
Thanks for review.
Hongbo Zhang @ NXP/Freescale
> Cheers,
> Andre.
More information about the U-Boot
mailing list