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

[Chen-Yu Tsai]

On Tue, Jun 28, 2016 at 5:23 PM, Hongbo Zhang <macro.wave.z at gmail.com> wrote:
> 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?

That's the idea. Having common save/get helper functions for target
PC, context ID, power state, etc. which others can use in their
platform specific code should reduce duplicate code and make it
easier to maintain. The platform code shouldn't care how or where
this stuff is stored, as long as it can be reliably used. With
your patches they are stored at the bottom of the stack. With mine
they are moved to a secure data section.

>
>> 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.

Originally the PSCI implementation was contained in one file, written
by one person. It was easy to check if registers were used. Now it's
split up into a number of files across different platforms, with sunxi
even using C. It's time to start paying attention to these details.

> Sure saving register context makes it safer.

Thanks. With sunxi C code, the compiler might use these registers,
and we might get weird issues after calling such a common function.

>
>>
>>>         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.

Thanks. The series was pulled in later than expected.

>>>
>>>         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.

Let's define "stack top" as the address of SP when the stack is empty.

> 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.

Yes. You are right. This is what I alluded to.

Regards
ChenYu

>
>>
>> Regards
>> ChenYu
>>
>>> +
>>>  #ifndef __ASSEMBLY__
>>>  int psci_update_dt(void *fdt);
>>>  void psci_board_init(void);
>>> --
>>> 2.1.4
>>>