From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Date: Tue, 24 May 2016 09:41:13 +0100 Subject: [U-Boot] [PATCH 10/10] sunxi: Add PSCI implementation in C In-Reply-To: <1464007306-30269-11-git-send-email-wens@csie.org> References: <1464007306-30269-1-git-send-email-wens@csie.org> <1464007306-30269-11-git-send-email-wens@csie.org> Message-ID: <574413A9.2090105@arm.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > --- > 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/Makefile b/arch/arm/cpu/armv7/sunxi/Makefile > index 4d2274a38ed1..c2085101685b 100644 > --- a/arch/arm/cpu/armv7/sunxi/Makefile > +++ b/arch/arm/cpu/armv7/sunxi/Makefile > @@ -13,11 +13,8 @@ obj-$(CONFIG_MACH_SUN6I) += tzpc.o > obj-$(CONFIG_MACH_SUN8I_H3) += tzpc.o > > ifndef CONFIG_SPL_BUILD > -ifdef CONFIG_ARMV7_PSCI > -obj-$(CONFIG_MACH_SUN6I) += psci_sun6i.o > -obj-$(CONFIG_MACH_SUN7I) += psci_sun7i.o > -obj-$(CONFIG_MACH_SUN8I) += psci_sun6i.o > -endif > +obj-$(CONFIG_ARMV7_PSCI) += psci.o > +obj-$(CONFIG_ARMV7_PSCI) += psci_head.o > endif > > ifdef CONFIG_SPL_BUILD > diff --git a/arch/arm/cpu/armv7/sunxi/psci.c b/arch/arm/cpu/armv7/sunxi/psci.c > new file mode 100644 > index 000000000000..943061937f7c > --- /dev/null > +++ b/arch/arm/cpu/armv7/sunxi/psci.c > @@ -0,0 +1,229 @@ > +/* > + * Copyright (C) 2016 > + * Author: Chen-Yu Tsai > + * > + * Based on assembly code by Marc Zyngier , > + * which was based on code by Carl van Schaik . > + * > + * SPDX-License-Identifier: GPL-2.0 > + */ > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define __secure __attribute__ ((section ("._secure.text"))) > +#define __irq __attribute__ ((interrupt ("IRQ"))) > + > +#define GICD_BASE (SUNXI_GIC400_BASE + GIC_DIST_OFFSET) > +#define GICC_BASE (SUNXI_GIC400_BASE + GIC_CPU_OFFSET_A15) > + > +static void __secure __mdelay(u32 ms) > +{ > + u32 reg = DIV_ROUND_UP(CONFIG_TIMER_CLK_FREQ, ms); > + > + /* CNTP_TVAL */ > + asm volatile ("mcr p15, 0, %0, c14, c2, 0" : : "r" (reg)); Since you made the effort of switching to C code, please move all the asm statements into static helpers: static write_cntp_tval(u32 tval) { asm volatile("mcr p15, 0, %0, c14, c2, 0" : : "r" (tval)); } This has the benefit of making it obvious which CP15 register you're dealing with (specially further down when you're dealing with SCR). > + ISB; > + /* CNTP_CTL */ > + asm volatile ("mcr p15, 0, %0, c14, c2, 1" : : "r" (3)); > + > + do { > + ISB; > + /* CNTP_CTL */ > + asm volatile ("mrc p15, 0, %0, c14, c2, 1" : "=r" (reg) : : > + "cc" ); Why are the flags part of the clobber list? > + } while (!(reg & BIT(2))); > + > + /* CNTP_CTL */ > + asm volatile ("mcr p15, 0, %0, c14, c2, 1" : : "r" (0)); > +} > + > +void __secure sunxi_cpu_power_off(u32 cpuid) > +{ > +#ifdef CONFIG_SUNXI_GEN_SUN6I > + struct sunxi_prcm_reg *prcm = > + (struct sunxi_prcm_reg *)SUNXI_PRCM_BASE; > +#endif > + struct sunxi_cpucfg_reg *cpucfg = > + (struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE; > + u32 cpu = cpuid & 0x3; > + u32 tmp __maybe_unused; This doesn't look used at all. Why is it there? > + > + /* Wait for the core to enter WFI */ > + while (1) { > + if (readl(&cpucfg->cpu[cpu].status) & BIT(2)) > + break; > + __mdelay(1); > + } > + > + /* Assert reset on target CPU */ > + writel(0, &cpucfg->cpu[cpu].rst); > + > + /* Lock CPU (Disable external debug access) */ > + clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu)); > + > +#ifdef CONFIG_MACH_SUN7I > + /* Set power gating */ > + setbits_le32(&cpucfg->cpu1_pwroff, BIT(0)); > +#else > + /* Set power gating */ > + setbits_le32(&prcm->cpu_pwroff, BIT(cpu)); > +#endif > + > +#ifdef CONFIG_MACH_SUN7I > + /* Activate power clamp */ > + writel(0xff, &cpucfg->cpu1_pwr_clamp); > +#elif defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN8I_H3) > + /* Activate power clamp */ > + writel(0xff, &prcm->cpu_pwr_clamp[cpu]); > +#endif Why don't you put all the #ifdefery in two helper functions (one for syn7i, one for all the others)? This would look a lot better. > + > + /* Unlock CPU (Disable external debug access) */ > + setbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu)); > +} > + > +/* > + * Although this is an FIQ handler, the FIQ is processed in monitor mode, > + * which means there's no FIQ banked registers. This is the same as IRQ > + * mode, so use the IRQ attribute to ask the compiler to handler entry > + * and return. > + */ > +void __secure __irq psci_fiq_enter(void) > +{ > + u32 scr, reg, cpu; > + > + /* Switch to secure mode */ > + asm volatile ("mrc p15, 0, %0, c1, c1, 0" : "=r" (scr) : : "cc"); > + reg = scr & ~(1 << 0); > + asm volatile ("mcr p15, 0, %0, c1, c1, 0" : : "r" (reg) : "cc"); > + ISB; Same question about the flags. > + > + /* Validate reason based on IAR and acknowledge */ > + reg = readl(GICC_BASE + GICC_IAR); > + > + /* Skip spurious interrupts 1022 and 1023 */ > + if (reg == 1023 || reg == 1022) > + goto out; > + > + /* Acknowledge interrupt */ No, this is an End Of Interrupt. The Acknowledge is done by reading GICC_IAR. > + writel(reg, GICC_BASE + GICC_EOIR); > + DSB; > + > + /* Get CPU number */ > + cpu = (reg >> 10) & 0xf; /* but GIC specs say only 3 bits? */ Indeed, only GICC_IAR[12:10]. Seems like a bug in the original code, no need to reproduce it here. > + > + /* Power off the CPU */ > + sunxi_cpu_power_off(cpu); > + > +out: > + /* Restore security level */ > + asm volatile ("mcr p15, 0, %0, c1, c1, 0" : : "r" (scr) : "cc"); Same question about flags. > +} > + > +int __secure psci_cpu_on(u32 unused __always_unused, u32 mpidr, u32 pc) > +{ > +#ifdef CONFIG_SUNXI_GEN_SUN6I > + struct sunxi_prcm_reg *prcm = > + (struct sunxi_prcm_reg *)SUNXI_PRCM_BASE; > +#endif > + struct sunxi_cpucfg_reg *cpucfg = > + (struct sunxi_cpucfg_reg *)SUNXI_CPUCFG_BASE; > + u32 cpu = (mpidr & 0x3); > + u32 tmp __maybe_unused; > + > + /* store target PC at target CPU stack top */ > + writel(pc, psci_get_cpu_stack_top(cpu)); > + DSB; > + > + /* Set secondary core power on PC */ > + writel((u32)&psci_cpu_entry, &cpucfg->priv0); > + > + /* Assert reset on target CPU */ > + writel(0, &cpucfg->cpu[cpu].rst); > + > + /* Invalidate L1 cache */ > + clrbits_le32(&cpucfg->gen_ctrl, BIT(cpu)); > + > + /* Lock CPU (Disable external debug access) */ > + clrbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu)); > + > +#ifdef CONFIG_MACH_SUN7I > + /* Release power clamp */ > + tmp = 0x1ff; > + do { > + tmp >>= 1; > + writel(tmp, &cpucfg->cpu1_pwr_clamp); > + } while (tmp); > +#elif defined(CONFIG_MACH_SUN6I) || defined(CONFIG_MACH_SUN8I_H3) > + /* Release power clamp */ > + tmp = 0x1ff; > + do { > + tmp >>= 1; > + writel(tmp, &prcm->cpu_pwr_clamp[cpu]); > + } while (tmp); > +#endif > + > + __mdelay(10); > + > +#ifdef CONFIG_MACH_SUN7I > + /* Clear power gating */ > + clrbits_le32(&cpucfg->cpu1_pwroff, BIT(0)); > +#else > + /* Clear power gating */ > + clrbits_le32(&prcm->cpu_pwroff, BIT(cpu)); > +#endif Same remark about having per variant helpers. This will get rid of your __maybe_unused attribute. > + > + /* De-assert reset on target CPU */ > + writel(BIT(1) | BIT(0), &cpucfg->cpu[cpu].rst); > + > + /* Unlock CPU (Disable external debug access) */ > + setbits_le32(&cpucfg->dbg_ctrl1, BIT(cpu)); > + > + return ARM_PSCI_RET_SUCCESS; > +} > + > +void __secure psci_cpu_off(void) > +{ > + psci_cpu_off_common(); > + > + /* Ask CPU0 via SGI15 to pull the rug... */ > + writel(BIT(16) | 15, GICD_BASE + GICD_SGIR); > + DSB; > + > + /* Wait to be turned off */ > + while (1) > + wfi(); > +} > + > +void __secure sunxi_gic_init(void) > +{ > + u32 reg; > + > + /* SGI15 as Group-0 */ > + clrbits_le32(GICD_BASE + GICD_IGROUPRn, BIT(15)); > + > + /* Set SGI15 priority to 0 */ > + writeb(0, GICD_BASE + GICD_IPRIORITYRn + 15); > + > + /* Be cool with non-secure */ > + writel(0xff, GICC_BASE + GICC_PMR); > + > + /* Switch FIQEn on */ > + setbits_le32(GICC_BASE + GICC_CTLR, BIT(3)); > + > + asm volatile ("mrc p15, 0, %0, c1, c1, 0" : "=r" (reg) : : "cc"); > + reg |= BIT(2); /* Enable FIQ in monitor mode */ > + reg &= ~BIT(0); /* Secure mode */ > + asm volatile ("mcr p15, 0, %0, c1, c1, 0" : : "r" (reg) : "cc"); Flags, helpers... > + ISB; > +} > 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 > + * > + * Based on code by Carl van Schaik . > + * > + * 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 . > + */ > + > +#include > + > +#include > +#include > +#include > +#include > +#include > + > +/* > + * 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? Thanks, M. -- Jazz is not dead. It just smells funny...