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

On 25/05/16 03:14, Chen-Yu Tsai wrote:

On Tue, May 24, 2016 at 4:41 PM, Marc Zyngier marc.zyngier@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@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@arm.com
• • Based on code by Carl van Schaik carl@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.