Hi Arnab, On Wed, Aug 27, 2014 at 09:29:58PM +0100, Arnab Basu wrote: > Implement core support for PSCI. As this is generic code, it doesn't > implement anything really useful (all the functions are returning > Not Implemented).
This is really nice to see! Thanks for working on this. Some functions which return NOT_IMPLEMENTED below are requried to be implemented per the PSCI 0.2 spec. I hope that the plan is to implement all the required functions before we turn this on? Otherwise, comments below. > This is largely ported from the similar code that exists for ARMv7 > > Signed-off-by: Arnab Basu <arnab.b...@freescale.com> > Reviewed-by: Bhupesh Sharma <bhupesh.sha...@freescale.com> > Cc: Marc Zyngier <marc.zyng...@arm.com> > --- > arch/arm/cpu/armv8/Makefile | 1 + > arch/arm/cpu/armv8/psci.S | 171 > +++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 172 insertions(+), 0 deletions(-) > create mode 100644 arch/arm/cpu/armv8/psci.S > > diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile > index 4f0ea87..8f6988d 100644 > --- a/arch/arm/cpu/armv8/Makefile > +++ b/arch/arm/cpu/armv8/Makefile > @@ -15,3 +15,4 @@ obj-y += cache.o > obj-y += tlb.o > obj-y += transition.o > obj-y += cpu-dt.o > +obj-$(CONFIG_ARMV8_PSCI) += psci.o > diff --git a/arch/arm/cpu/armv8/psci.S b/arch/arm/cpu/armv8/psci.S > new file mode 100644 > index 0000000..5f4e3b2 > --- /dev/null > +++ b/arch/arm/cpu/armv8/psci.S > @@ -0,0 +1,171 @@ > +/* > + * (C) Copyright 2014 > + * Arnab Basu <arnab.b...@freescale.com> > + * > + * Based on arch/arm/cpu/armv7/psci.S > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <linux/linkage.h> > +#include <asm/psci.h> > + > +.pushsection ._secure.text, "ax" > + > +ENTRY(psci_0_2_cpu_suspend_64) > +ENTRY(psci_0_2_cpu_on_64) > +ENTRY(psci_0_2_affinity_info_64) > +ENTRY(psci_0_2_migrate_64) > +ENTRY(psci_0_2_migrate_info_up_cpu_64) > + mov x0, #ARM_PSCI_RET_NI /* Return -1 (Not Implemented) */ > + ret > +ENDPROC(psci_0_2_cpu_suspend_64) > +ENDPROC(psci_0_2_cpu_on_64) > +ENDPROC(psci_0_2_affinity_info_64) > +ENDPROC(psci_0_2_migrate_64) > +ENDPROC(psci_0_2_migrate_info_up_cpu_64) > +.weak psci_0_2_cpu_suspend_64 > +.weak psci_0_2_cpu_on_64 > +.weak psci_0_2_affinity_info_64 > +.weak psci_0_2_migrate_64 > +.weak psci_0_2_migrate_info_up_cpu_64 > + > +ENTRY(psci_0_2_psci_version) > + mov x0, #2 /* Return Major = 0, Minor = 2*/ > + ret > +ENDPROC(psci_0_2_psci_version) > + > +.align 4 > +_psci_0_2_table: > + .quad PSCI_0_2_FN_PSCI_VERSION > + .quad psci_0_2_psci_version > + .quad PSCI_0_2_FN64_CPU_SUSPEND > + .quad psci_0_2_cpu_suspend_64 > + .quad PSCI_0_2_FN64_CPU_ON > + .quad psci_0_2_cpu_on_64 > + .quad PSCI_0_2_FN64_AFFINITY_INFO > + .quad psci_0_2_affinity_info_64 > + .quad PSCI_0_2_FN64_MIGRATE > + .quad psci_0_2_migrate_64 > + .quad PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU > + .quad psci_0_2_migrate_info_up_cpu_64 > + .quad 0 > + .quad 0 It would be nice if we could reorganise this something like: .quad PSCI_0_2_FN_PSCI_VERSION, psci_0_2_psci_version .quad PSCI_0_2_FN64_CPU_SUSPEND, psci_0_2_cpu_suspend_64 .quad PSCI_0_2_FN64_CPU_ON, psci_0_2_cpu_on_64 .quad PSCI_0_2_FN64_AFFINITY_INFO, psci_0_2_affinity_info_64 .quad PSCI_0_2_FN64_MIGRATE, psci_0_2_migrate_64 .quad PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU, psci_0_2_migrate_info_up_cpu_64 .quad 0, 0 As that would make the relationship between IDs and functions clearer (at least to me). Maybe a macro could make this less painful. > +.macro psci_enter > + stp x29, x30, [sp, #-16]! > + stp x27, x28, [sp, #-16]! > + stp x25, x26, [sp, #-16]! > + stp x23, x24, [sp, #-16]! > + stp x21, x22, [sp, #-16]! > + stp x19, x20, [sp, #-16]! > + stp x17, x18, [sp, #-16]! > + stp x15, x16, [sp, #-16]! > + stp x13, x14, [sp, #-16]! > + stp x11, x12, [sp, #-16]! > + stp x9, x10, [sp, #-16]! > + stp x7, x8, [sp, #-16]! > + stp x5, x6, [sp, #-16]! > + mrs x5, elr_el3 > + stp x5, x4, [sp, #-16]! > + > + /* EL0 and El1 will execute in secure */ I think this would be better as: /* U-Boot will run on the secure side */ > + mrs x4, scr_el3 > + bic x4, x4, #1 > + msr scr_el3, x4 > +.endm > + > +.macro psci_return > + /* EL0 and El1 will execute in non-secure */ Similarly: /* The OS will run on the non-secure side */ > + mrs x4, scr_el3 > + orr x4, x4, #1 > + msr scr_el3, x4 > + > + ldp x5, x4, [sp], #16 > + msr elr_el3, x5 > + ldp x5, x6, [sp], #16 > + ldp x7, x8, [sp], #16 > + ldp x9, x10, [sp], #16 > + ldp x11, x12, [sp], #16 > + ldp x13, x14, [sp], #16 > + ldp x15, x16, [sp], #16 > + ldp x17, x18, [sp], #16 > + ldp x19, x20, [sp], #16 > + ldp x21, x22, [sp], #16 > + ldp x23, x24, [sp], #16 > + ldp x25, x26, [sp], #16 > + ldp x27, x28, [sp], #16 > + ldp x29, x30, [sp], #16 > + eret > +.endm PSCI 0.2 follows the SMC calling convention, where x0-x17 are caller-saved as with the usual AArch64 calling conventions, so we don't need to save/restore them here for AArch64 callers. We do need to ensure we don't clobber x18-x30, sp_elx and sp_el0. The calling convention is ambiguous as to whether we can clobber sp_el1, but for the moment I'd assume we shouldn't. For AArch32 callers, only x0-x3 are caller-saved. > +ENTRY(_smc_psci) > + psci_enter > + adr x4, _psci_0_2_table > +1: ldr x5, [x4] /* Load PSCI function ID */ > + ldr x6, [x4, #8] /* Load target PC */ 1: ldp x5, x6, [x4] /* Load function ID and handler addr */ > + cmp x5, #0 /* If reach the end, bail out */ > + b.eq fn_not_found cbz x5, fn_not_found /* If reach the end, bail out */ > + cmp x0, x5 /* If not matching, try next entry */ > + b.eq fn_call > + add x4, x4, #16 > + b 1b > + > +fn_call: > + blr x6 > + psci_return > + > +fn_not_found: > + mov x0, #ARM_PSCI_RET_INVAL /* Return -2 (Invalid) */ I believe this should be NOT_SUPPORTED (-1), which states that the PSCI implementation doesn't implement this function rather than the arguments to the function were invalid. > + psci_return > +ENDPROC(_smc_psci) > + > +ENTRY(default_psci_vector) > + eret > +ENDPROC(default_psci_vector) This looks misnamed. This is the vector for exceptions triggered by something other than PSCI. That said I think we need to do something else here. If we haven't handled the exception we're likely to just return to whatever caused it and take the same exception again. A failstop behaviour would seem better if the only other exceptions we expect to take are fatal anyway. > +.align 2 I don't think this is necessary; we had code immediately before. > +__handle_sync: > + str x4, [sp, #-8]! > + mrs x4, esr_el3 > + ubfx x4, x4, #26, #6 > + cmp x4, #0x17 Could we not have a macro like ESR_EC_SMC64 instead of the #17? Thanks, Mark. > + b.eq smc_found > + ldr x4, [sp], #8 > + b default_psci_vector > +smc_found: > + ldr x4, [sp], #8 > + b _smc_psci > + > +/* > + * PSCI Exception vectors. > + */ > + .align 11 > + .globl psci_vectors > +psci_vectors: > + .align 7 > + b default_psci_vector /* Current EL Synchronous Thread */ > + .align 7 > + b default_psci_vector /* Current EL IRQ Thread */ > + .align 7 > + b default_psci_vector /* Current EL FIQ Thread */ > + .align 7 > + b default_psci_vector /* Current EL Error Thread */ > + .align 7 > + b default_psci_vector /* Current EL Synchronous Handler */ > + .align 7 > + b default_psci_vector /* Current EL IRQ Handler */ > + .align 7 > + b default_psci_vector /* Current EL FIQ Handler */ > + .align 7 > + b default_psci_vector /* Current EL Error Handler */ > + .align 7 > + b __handle_sync /* Lower EL Synchronous (64b) */ > + .align 7 > + b default_psci_vector /* Lower EL IRQ (64b) */ > + .align 7 > + b default_psci_vector /* Lower EL FIQ (64b) */ > + .align 7 > + b default_psci_vector /* Lower EL Error (64b) */ > + > +.popsection > -- > 1.7.7.4 > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot