[U-Boot] [PATCH v5 3/8] ARMv7: PSCI: update the place of saving target PC

[Hongbo Zhang]

On Tue, Jun 28, 2016 at 11:24 AM, Chen-Yu Tsai <wens at csie.org> wrote:
> On Tue, Jun 14, 2016 at 3:01 PM,  <macro.wave.z at gmail.com> wrote:
>> From: Hongbo Zhang <hongbo.zhang at 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 at nxp.com>
>> Signed-off-by: Wang Dongsheng <dongsheng.wang at 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_OFFSET
>> +       str     r2, [r5]                        @ store target PC
>
> IMO 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_OFFSET
>> +       ldr     r0, [r0]                        @ get target PC
>>         b       _do_nonsec_entry
>>  ENDPROC(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
>>