On Tue, Jun 28, 2016 at 11:24 AM, Chen-Yu Tsai wens@csie.org wrote:
On Tue, Jun 14, 2016 at 3:01 PM, macro.wave.z@gmail.com wrote:
From: Hongbo Zhang hongbo.zhang@nxp.com
The legacy code saves target PC at stack top, this patch changes it to stack bottom, because we will save more contents for PSCI v1.0, by this way we don't need to adjust the stack pointer when more contents are saved.
Signed-off-by: Hongbo Zhang hongbo.zhang@nxp.com Signed-off-by: Wang Dongsheng dongsheng.wang@nxp.com
arch/arm/cpu/armv7/psci.S | 9 +++++---- arch/arm/include/asm/psci.h | 4 ++++ 2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/arch/arm/cpu/armv7/psci.S b/arch/arm/cpu/armv7/psci.S index 3b92f1d..5b235df 100644 --- a/arch/arm/cpu/armv7/psci.S +++ b/arch/arm/cpu/armv7/psci.S @@ -259,7 +259,8 @@ ENTRY(psci_cpu_on_common)
mov r0, r1 bl psci_get_cpu_stack_top @ get stack top of target CPU
str r2, [r0] @ store target PC at stack top
sub r5, r0, #PSCI_TARGET_PC_OFFSETstr r2, [r5] @ store target PCIMO having a separate function for this would be better. It would be clearer, and easier to reuse or replace.
Renaming psci_cpu_on_common to psci_save_target_pc doesn't apply because saving context id will be added. Do you mean the psci_cpu_on_common calls two functions psci_save_target_pc and psci_save_context_id?
Also you should save and restore r5 across this function.
I did pay attention to this, and found that in the psci codes, general purpose registers are used carefully without push/pop, and there is example of using r5 without push/pop, so I just follow this pattern. Sure saving register context makes it safer.
dsb pop {pc}@@ -286,14 +287,13 @@ ENDPROC(psci_cpu_off_common)
@ expects CPU ID in r0 and returns stack top in r0 ENTRY(psci_get_cpu_stack_top)
mov r5, #0x400 @ 1kB of stack per CPU
mov r5, #PSCI_PERCPU_STACK_SIZE @ 1kB of stack per CPU mul r0, r0, r5 ldr r5, =psci_text_end @ end of monitor text add r5, r5, #0x2000 @ Skip two pages lsr r5, r5, #12 @ Align to start of page lsl r5, r5, #12
sub r5, r5, #4 @ reserve 1 word for target PC sub r0, r5, r0 @ here's our stack!This does not apply. r5 was changed to r3.
I noticed you sent such a patch before, but when I sent out this patch set, it wasn't pulled in. Will re-check all the updates.
bx lr@@ -306,7 +306,8 @@ ENTRY(psci_cpu_entry)
bl psci_get_cpu_id @ CPU ID => r0 bl psci_get_cpu_stack_top @ stack top => r0
ldr r0, [r0] @ target PC at stack top
sub r0, r0, #PSCI_TARGET_PC_OFFSETldr r0, [r0] @ get target PC b _do_nonsec_entryENDPROC(psci_cpu_entry)
diff --git a/arch/arm/include/asm/psci.h b/arch/arm/include/asm/psci.h index 2367ec0..cb08544 100644 --- a/arch/arm/include/asm/psci.h +++ b/arch/arm/include/asm/psci.h @@ -63,6 +63,10 @@ #define ARM_PSCI_1_0_FN_STAT_RESIDENCY ARM_PSCI_0_2_FN(16) #define ARM_PSCI_1_0_FN_STAT_COUNT ARM_PSCI_0_2_FN(17)
+/* size of percpu stack, 1kB */ +#define PSCI_PERCPU_STACK_SIZE 0x400 +#define PSCI_TARGET_PC_OFFSET (PSCI_PERCPU_STACK_SIZE - 4)
I think you want PSCI_PERCPU_STACK_SIZE?
Yes.
A stack starts at 0x400 and goes down to 0x0. You want to store the target PC at 0x0, not 0x4.
But there are more details here. Suppose the stack bottom is 0x0 and stack size is 0x400, the top of stack should be 0x3fc (32bits world, covering 4 bytes 0x3fc ~ 0x3ff), 0x400 is out of the stack range. If the psci_get_stack_top returns 0x3fc, then (PSCI_PERCPU_STACK_SIZE - 4) is right value. But the psci_get_stack_top returns 0x400 actually, I've tried to change the code to return value like 0x3fc but found out that it isn't needed, because when push operation executed , sp is subtracted by 4 first and then data is saved into where pointed by the decreased sp. So I canceled changed to stack top but forgot to update this macro definiation.
Regards ChenYu
#ifndef __ASSEMBLY__ int psci_update_dt(void *fdt); void psci_board_init(void); -- 2.1.4