On Wed, May 18, 2016 at 6:39 PM, Andre Przywara andre.przywara@arm.com wrote:
Hi,
On 18/05/16 10:10, macro.wave.z@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.
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 0So 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.