[U-Boot] [PATCH 10/10] sunxi: Add PSCI implementation in C

[Marc Zyngier]

On 25/05/16 03:14, Chen-Yu Tsai wrote:
> On Tue, May 24, 2016 at 4:41 PM, Marc Zyngier <marc.zyngier at arm.com> wrote:
>> On 23/05/16 13:41, Chen-Yu Tsai wrote:
>>> To make the PSCI backend more maintainable and easier to port to newer
>>> SoCs, rewrite the current PSCI implementation in C.
>>>
>>> Some inline assembly bits are required to access coprocessor registers.
>>> PSCI stack setup is the only part left completely in assembly. In theory
>>> this part could be split out of psci_arch_init into a separate common
>>> function, and psci_arch_init could be completely in C.
>>>
>>> Signed-off-by: Chen-Yu Tsai <wens at csie.org>
>>> ---
>>>  arch/arm/cpu/armv7/sunxi/Makefile     |   7 +-
>>>  arch/arm/cpu/armv7/sunxi/psci.c       | 229 +++++++++++++++++++++++++++++
>>>  arch/arm/cpu/armv7/sunxi/psci_head.S  |  61 ++++++++
>>>  arch/arm/cpu/armv7/sunxi/psci_sun6i.S | 262 ----------------------------------
>>>  arch/arm/cpu/armv7/sunxi/psci_sun7i.S | 237 ------------------------------
>>>  5 files changed, 292 insertions(+), 504 deletions(-)
>>>  create mode 100644 arch/arm/cpu/armv7/sunxi/psci.c
>>>  create mode 100644 arch/arm/cpu/armv7/sunxi/psci_head.S
>>>  delete mode 100644 arch/arm/cpu/armv7/sunxi/psci_sun6i.S
>>>  delete mode 100644 arch/arm/cpu/armv7/sunxi/psci_sun7i.S

[...]

>>> diff --git a/arch/arm/cpu/armv7/sunxi/psci_head.S b/arch/arm/cpu/armv7/sunxi/psci_head.S
>>> new file mode 100644
>>> index 000000000000..40b350636e32
>>> --- /dev/null
>>> +++ b/arch/arm/cpu/armv7/sunxi/psci_head.S
>>> @@ -0,0 +1,61 @@
>>> +/*
>>> + * Copyright (C) 2013 - ARM Ltd
>>> + * Author: Marc Zyngier <marc.zyngier at arm.com>
>>> + *
>>> + * Based on code by Carl van Schaik <carl at ok-labs.com>.
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope that it will be useful,
>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + * GNU General Public License for more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License
>>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>>> + */
>>> +
>>> +#include <config.h>
>>> +
>>> +#include <asm/arch-armv7/generictimer.h>
>>> +#include <asm/gic.h>
>>> +#include <asm/macro.h>
>>> +#include <asm/psci.h>
>>> +#include <asm/arch/cpu.h>
>>> +
>>> +/*
>>> + * Memory layout:
>>> + *
>>> + * SECURE_RAM to text_end :
>>> + *   ._secure_text section
>>> + * text_end to ALIGN_PAGE(text_end):
>>> + *   nothing
>>> + * ALIGN_PAGE(text_end) to ALIGN_PAGE(text_end) + 0x1000)
>>> + *   1kB of stack per CPU (4 CPUs max).
>>> + */
>>> +
>>> +     .pushsection ._secure.text, "ax"
>>> +
>>> +     .arch_extension sec
>>> +
>>> +#define      GICD_BASE               (SUNXI_GIC400_BASE +  0x1000)
>>> +#define      GICC_BASE               (SUNXI_GIC400_BASE +  0x2000)
>>> +
>>> +.globl       psci_arch_init
>>> +psci_arch_init:
>>> +     mov     r6, lr
>>> +     bl      psci_get_cpu_id         @ CPU ID => r0
>>> +     bl      psci_get_cpu_stack_top  @ stack top => r0
>>> +     sub     r0, r0, #4              @ Save space for target PC
>>> +     mov     sp, r0
>>> +     mov     lr, r6
>>> +
>>> +     push    {r0, r1, r2, ip, lr}
>>> +     bl      sunxi_gic_init
>>> +     pop     {r0, r1, r2, ip, pc}
>>
>> I'm a bit sceptical with this sequence. You're saving registers that may
>> be clobbered by the called C code, but you're missing r3. But more
>> fundamentally, you're saving these registers after having already
>> clobbered them (r0). To me, you should be able to replace these three
>> instructions with a single:
>>
>>         b       sunxi_gic_init
>>
>> Or am I missing something?
> 
> This gets called at the top of the secure monitor procedure, which itself
> is entered via the smc call in _do_nonsec_entry(). _do_nonsec_entry() puts
> whatever arguments in r0 ~ r2, and the entry point in ip.
> 
> _do_nonsec_entry() is called in 2 places:
> 
> a) the PSCI entry point for secondary cores. For this part we only care
> about the entry point.
> 
> b) The Linux kernel entry point (see arch/arm/lib/bootm.c), which results
> in {r0, r1, r2, ip} = {0, mach_id, dt_addr, kernel_entry}. I'm not sure if
> the kernel doesn't care about r0, but we could reset it back to 0 at the
> end of the code above. What must be saved here are r1, r2, and lr.

Right. I completely forgot how this thing worked. It makes sense then.
But we definitely should preserve r0, as it is part of the calling
convention. And for the sake of being generic, don't just reset it to
zero, but preserve it from the beginning.

> I think we can split out the stack setup stuff into a separate function
> that gets called explicitly before psci_arch_init, and psci_arch_init
> should just stick to the ARM calling convention. However I intended this
> series to be mostly sunxi specific, and do the cross platform refactoring
> in a followup series.

Yeah, I'm not too worried about that just yet.

> Thanks for the thorough review. For the first version I wanted something
> that works and closely resembles the original to the point that the
> disassembled code can be matched to the original to aid in working out
> issues.

No problem, you're doing some good job here.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...